Correction of issue #3678 (Symbol list does not get the focus when access through key binding) You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3681
-- Commit Summary --
* Correction of issue #3678 (Symbol list does not get the focus when access through key binding
-- File Changes --
M src/sidebar.c (11)
-- Patch Links --
https://github.com/geany/geany/pull/3681.patch https://github.com/geany/geany/pull/3681.diff
One issue with this is that it always focuses the treeview. If the entry was focused, it will change the focus to the treeview. I think the entry may be more useful, because even if the treeview focus is wanted, the user can just press the down key. To do that, this lookup works: ``` gtk_widget_grab_focus(ui_lookup_widget(main_widgets.window, "entry_tagfilter")); ```
Or the normal tab/shift tab navigation can go between entry and tree (and back).
I confirm that ``` gtk_widget_grab_focus(ui_lookup_widget(main_widgets.window, "entry_tagfilter")); ``` works for me, and I too really don't understand why the same call on `treeview2` (the initial correction I suggested) doesn't work...
About focusing the tree or the entry, there is something I don't quite understand here: what's the point of the new entry ? I was used to a version of Geany without the entry widget. After focusing the tree, if I input something it automatically opens an entry widget at the bottom of the tree, which seems to work exactly as the new entry, and which still exists in the new version. Hence, to me, the new entry widget only waste one line that could be used for the tree, and require one more action (arrow or tab) to do something that could, and still can, be done without using it.
The new entry is a filter, it reduces the tree to matching elements, the one that pops up when you type on the tree is a search, it changes the selected element to the first match.
Oh, I've just noticed the difference between the two ! Sorry for my previous comment and thanks for the clarification. That's a nice feature addition indeed.
I'm so use to the search mode only that I think I would prefer to keep the tree focused, but I would understand that someone else think the other way, in which case the correction suggested by @ntrel is surely better than mine (shorter and more readable).
Using the name is in theory more robust to further layout changes, but "treeview2" should be too, but isn't. I have no preference so will referee any duel between users that eventuates ;-)
I don't really care either for which it is; but maybe restoring this to the previous state is the safer solution. FWIW, I don't use that shortcut (didn't even know it existed :smile:) so I don't mind.
works for me, and I too really don't understand why the same call on `treeview2` (the initial correction I suggested) doesn't work...
This is more subtle :) The reason is that the actual `GtkTreeView` is either the default empty one, or the *document's* one. So the one in Glade (retrieved with the `treeview2` name) is likely not the one actually attached to the symbols tab. The correct fix is:
```diff diff --git a/src/sidebar.c b/src/sidebar.c index 9d004f392..435e31394 100644 --- a/src/sidebar.c +++ b/src/sidebar.c @@ -1649,7 +1649,7 @@ void sidebar_focus_symbols_tab(void) if (ui_prefs.sidebar_visible && interface_prefs.sidebar_symbol_visible) { GtkNotebook *notebook = GTK_NOTEBOOK(main_widgets.sidebar_notebook); - GtkWidget *symbol_list_scrollwin = gtk_notebook_get_nth_page(notebook, TREEVIEW_SYMBOL); + GtkWidget *symbol_list_scrollwin = ui_lookup_widget(main_widgets.window, "scrolledwindow2");
gtk_notebook_set_current_page(notebook, TREEVIEW_SYMBOL); gtk_widget_grab_focus(gtk_bin_get_child(GTK_BIN(symbol_list_scrollwin))); ```
@b4n requested changes on this pull request.
This is way too specific and fragile. Do this instead:
```diff diff --git a/src/sidebar.c b/src/sidebar.c index 9d004f392..435e31394 100644 --- a/src/sidebar.c +++ b/src/sidebar.c @@ -1649,7 +1649,7 @@ void sidebar_focus_symbols_tab(void) if (ui_prefs.sidebar_visible && interface_prefs.sidebar_symbol_visible) { GtkNotebook *notebook = GTK_NOTEBOOK(main_widgets.sidebar_notebook); - GtkWidget *symbol_list_scrollwin = gtk_notebook_get_nth_page(notebook, TREEVIEW_SYMBOL); + GtkWidget *symbol_list_scrollwin = ui_lookup_widget(main_widgets.window, "scrolledwindow2");
gtk_notebook_set_current_page(notebook, TREEVIEW_SYMBOL); gtk_widget_grab_focus(gtk_bin_get_child(GTK_BIN(symbol_list_scrollwin))); ```
@BayashiPascal pushed 1 commit.
0c099631b18d5ce14216ce5b2ea60347624dfdff Correction of issue #3678 with modification suggested by @b4n instead of my previous one
I've tested @b4n correction and it works for me. I agree that using `scrolledwindow2` is a better way of correcting the problem, so I've modified the PR accordingly.
@b4n requested changes on this pull request.
LGTM (obviously) but for the tiny style issue.
@@ -1649,8 +1649,7 @@ void sidebar_focus_symbols_tab(void)
if (ui_prefs.sidebar_visible && interface_prefs.sidebar_symbol_visible) { GtkNotebook *notebook = GTK_NOTEBOOK(main_widgets.sidebar_notebook); - GtkWidget *symbol_list_scrollwin = gtk_notebook_get_nth_page(notebook, TREEVIEW_SYMBOL); - + GtkWidget *symbol_list_scrollwin = ui_lookup_widget(main_widgets.window, "scrolledwindow2");
Please keep the blank line between the variable declaration block and the rest (yes, it's kind of the same here, but that's currently how we do things everywhere else)
```suggestion GtkWidget *symbol_list_scrollwin = ui_lookup_widget(main_widgets.window, "scrolledwindow2");
```
@BayashiPascal pushed 1 commit.
c70f3afa58295a6371eba14bdcb4303d07d925ec Correction code style in src/sidebar.c (blank line after variable declaration block)
@b4n approved this pull request.
LGTM
@b4n pushed 17 commits.
ffad343f21be323b55b3ca44c6f2d0d4d9a008fb Fix harmless GCC warning 7f630aaf917b733c76640574610017fc3a7c15ab Fix fairly nasty implicit integer conversion 73658f185b0398b2ec8610d6060f84033906001d "Fix" passing const arguments to spawn functions 910a7e651fb102fae18e788080da2666f31bd6c4 Add casts for irrelevant -Wwrite-strings warnings 0e0a96ef8e7fec5117f67f3aea1bcfa685d3f6d0 Remove invalid placeholder code 727e8cde61d5357a63538df96ab4ca6c4cc8f931 Use g_*list_free_full() instead of g_*list_foreach() f3d466a359c9c501d815938602f24dff6ab622ff Avoid an easy-to-fix function cast 09b1558d3496a193d2efdd2f99adcb321743fffb Silence warnings on appropriate GSourceFunc casts 2705dd623e5c5b88679f7c2e790c3af5e371025b Fix variable shadowing 496519d6631dfdef8d41559c642deac933e22f97 Only pass literals as format strings for e.g. printf-style functions 50f4178ecce6abf802a44f563107bd0bca90720a Do not use GtkResponseType as response ID type c42004402ecefea7d809d4a51f7b427937ff4270 Silence some -Wcast-function-type warnings 88a0bfcb3618b2533a5e7e253c7ef7af3c0b0590 Merge pull request #3665 from b4n/less-warnings 0629be7ac2b97dc254fc51f1beda8dfe83c952a8 Add `.mjs` extension for JavaScript e5680fe85de536fc61ff0f2d4eadc54171d6c982 Update Pascal filedef and extension mappings (#3694) aaaf1ed59b56aecb5fb8d76b93a177b3652f3a17 Merge pull request #3698 from TheDcoder/patch-1 97b48e285e8624d04db00788fa22ff79adb45153 Merge pull request #3681 from BayashiPascal/master
@b4n pushed 1 commit.
ea2458e4472fe0d42d4e8ae460596b561b1d37c7 Fix focusing the symbol list through key binding
Merged #3681 into master.
@ntrel @elextr I'm merging this, as if fixes a clear bug and restores the previous behavior. If you'd like to focus the search field instead (which indeed might make sense, but I don't have a strong opinion yet), feel free to open a follow-up.
@BayashiPascal I squashed the commits together and adjusted the commit message to make it clearer. Thanks for the patch! :+1:
@b4n You're welcome. I'm glad to have been able to contribute and hope I can do it again in the future.
If you'd like to focus the search field instead (which indeed might make sense, but I don't have a strong opinion yet)
Nah, I use the moose, so I can click on what I want.
github-comments@lists.geany.org