Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter symbols in the Symbol List (new feature) #2657

Closed
wants to merge 1 commit into from

Conversation

dmitryunruh
Copy link
Contributor

It adds an entry field at the top of the symbol list to filter this list.

The text of the entry field divides into parts with a space symbol. A symbol of the symbol list (tag) must contain each of these parts to be shown in the filtered list. Filtering applies immediately during changing the text of the entry field.

techee added a commit to techee/geany that referenced this pull request Dec 9, 2021
This patch fixes symbol tree construction when geany#2657 is applied.

When a filter for tags is used for the symbol tree, it may happen that
parent tags in the symbol tree are removed because they don't match
the filter. Such tags are still visualised nicely by the symbol tree
which shows these tags with full scope like

MissingStruct::some_member

However, the current code doesn't expect that it may happen that
MissingStruct re-appears in the results when filter is deleted. In such
case 'some_member' isn't re-parented under MissingStruct and stays
displayed in the above format.

This patch simply removes tags like 'MissingStruct::some_member' for
which parent hasn't been constructed yet from the symbol tree and re-adds
them in the "insertion pass" for the symbol tree update code.

The only change this patch does is that it replaces

if (parent_name && ! g_hash_table_lookup(parents_table, parent_name))
    parent_name = NULL;

with

if (!tm_tags_equal(tag, found))
    cont = tree_store_remove_row(store, &iter);
else
{

The rest is just indentation after the 'else'.
techee added a commit to techee/geany that referenced this pull request Dec 9, 2021
This patch fixes symbol tree construction when geany#2657 is applied.

When a filter for tags is used for the symbol tree, it may happen that
parent tags in the symbol tree are removed because they don't match
the filter. Such tags are still visualised nicely by the symbol tree
which shows these tags with full scope like

MissingStruct::some_member

However, the current code doesn't expect that it may happen that
MissingStruct re-appears in the results when filter is deleted. In such
case 'some_member' isn't re-parented under MissingStruct and stays
displayed in the above format.

This patch simply removes tags like 'MissingStruct::some_member' for
which parent hasn't been constructed yet from the symbol tree and re-adds
them in the "insertion pass" for the symbol tree update code.

The only change this patch does is that it replaces

if (parent_name && ! g_hash_table_lookup(parents_table, parent_name))
    parent_name = NULL;

with

if (parent_name && ! g_hash_table_lookup(parents_table, parent_name))
    cont = tree_store_remove_row(store, &iter);
else
{

The rest is just indentation after the 'else'.
@techee
Copy link
Member

techee commented Dec 9, 2021

@dmitryunruh I'd love to see this feature in Geany!

@eht16 @elextr What do you think about this feature? Personally I'd love to be able to filter the symbol tree in a similar way like the plugins list. Also, the patch is quite tiny and limited in scope so it shouldn't introduce any problems.

I had a look at the patch and apart from some style changes it seems to do the right thing in principle (I could do a proper review if there's an interest in this feature). The searchbar is currently placed directly above the symbol tree without anything and it might look better if it were placed inside a panel or something so there's something around it. It might also be better to perform case-insensitive filtering, but these are just minor points.

I created #3015 to address a minor problem where the tree didn't get re-constructed right after cancelling the search.

@eht16
Copy link
Member

eht16 commented Dec 9, 2021

s/I created #3015 to/I created #3050 to/ :)

To the topic:

  • looks quite good!
  • I gave it a quick try and works as advertised.
  • I agree on better searching case-insensitive.
  • maybe the update of the symbol list on each notebook tab change might be a noticeable performance hit on older machines ("unfortunately" I've recently got a new notebook with a bleeding fast CPU so I won't notice :D).

I see why the update on notebook tab change is necessary; the symbol filter text is static and applies to all documents. I think it would be even better to have filters per document, otherwise it could be confusing if I switch to another document and suddenly the symbol list is empty. After wondering about it, the user will probably realize it's because of the still set filter and then will clear the filter.
Though having per document filters obviously requires much more efforts :(.

@techee
Copy link
Member

techee commented Dec 9, 2021

maybe the update of the symbol list on each notebook tab change might be a noticeable performance hit on older machines ("unfortunately" I've recently got a new notebook with a bleeding fast CPU so I won't notice :D).

That shouldn't be a problem - update of the symbol tree is performed every time the document is re-parsed while typing and if the machine is too slow, editing the document itself would be a much bigger problem already.

I think it would be even better to have filters per document, otherwise it could be confusing if I switch to another document and suddenly the symbol list is empty. After wondering about it, the user will probably realize it's because of the still set filter and then will clear the filter.

Good point, I didn't think about that.

Though having per document filters obviously requires much more efforts :(.

What about a "lazy" solution to clear the filter automatically on a tab change?

@techee
Copy link
Member

techee commented Dec 9, 2021

One more nice thing to have after playing with it would be to highlight the first item in the tree after pressing enter (but not confirming it so arrows can be used to select the right item).

"unfortunately" I've recently got a new notebook with a bleeding fast CPU so I won't notice :D

Same sad story here with the 8-core ARM Mac :-).

@elextr
Copy link
Member

elextr commented Dec 9, 2021

"unfortunately" I've recently got a new notebook with a bleeding fast CPU so I won't notice :D

Same sad story here with the 8-core ARM Mac :-).

Sigh, boys and their toys 😄

Though having per document filters obviously requires much more efforts :(.

What about a "lazy" solution to clear the filter automatically on a tab change?

Then the user would be confused on switching back to the original tab to find their filter gone?

@techee
Copy link
Member

techee commented Dec 9, 2021

Then the user would be confused on switching back to the original tab to find their filter gone?

The question is whether not to even clear it after selecting an item in the symbol tree. The use case for this feature I had in mind was to just get quickly to the right symbol in the tree and navigate to it and not really having the tree filtered permanently (and having to manually clear it later).

@techee
Copy link
Member

techee commented Dec 9, 2021

The question is whether not to even clear it after selecting an item in the symbol tree. The use case for this feature I had in mind was to just get quickly to the right symbol in the tree and navigate to it and not really having the tree filtered permanently (and having to manually clear it later).

Hmm, I tried it but it's strange. So if it's on a per-document basis, the filter string should probably go to GeanyDocumentPrivate.

@elextr
Copy link
Member

elextr commented Dec 10, 2021

The use case for this feature I had in mind was to just get quickly to the right symbol in the tree and navigate to it and not really having the tree filtered permanently (and having to manually clear it later).

Ahh, so its a search facility, not really a filter. Would be good, the current search is useless unless you happen to want the first occurrence.

So as a search then yeah it should go away on loss of focus really.

@techee
Copy link
Member

techee commented Dec 10, 2021

Ahh, so its a search facility, not really a filter. Would be good, the current search is useless unless you happen to want the first occurrence.

But it turned out to be weird when I tried it and it really should behave as a (permanent until cleared) filter. Some things I discovered in addition:

  1. The filter should not probably be applied against just name but the whole scope of a symbol - it's strange to see a class without its members after filtering for its name.
  2. The stuff from Fix setting parents in symbol tree when filtering is used #3050 should only be applied when filter changes. For C++ classes you get
MyClass::foo()
MyClass::bar()
MyClass::baz()

in the symbol tree because MyClass is defined in the header and its tag isn't present in the source file. That means lots of removals of symbols in the first pass of the symbol tree update when the patch is applied which causes scrolling of the symbol tree when editing file.

Sigh, I thought this would be a nice little patch, quick to apply, and here it comes again :-).

@techee
Copy link
Member

techee commented Dec 10, 2021

The stuff from Fix setting parents in symbol tree when filtering is used #3050 should only be applied when filter changes

Or, to avoid making the code more complicated in the tree update function, just to remove everything from the tree when filter changes and get the whole tree re-populated. The main purpose of the complicated code in the symbol tree update function is to avoid scrolling when a change is made in editor but when applying filter, this will happen anyway because the changes in the tree are big.

@eht16
Copy link
Member

eht16 commented Dec 11, 2021

JFTR, I didn't mean to suggest it should be a per-document filter. Only that it would maybe create the most comfortable solution but also with the most effort needed.

Getting back to @techee's first idea of this feature: when we want tojust jump to the first match of a symbol, it might be enough to enable GTK's treeview search feature (where a little text input field pos up automagically and filters the tree, https://docs.gtk.org/gtk3/method.TreeView.set_enable_search.html).
Then we don't need a permanent visible search field and probably even less code. At the price thata search feature enabled by just typing in a list is less intuitive and might not be recognised by all users.
I didn't try it yet, it's just a thought.

In case someone wants to try, there might be signal handlers on the symbol list to switch focus to the editing widget on any keypress, at least it feels like this.

@elextr
Copy link
Member

elextr commented Dec 12, 2021

@eth16 if its only really a search facility and goes away as soon as something is selected, or the little text box cleared, or the tab switched then agree there is no need for it to be per-document.

But first match is not very useful for languages that are not C. C is ok because everything is distinguished by name and there is no overloading but all modern languages allow and even encourage multiple occurrences of the same name.

And anyway I think enable_search is set by default because first match works now, just type on the symbols pane and it selects first match, but can't go to subsequent matches (AFAICT). The default value of enable_search is not documented that I could find, and we explicitly turn if off for all the message window treeviews.

So unless any change improves the capability for multiple search by either having a "next" or by filtering it won't add much.

@techee
Copy link
Member

techee commented Dec 13, 2021

I created #3055 with some of the improvements discussed here.

@eht16
Copy link
Member

eht16 commented Jan 23, 2022

Closed by the merge of #3055.
Thanks @dmitryunruh and @techee.

@eht16 eht16 closed this Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants