I took the symbol tree filtering implementation from #2657 and applied some of the suggested changes on top of it: - made the filter entry per-document - made filtering case-insensitive - cleared the symbol tree completely when filtering to ensure it's fully re-created - filtering using full tag name including scope - focusing the symbol tree after pressing enter in the search entry
What do you think? You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3055
-- Commit Summary --
* Filter symbols in the Symbol List (new feature) * Merge branch 'tagfilter' of https://github.com/dmitryunruh/geany into dmitryunruh-tagfilter * Simplify the filtering code a bit and follow Geany style * Make tag filtering case-insensitive * Add search icon to the entry * Perform filtering in full name with scope * Clear symbol tree before filtering to ensure it's fully re-created * Focus the tree when pressing enter in the search entry * Use per-document filter for symbol tree
-- File Changes --
M data/geany.glade (45) M src/callbacks.c (44) M src/callbacks.h (6) M src/document.c (2) M src/documentprivate.h (2) M src/symbols.c (42)
-- Patch Links --
https://github.com/geany/geany/pull/3055.patch https://github.com/geany/geany/pull/3055.diff
Except two minor remarks, this is great! Tested and works very well. I'd expected that it required more code to make the filter per document but happy to be proven wrong :D.
Gentle prompt, its a UI change and new functionality so manual change also needed.
Except two minor remarks, this is great!
Where are those? Since I can't find them, maybe a minor remark of my own :-).
Maybe we should consider what widget in the sidebar gets the focus by default - right now it's the entry (and the tree focus is just one `tab` or `enter` press away) but maybe users who use primarily keyboard are more used to having the tree focused. I don't mind much myself as I just use mouse for the tree.
I'd expected that it required more code to make the filter per document but happy to be proven wrong :D.
Same here and it actually applies to the whole patch. I was thinking about this feature for a long time but thinking it would be annoying to do and then saw the pull request which was pretty trivial and mostly working so I got attracted :-).
Gentle prompt, its a UI change and new functionality so manual change also needed.
Sure, if it's something that's going to be merged, I'll definitely update it.
@eht16 requested changes on this pull request.
sidebar_select_openfiles_item(doc);
ui_save_buttons_toggle(doc->changed); ui_set_window_title(doc); ui_update_statusbar(doc, -1); ui_update_popup_reundo_items(doc); ui_document_show_hide(doc); /* update the document menu */ build_menu_update(doc); - sidebar_update_tag_list(doc, FALSE); + if (g_strcmp0(entry_text, doc->priv->tag_filter) != 0) + gtk_entry_set_text(filter_entry, doc->priv->tag_filter);
If the text of the filter entry is changed because it differs from the document's filter, `gtk_entry_set_text()` will eventually emit the "changed" signal and in its handler `sidebar_update_tag_list()` is called and then below it is called again. So if the new or previous document have filters set and they don't equal, we update the symbol list twice on each notebook page change.
Maybe we could just make the symbol list update below dependent of the if condition above?
g_return_val_if_fail(doc, NULL);
if (! doc->tm_file || ! doc->tm_file->tags_array) return NULL;
+ tf_strv = g_strsplit_set(doc->priv->tag_filter, " ", -1);
This means we can have multiple filters seperarted by space which are then combined (AND)? At least it feels so while testing. This is cool but might not be too obvious to the user. Maybe document this in the widget's tooltip?
Except two minor remarks, this is great!
Where are those? Since I can't find them, maybe a minor remark of my own :-).
Sorry, I forgot to submit my review. Now done.
Maybe we should consider what widget in the sidebar gets the focus by default - right now it's the entry (and the tree focus is just one `tab` or `enter` press away) but maybe users who use primarily keyboard are more used to having the tree focused. I don't mind much myself as I just use mouse for the tree.
Same here and not sure what would be the best default.
I'd expected that it required more code to make the filter per document but happy to be proven wrong :D.
Same here and it actually applies to the whole patch. I was thinking about this feature for a long time but thinking it would be annoying to do and then saw the pull request which was pretty trivial and mostly working so I got attracted :-).
:)
@techee commented on this pull request.
sidebar_select_openfiles_item(doc);
ui_save_buttons_toggle(doc->changed); ui_set_window_title(doc); ui_update_statusbar(doc, -1); ui_update_popup_reundo_items(doc); ui_document_show_hide(doc); /* update the document menu */ build_menu_update(doc); - sidebar_update_tag_list(doc, FALSE); + if (g_strcmp0(entry_text, doc->priv->tag_filter) != 0) + gtk_entry_set_text(filter_entry, doc->priv->tag_filter);
I _really_ hoped nobody would notice this issue. Dammit :-). I remember I couldn't find a simple solution with all the callbacks involved. I'll have a look at the code a little more to figure out something.
@techee commented on this pull request.
g_return_val_if_fail(doc, NULL);
if (! doc->tm_file || ! doc->tm_file->tags_array) return NULL;
+ tf_strv = g_strsplit_set(doc->priv->tag_filter, " ", -1);
OK, can add it - but AND-ing space separated search expressions shouldn't be too surprising for users as it's quite a standard behavior of similar "search engines".
@eht16 commented on this pull request.
sidebar_select_openfiles_item(doc);
ui_save_buttons_toggle(doc->changed); ui_set_window_title(doc); ui_update_statusbar(doc, -1); ui_update_popup_reundo_items(doc); ui_document_show_hide(doc); /* update the document menu */ build_menu_update(doc); - sidebar_update_tag_list(doc, FALSE); + if (g_strcmp0(entry_text, doc->priv->tag_filter) != 0) + gtk_entry_set_text(filter_entry, doc->priv->tag_filter);
Could the following maybe be enough: ```c if (g_strcmp0(entry_text, doc->priv->tag_filter) != 0) { gtk_entry_set_text(filter_entry, doc->priv->tag_filter); } else { sidebar_update_tag_list(doc, TRUE); } ``` ?
@techee pushed 3 commits.
f359765a499c40581085b4cc34e072d852e66434 Avoid sidebar_update_tag_list() called twice when using filter 23de061a822c65d695b511c39cf3bc8b49087376 Avoid console warning about invalid tag store 79c220597a2a888b6a206df58782c22d4ba5f1cc Mention the possibility to use more filters separated by space in tooltip
@techee commented on this pull request.
sidebar_select_openfiles_item(doc);
ui_save_buttons_toggle(doc->changed); ui_set_window_title(doc); ui_update_statusbar(doc, -1); ui_update_popup_reundo_items(doc); ui_document_show_hide(doc); /* update the document menu */ build_menu_update(doc); - sidebar_update_tag_list(doc, FALSE); + if (g_strcmp0(entry_text, doc->priv->tag_filter) != 0) + gtk_entry_set_text(filter_entry, doc->priv->tag_filter);
Yes, I guess it's enough. Done.
@techee commented on this pull request.
g_return_val_if_fail(doc, NULL);
if (! doc->tm_file || ! doc->tm_file->tags_array) return NULL;
+ tf_strv = g_strsplit_set(doc->priv->tag_filter, " ", -1);
I updated the tooltip similarly to the file browser plugin.
Gentle prompt, its a UI change and new functionality so manual change also needed.
I actually haven't found anything in the documentation regarding the symbol tree (unless I missed it) so I'd have to write some new section about it. In my opinion, the tooltip says it all and there shouldn't be any need for any extra explanation.
Maybe we should consider what widget in the sidebar gets the focus by default - right now it's the entry (and the tree focus is just one tab or enter press away) but maybe users who use primarily keyboard are more used to having the tree focused. I don't mind much myself as I just use mouse for the tree.
I kept the focus as it is now - it seems to behave quite OK and and it would require moving
https://github.com/geany/geany/blob/0601c73f7a7abc40fe9ddbff6b906b0d0e5b5f79...
to `sidebar.c` so it has access to the tree to call `gtk_widget_grab_focus()` and also updating
https://github.com/geany/geany/blob/0601c73f7a7abc40fe9ddbff6b906b0d0e5b5f79...
None of it is a problem to do though if someone wants to focus the tree by default.
I'm happy with current focus behavior and also with the rest. Great.
I'll leave @elextr a bit time to raise his veto, otherwise I'd like to merge soon.
@eht16 approved this pull request.
Ok, so we have abandoned the manual then? (This is not directed at @techee specifically, it is a general question, a number of recent changes have not been documented, but then thats what the code is for, right?)
Every time something is merged without documenting it "because there is nothing in the manual at the moment" is another time the manual becomes more useless to users.
[end grumpy]
Every time something is merged without documenting it "because there is nothing in the manual at the moment" is another time the manual becomes more useless to users.
I was just asking, I can write something if desired. I just haven't found any place where I should put it and none of the items in the right-click menu in symbol tree is documented either right now. The searchbar in the plugin manager isn't documented either.
I'm not sure if documenting obvious things like filtering just doesn't add noise to the documentation without any real value.
I was just asking
Sorry if you took it personally, as I said, my grump wasn't directed at you, we are all responsible for it.
I'm not sure if documenting obvious things like filtering just doesn't add noise to the documentation without any real value.
Its a good question, what are we targeting the manual at?
Its currently a random mixture of beginners info and experts info. Certainly Geany users are mostly some sort of developer (I count Markdown and similar writers as developers), but not necessarily the same type, a C expert may not be a HTML/web expert and vice versa. So things that are "obvious" to some users may not be obvious to others even if they are not "beginners", so at least mentioning capabilities to make them discoverable would be good.
As an example I would just add a subsection to the end of the "The Geany workspace" section that mentions the capabilities without going into chapter and verse about how to invoke them or needing @eht16 to add and maintain an image of his right click menus :smile:.
```rest Sidebar Usage ^^^^^^^^^^^^^
The sidebar has a right click menu that can control what is visible and has actions specific to the tab (other tabs added by plugins are described by that plugin documentation):
* Symbols Tab * expand/collapse the tree * control sorting order * locate the symbol in documents
The symbols tab can also be filtered by typing a symbol prefix into ... [@techee to fill in, don't forget when the filter autoremoves (if it does)]
* Documents * expand/collapse the tree * save to or reload from files * search tree based at selected file * show or hide the document paths ```
(Warning: my rEsT may need correcting)
Ok, so we have abandoned the manual then? (This is not directed at @techee specifically, it is a general question, a number of recent changes have not been documented, but then thats what the code is for, right?)
IMO "abandoned" is a bit too strong here. I try to remind myself and others of updating the manual but sometimes I consider additions also as not worth adding or just obvious while being aware that "obvious" is very subjective. Maybe we should pay some more attention to it but we also should try to not extend the already existing burdens for getting PRs created/merged even more.
I'm not sure if documenting obvious things like filtering just doesn't add noise to the documentation without any real value.
Its a good question, what are we targeting the manual at?
I guess the target audience is what you described: users with a very beginner level as well as "C experts". I think we can not and should not decrease the target audience.
In this case, I would agree with @techee that the filter feature (with its tooltip) is pretty much obvious. But since you already provided a good stub for a new section in the manual, it's enough to use and extend it. Great!
Sorry if you took it personally, as I said, my grump wasn't directed at you, we are all responsible for it.
No problem at all, I just wasn't sure what to do.
I'll use your stub (thanks) and update the docs using it.
IMO "abandoned" is a bit too strong here.
Sorry, but grumpy olde guys are required to go a bit overboard, its in our contract :smile:
users with a very beginner level as well as "C experts".
Because I have been trying other tools recently where I am what I would call an "expert beginner" I can appreciate how useful it is having at least a mention of features in the help/manual to point in the right direction.
Things like filters behave differently on different tools, neither Eclipse or Vscode have filters on symbols (AFAICT), and for example the Eclipse filter for the equivalent of the "Documents" sidebar has a tick list to filter by "C/C++ Files" and categories like that, so having an outline of what the Geany one does is worthwhile to reduce expectations and "how do I do XXX" issues (they become "feature request" issues instead :imp: ).
but we also should try to not extend the already existing burdens for getting PRs created/merged even more.
I think sometimes PR writers overestimate what I expect by "documentation" and I appreciate that English isn't everyone's native language, thats why I provided the outline as an example to show what I personally think is useful.
So long as there is some outline or mention of the feature, at least it indicates that there is some functionality in that area, where it is hiding (eg Eclipse document filter is hidden behind a funnel icon on the toolbar, not an entry like Geany symbols filter), and will show up when help is searched, which is totally sufficient for "expert beginners" and somewhere for "beginner beginners" to start from.
Remember the Geany "Manual" is also its "Help" and as it opens in a browser there is usually good search capability.
Rest assured I have no intention of turning the Geany Manual into a full 1000 page book :smile:
The outline can also be added to when the "Documents" filter [is added](https://github.com/geany/geany/issues/3078#issuecomment-1003739982).
@techee pushed 1 commit.
36bcb875ceaa3f2f02a083138e269ad99e926274 Add documentation about the filtering feature
Docs look fine to me.
Generally works for me for C, but there is a problem with Asciidoc and Rest where stray headings are catenated by `0003` characters, to reproduce open `geany.txt` and filter on `re`.
@techee pushed 4 commits.
0e205c5e31b62b95ed70e5bf18c2cc4dc53614e3 Rename tm_parser_context_separator() to tm_parser_scope_separator() d41b467776ad6ef3e1160eabedd17c8d843ba807 Rename tm_parser_has_full_context() to tm_parser_has_full_scope() 3939c24ae1911b8d45b0dc69401717bc5acb5e76 Use tm_parser_scope_separator() instead of symbols_get_context_separator() 63a5275a9daada15d0f6a0c0de60172cd430a9a3 Add tm_parser_scope_separator_printable()
Generally works for me for C, but there is a problem with Asciidoc and Rest where stray headings are catenated by 0003 characters, to reproduce open geany.txt and filter on re.
Good catch, I was thinking about it but then forgot to implement it. I added `tm_parser_scope_separator_printable()` which returns a printable representation of scope separator (for most languages identical to `tm_parser_scope_separator()`). I cleaned up the scope separator calls a bit on the way as it was easy to get confused by their different versions.
@techee why do we have conflicts?
This is because #3060 was merged and I used `tm_parser_scope_separator_printable()` also inside the calltip formatting function that was affected by that pull request.
How should I resolve the conflict - merge master to this branch, rebase, or possibly submit the last 4 patches as a separate pull request?
This is because #3060 was merged
Ahh, tripped yourself up
Probably this is a case where there is no choice but to rebase this (but not Git expert).
I did a hybrid approach - did reset to `HEAD~4` so the branch contained only the tag filter feature without any conflicts, then rebased it on top of master, and then re-wrote the last 4 patches manually - it was simpler than rebasing the whole branch.
Still LGTM, the stray headings problem seems solved.
Fine to merge?
I can't test anything for a while, but if ReSt and Asciidoc are fixed its fine with me.
I just tested the filtering for "re" in `geany.txt` as you mentioned above, this works. Though I don't have any Asciidoc files around to test with.
It was the same problem IIUC, so if rest is ok, asciidoc should be ok too (except for bugs :-) so don't wait for me.
Great.
@techee do you like to squash some of the commits?
@techee do you like to squash some of the commits?
Yeah, will do for the obvious "bugfix" ones.
Yeah, will do for the obvious "bugfix" ones.
Done.
Though I don't have any Asciidoc files around to test with.
All of the non-printable scope separators (and "strange" separators like `""`) are handled by
https://github.com/geany/geany/pull/3055/commits/bd93be412a5eb286cad978928d0...
the same way so when it works for rst, the same will work for Asciidoc too.
Merged #3055 into master.
Great! Thanks.
github-comments@lists.geany.org