godot `EditorHelpSearch` improvements

Makes editor help search show constructors/methods/signals/etc. when search is empty

This also adds operators and constructors as they were lost after #53452, though only builtins have those so it's relatively minor.

~Depends on #77554 and #77471, which it includes, please add notes for those (the first two commits) there~

Fixes #62508

Production edit: closes godotengine/godot-roadmap#19

Jun 29 '22 15:06 AThousandShips

I believe the issue is solved but gonna have to make icons for constructors and operators unless those should just be kept as method

Jun 29 '22 16:06 AThousandShips

For consideration, currently there's an inconsistency in how search term length is handled:

Classes: Starts searching when a single character is entered

Members: Starts searching when two or more characters are entered

I think this should be unified into one of the two, and I'm personally in favor of two characters as searching for just one is kind of pointless, but I'd like some input on this

I'm currently going for the two characters option and looking at limiting search update when going from 0->1 or 1->0 characters where no change would occur

Jun 29 '22 18:06 AThousandShips

This PR currently:

Unifies search length, classes and class members are searched for with terms of length > 1
Makes searches of length < 2 show all the requested entries
Prevents searches from updating when searches go from 0->1 and 1->0 length (unless parameters change)
Adds constructors back to search (still needs some tweaking) and adds operators (ditto)
Prevents a crash when the dialog closes when searches are in progress (rarely if ever occurred before as searches were far faster)
Tweaks time in each search iteration to prevent perceptible slowdown (still needs some tweaking)

I would appreciate some input on the following:

Should constructors and operators even show up (they are only available for variants, and are currently not included without this PR)
- If yes, should they have unique icons or use the MemberMethod icon, I can try my hand at designing new ones but if someone would like to help with that I'd appreciate it and I'll add as co-author
- And should constructors/operators list their parameters in the search (currently they are shown as tooltips) and show each or should they be unified with a single entry for each unique constructor/operator
Should searches be unified to length one or two, I am going for length two (as I can try and solve some problems like point 3 and won't be a problem if that ends up not used, and as I think it's the most reasonable)
Should ongoing searches show progress

I'm currently working on preventing search from updating when no change would occur, and ensuring nothing weird happens when the popup opens again, and tweaking the iteration to make it feel smooth

Any and all input is very welcome, this is my first PR and I just want to say that I've been following Godot and making things with it for some time now and I've rarely been in a more welcoming and productive environment, much love!

Edit: Much of this is now outdated see below

Jun 30 '22 07:06 AThousandShips

I implemented a basic index and filling from this index, it's very much a work in progress and code needs some cleaning up, some that belongs in the header is kept out of it to avoid recompiling lots of unrelated files etc. but I wanted to show what my current approach was. It probably needs some further tweaking to be smoother but it works far better I feel.

I will probably work more later today and clean it up and restructure a bunch of the setup, probably try move the filling into the runner to unify the code, but threw something together to see how to make it work.

Edit: Restructured the filling and made the tentative decision to collapse classes with no child classes, this greatly improves speed and reduces clutter in my opinion, it needs a bunch of tweaking and restructuring still but if this is a feature we want (I for one do) then I think it is now manageable in its implementation.

For comparison between collapsed and uncollapsed comment out: https://github.com/godotengine/godot/blob/9a6fe8eac6fe2b130beaa7cb4a338c1b94cbc49b/editor/editor_help_search.cpp#L486

It turns out that shaping a tree of about 17700 items is a bit of a slog 😖

Jul 01 '22 08:07 AThousandShips

Filling of the tree from the index is now moved to the runner, I've still got some details to work out and some style aspects to decide on, and add code for the filling without hierarchy which takes a little reworking from my current temporary solution, but it appears to work smoothly for me now.

Update on the changes of this PR:

Short searches now show the requested types, using an index built on first use.
The icons for member items are pre-loaded as they (especially in filling from the index) are loaded many times.
Closing the popup while searching/filling is under way does not cause a crash (would rarely if ever occur before as ordinary searches were very fast)
Constructors and operators are restored to the search/fill
The tree can now be collapsed/uncollapsed recursively like the node dock tree does (holding shift while clicking the fold arrow)

Things still to do:

Icons for constructors and operators (if anyone would be up for making such I'd be very grateful and give co-authorship)
Filling without hierarchy, minor change for that but didn't focus on it at the moment.

For consideration:

Should constructors and operators be shown, constructors were lost after constructors and operators were split out from methods
- If yes, should they be shown uniquely, i.e. "Type (Constructors)" as they were before, and same for multiple copies of the same operator

Jul 02 '22 12:07 AThousandShips

Testing out filling with no hierarchy makes me lean toward not showing it like that, it gets extremely cluttered and without adding a second index or reverting to using the documentation it won't be sorted alphabetically, so I made the decision to always draw the hierarchy when showing the index.

Jul 02 '22 13:07 AThousandShips

I've added some tentative icons for constructors and operators

Jul 03 '22 07:07 AThousandShips

I incorrectly thought that the help search didn't show custom classes at all, but as we are already experiencing a bug with custom classes being added breaking the help search I'm not sure what's going on with that, that would require the index to be rebuilt but it would require some signal on documentation being added, I'm not sure how that would be approached

Because of #61970 I can't really test this aspect

Edit: It appears this doesn't seem to cause any problems though new classes don't show up they will still be searchable for, I will continue to see how this works

Jul 03 '22 17:07 AThousandShips

Rebased and restructured this PR, still going to look over some details but I believe it is about ready, the following new changes have been done:

Constructors and operators now list their argument types, as they are the only ones to be overloadable and I find it more helpful than just having to get these details from the tooltip
Removed the update spinner as it was only to indicate progress during development

Oct 27 '22 14:10 AThousandShips

I believe things are ready, any feedback would be much appreciated but I believe things are working as they should, there is the problem of new scripts not showing up in search but that affects the search already and try as I might I haven't been able to identify where the problem is.

More specific decisions that I'd like some input on are:

The choice to show all constructors, or just one "constructors", and if yes should they be shown as they are now with arguments
Same for operators
Opinions on the icons

Oct 30 '22 14:10 AThousandShips

Can this get some feedback on my questions and approach?

May 24 '23 16:05 AThousandShips

So I was looking into some improvements to this dialog without realizing that you've already submitted this PR with a ton of them 🙃

This PR does a different thing than what I want to do, but I also arrived at a need for some sort of optimization to the entire runner logic. For one I want to optimize TreeItem creation (see https://github.com/godotengine/godot/pull/77446), but I also was looking into sorting improvements.

All of that, and your changes here, require some sort of hierarchy for sure, because even sorting must be done in a hierarchy-aware way. You introduced an index, and I'm trying to make sense of it, but there is quite a lot going on here. Perhaps it would be worth extracting it into a separate PR, and leave this one for the rest of the improvements? I can promise to review it as soon as it's ready (and this one too).

May 24 '23 17:05 YuriSizov

I will look into it! The index yes?

May 24 '23 17:05 AThousandShips

Yes, I think we should extract the index into its own PR, and rebase this one on top of that.

May 24 '23 17:05 YuriSizov

Ironing out some details but been able to separate the index part, might be able to push tomorrow

May 24 '23 21:05 AThousandShips

Update: The index now respects updates to class index (see #77471)

Changes from before:

Case sensitive and show hierarchy buttons are now not deactivated on short searches, to be able to set them before entering the search, might be confusing that they have no effect though

May 25 '23 12:05 AThousandShips

When searching for all categories I now nest these member items under a common category, I feel this reduces clutter (and speeds up the process), added as a separate commit if it's not considered helpful as a feature

May 25 '23 14:05 AThousandShips

Restructured and rebased, now includes #77554 and #77471

Included icons by MewPurPur

Currently contains a commit with nesting items in their own categories if searching for short string and all categories are included, to reduce clutter

Should be ready for reviews now, see the previous conversations for more details on some of the decisions, though some is outdated as the code is reworked

Dec 07 '23 15:12 AThousandShips

Here are my performance tests based on https://github.com/godotengine/godot/pull/77471#issuecomment-1893923370

No caching, clearing tree instead of storing all the items

# Open, empty search
EditorHelp search done in 349790 usec

# Type in `CanvasItem`
EditorHelp search done in 131627 usec
EditorHelp search done in 42886 usec
EditorHelp search done in 28333 usec
EditorHelp search done in 26360 usec
EditorHelp search done in 26840 usec
EditorHelp search done in 24004 usec
EditorHelp search done in 22215 usec
EditorHelp search done in 21156 usec
EditorHelp search done in 20433 usec

# Clear search
EditorHelp search done in 284755 usec

# Type in `CanvasItem` again
EditorHelp search done in 132012 usec
EditorHelp search done in 40711 usec
EditorHelp search done in 26778 usec
EditorHelp search done in 25513 usec
EditorHelp search done in 23497 usec
EditorHelp search done in 21685 usec
EditorHelp search done in 20848 usec
EditorHelp search done in 20610 usec

# Clear search again
EditorHelp search done in 286064 usec

No caching, but not clearing tree (this builds up used objects, but don't think it necessarily affects this part of the performance)

# Open, empty search
EditorHelp search done in 319704 usec

# Type in `CanvasItem`
EditorHelp search done in 60183 usec
EditorHelp search done in 30179 usec
EditorHelp search done in 26794 usec
EditorHelp search done in 26078 usec
EditorHelp search done in 25397 usec
EditorHelp search done in 23168 usec
EditorHelp search done in 22543 usec
EditorHelp search done in 21628 usec
EditorHelp search done in 21260 usec

# Clear search
EditorHelp search done in 289458 usec

# Type in `CanvasItem` again
EditorHelp search done in 62680 usec
EditorHelp search done in 29478 usec
EditorHelp search done in 26036 usec
EditorHelp search done in 25211 usec
EditorHelp search done in 22878 usec
EditorHelp search done in 22875 usec
EditorHelp search done in 21384 usec
EditorHelp search done in 21342 usec

# Clear search again
EditorHelp search done in 284969 usec

With caching

# Open, empty search
EditorHelp search done in 320378 usec

# Type in `CanvasItem`
EditorHelp search done in 50075 usec
EditorHelp search done in 29114 usec
EditorHelp search done in 26448 usec
EditorHelp search done in 25903 usec
EditorHelp search done in 25190 usec
EditorHelp search done in 23436 usec
EditorHelp search done in 21987 usec
EditorHelp search done in 21060 usec
EditorHelp search done in 21046 usec

# Clear search
EditorHelp search done in 97474 usec

# Type in `CanvasItem` again
EditorHelp search done in 49374 usec
EditorHelp search done in 28493 usec
EditorHelp search done in 26446 usec
EditorHelp search done in 25367 usec
EditorHelp search done in 24538 usec
EditorHelp search done in 22766 usec
EditorHelp search done in 22145 usec
EditorHelp search done in 21414 usec
EditorHelp search done in 20597 usec

# Clear search again
EditorHelp search done in 96840 usec

Jan 16 '24 16:01 AThousandShips

Could you please rebase on the current master? We had an issue with macOS builds which failed your CI.

Jan 17 '24 18:01 YuriSizov

Will do was waiting for it to get merged, thank you for notifying me 🙂

Jan 17 '24 18:01 AThousandShips

That's not new, it was already in the code, I just moved where it's checked, so that's a separate matter IMO

Aug 27 '24 13:08 AThousandShips

Well, you united the search term length thresholds so that classes are searched only after >=2 characters. (1 prior)

Aug 27 '24 14:08 Geometror

No it searches for classes when terms are short, check the code for the else block, unless the code is broken since last I tried

It doesn't match correctly with short terms indeed, will fix that

Aug 27 '24 14:08 AThousandShips

There, now matches fully on short searches and checks keywords on short searches as well, but only checks classes and does not filter members on short searches

Aug 27 '24 15:08 AThousandShips

Thank you!

Don't see how we could add unit tests for this, what would we test? It's internal and we don't have unit tests for any editor features at all

The search is async but it fills in the list online, so not sure how that could be solved except with having a timer delaying starting searching until after a pause in inputting

Aug 27 '24 17:08 AThousandShips

The search is async but it fills in the list online, so not sure how that could be solved except with having a timer delaying starting searching until after a pause in inputting

I believe most of the times it's handled this way, yes, although I've not seen this kind of thing done in the editor as far as I know. Except, say, the script editor's autocompletion.

Aug 27 '24 19:08 Mickeon

It doesn't seem to work properly for me on Linux (Fedora 40 on Wayland, running X11 editor). Testing latest commit from this PR rebased on master (108c603f91b94100a1adc989316a372f0a6f8989). It looks like it's not refreshing/drawing properly after I start typing.

The search with empty filter looks correct: