Implement Tree Search Functionality with Highlighting and Filtering

[monxa]

Closes #192

Tested with GDExtension and Module.

Features:

• Tree highlighting: Highlights items that match the search query.
• Counting descendants: Shows the number of matching items within collapsed branches.
• Jump to next match: on enter.
• (Limbo-)Shortcut: Default CTRL-F.
• Menu entry: Misc->Search Tree.

Known issues:

• Unnecessarily redundant signals (af15dde)
• TreeSearch::_draw_highlight_item: uses one magic number (same as engine code)
• Performance
  • TreeSearch::_find_matching_entries et al (cc8f099)
    • Can be solved by making it iterative instead of recursive, using linear push_back instead of insert and sorting once.
  • Searches do a whole tree reconstruction for now: Noticeable lag. (Improve performance for TreeSearch monxa/limboai#1)
• Editor scaling needs fixing (380f80c)
• Searchbar spacing should be adjusted. (Already similar to Docs/Script search bar with 8c32395)
• Word highlight drawing might be too obstructive. Especially in filter mode. (Will be solved after this is merged)
• Jumping through entries currently inconsistent (8d29f16, 47706e9)
• Filtering unstable. (74cf985, 465757e)
• Decide if the current case matching behavior needs adjustment. Current: Match case when input includes capital letter.

Wishlist (non-blockers)

1. Word highlight drawing might be too obstructive. Especially in filter mode.
2. • Remember separate search-mask SearchInfo for each tab. (10c8f58)
3. • Buttons for select next/previous entry (align with script/docs viewport). (ec6c4c3)
4. Remember and restore tree collapse on jumping

This pr includes a significant amount of new code (~500 lines). I would greatly appreciate any feedback, especially regarding the known issues listed above.

demo_filter_tree.webm

[limbonaut]

Thanks for submitting! Since it's a big chunk of code, I need some time to review it fully.

Got a crash during testing: with an active search, I switched to another tab and pressed enter. It's one of those cached TreeItem pointers, judging by the backtrace.

Thinking about it, search state should probably be remembered per tab. I only skimmed through your code a little bit, and it doesn't look like it's the case, but would require changes to achieve that. Maybe TreeSearch instances could be kept per tab? But those TreeItem pointers are a problem then - they must be invalidated as soon as the user switches to another tab (or Tree is rebuilt).

[monxa]

Thanks for submitting! Since it's a big chunk of code, I need some time to review it fully.

Got a crash during testing: with an active search, I switched to another tab and pressed enter. It's one of those cached TreeItem pointers, judging by the backtrace.

Thinking about it, search state should probably be remembered per tab. I only skimmed through your code a little bit, and it doesn't look like it's the case, but would require changes to achieve that. Maybe TreeSearch instances could be kept per tab? But those TreeItem pointers are a problem then - they must be invalidated as soon as the user switches to another tab (or Tree is rebuilt).

Great catch!

[monxa]

Yes this is a good chunk of code. And I expect much of it not to align with what you want. Please take your time for review(s).

Will later find some time to address invalidation and add some FAIL-conds.

You are thinking into an interesting direction with your proposal of caching multiple tabs.
I think that would need the TreeSearch to have access to TaskTree. I tried to avoid that in favor of molecularity and to avoid circular includes. If you need this to be changed into such an direction tho: I am very open to make the necessary changes.

Edit: Now that I think about it, it'd not be necessary to include the TaskTree. We could pass the necessary information down into the TreeSearch. E.G. the current tree reference.

For now tho, I will make this work without error before such major changes.

[monxa]

Got a crash during testing: with an active search, I switched to another tab and pressed enter. It's one of those cached TreeItem pointers, judging by the backtrace.

47706e9
Fixed a rather prevalent issue within TreeSearch::_select_item that led to the nullptr exception.

9a1641e
Moved the TreeSearch::update_tree call from TaskTree::_update_tree to TaskTree::_create_tree. This should catch all cases where the task tree gets updated, also those where it gets updated from the Editor Plugin directly. AFAIK, the caching approach is infeasible. We don't cache trees at all after all. If you want to transition towards such a solution in the future, I apologize for any additional implementation considerations this solution causes.

[limbonaut]

Beware, nitpicks incoming 🤣

[limbonaut]

I've tested it, and it works really well - didn't find any other issues. If you are concerned about the selection box being confusing, we can try a custom StyleBox: a filled square built from the font color with the alpha set somewhere in [0.1,0.9] range - should work with any theme, I believe.

Some additional suggestions:

• Show "Ctrl+F" shortcut in the Misc menu (already mentioned in a comment)
• Remember search state per UI tab (borderline out-of-scope - we can make an issue to track it instead)
• Also, case sensitivity can be unintuitive: I mistyped "playAnimation" and was surprised that it didn't find anything (not sure if it's an issue, though).

[monxa]

As discussed. Squash incoming...

[ydeltastar]

It works really well even with drag and drop. The only major issue I noticed is that when I have a task named AnimationTreeTravel, "animationtreetravel" gets a match but "animationTreetravel" or other case variations don't.

[monxa]

It works really well even with drag and drop. The only major issue I noticed is that when I have a task named AnimationTreeTravel, "animationtreetravel" gets a match but "animationTreetravel" or other case variations don't.

Yes. The current behavior: Match case when input contains capital letter. Might need adjustment: Not intuitive. @limbonaut also pointed this out.

IMO, we can easily change that after this is merged.
We can do either:

• Always case-insensitive (Same as in docs)
• "Match Case"-Checkbox (Same as in script editor)

[limbonaut]

It works really well even with drag and drop. The only major issue I noticed is that when I have a task named AnimationTreeTravel, "animationtreetravel" gets a match but "animationTreetravel" or other case variations don't.

From reading the code, I think the idea is that if you supply a lower-case string, the search assumes case-insensitive, and if you mix-in CAPS - it assumes case-sensitive. We are trying to establish if it's a bug or a feature.

[ydeltastar]

Match case when input contains capital letter.

The search's placeholder text should probably include this as a hint.

[limbonaut]

Looking good! I've added just a few suggestions. In testing, it works like a charm.

We can consider if the case-sensitive issue needs changing, but no need to block this PR until that decision is made.

[monxa]

MacOS task appears to be broken.

Fixed on master. See:
godotengine/godot#97981

[monxa]

The search's placeholder text should probably include this as a hint.

Does a tooltip for the search box suffice for now?

line_edit_search->set_tooltip_text("Match case if input contains capital letter.");

[limbonaut]

MacOS task appears to be broken.

Fixed on master. See: godotengine/godot#97981

Hmm, bummer.

[limbonaut]

Looks like it's good to merge 👍
I just need to fix the Vulkan SDK in CI.

[limbonaut]

Great stuff! ❤️ Thanks for the contribution.