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

EditorHelpSearch improvements #62524

Merged
merged 1 commit into from
Aug 29, 2024
Merged

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jun 29, 2022

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

Production edit: closes godotengine/godot-roadmap#19

@AThousandShips AThousandShips changed the title Expand editor search Expand editor search results for empty search Jun 29, 2022
@AThousandShips
Copy link
Member Author

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

@AThousandShips
Copy link
Member Author

AThousandShips commented Jun 29, 2022

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

@AThousandShips AThousandShips force-pushed the help_search branch 2 times, most recently from 913bb23 to c777151 Compare June 30, 2022 05:48
@AThousandShips
Copy link
Member Author

AThousandShips commented Jun 30, 2022

This PR currently:

  1. Unifies search length, classes and class members are searched for with terms of length > 1
  2. Makes searches of length < 2 show all the requested entries
  3. Prevents searches from updating when searches go from 0->1 and 1->0 length (unless parameters change)
  4. Adds constructors back to search (still needs some tweaking) and adds operators (ditto)
  5. Prevents a crash when the dialog closes when searches are in progress (rarely if ever occurred before as searches were far faster)
  6. 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

editor/editor_help_search.cpp Outdated Show resolved Hide resolved
editor/editor_help_search.cpp Outdated Show resolved Hide resolved
editor/editor_help_search.cpp Outdated Show resolved Hide resolved
editor/editor_help_search.cpp Outdated Show resolved Hide resolved
editor/editor_help_search.cpp Outdated Show resolved Hide resolved
editor/editor_help_search.h Outdated Show resolved Hide resolved
@AThousandShips AThousandShips force-pushed the help_search branch 4 times, most recently from 9e333d1 to ccf9e6e Compare July 1, 2022 08:51
@AThousandShips
Copy link
Member Author

AThousandShips commented Jul 1, 2022

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 😖

@AThousandShips AThousandShips force-pushed the help_search branch 10 times, most recently from 836d0f6 to 4036832 Compare July 2, 2022 11:57
@AThousandShips
Copy link
Member Author

AThousandShips commented Jul 2, 2022

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

@AThousandShips AThousandShips force-pushed the help_search branch 2 times, most recently from 4ba15a9 to 619a8b9 Compare July 2, 2022 12:45
@AThousandShips
Copy link
Member Author

AThousandShips commented Aug 27, 2024

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

@AThousandShips AThousandShips force-pushed the help_search branch 2 times, most recently from 6531752 to 215454f Compare August 27, 2024 15:38
@AThousandShips
Copy link
Member Author

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

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested again and works as expected now.
Looks good, great work!

Things out of scope of this PR which came to my mind:

  • It would be nice to have unit tests for this
  • The icons look fine to me, although I think we should color (all of) them
  • Search should be async, so there is no noticeable input delay (any thoughts on this?)

@AThousandShips
Copy link
Member Author

AThousandShips commented Aug 27, 2024

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

@Mickeon
Copy link
Contributor

Mickeon commented Aug 27, 2024

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.

@akien-mga
Copy link
Member

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 (108c603).
It looks like it's not refreshing/drawing properly after I start typing.

The search with empty filter looks correct:
image

But after typing a search, nothing is shown:
image

Reopening the dialog makes it redraw and now results are visible:
image

@AThousandShips
Copy link
Member Author

AThousandShips commented Aug 28, 2024

Will investigate

Can confirm, recent regression no clue what might have caused it, will try digging

This happens on master as well so is a regression outside this

Regression from:

@AThousandShips AThousandShips marked this pull request as draft August 28, 2024 09:42
@AThousandShips AThousandShips marked this pull request as ready for review August 28, 2024 09:45
@AThousandShips AThousandShips requested a review from a team as a code owner August 28, 2024 10:07
@AThousandShips
Copy link
Member Author

Wrote a fix for the regression, including here temporarily to help with testing, see:

@Geometror
Copy link
Member

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

Yep, that is one thing that needs to improve in order to find editor regressions as soon as possible. We have the [Editor] tag for test cases which should allow testing this (if not it needs to be extended/fixed). The only thing we need is another small refactoring (maybe one or two interface methods) to make it testable. Writing some unit tests to find regressions would be easy then.

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

Alright, maybe it's not that bad in release builds, but at some point I think we should find a solution for the lag (although not here and now).

@akien-mga
Copy link
Member

I don't remember why the "Nest hierarchy" commit is separate, is that still up for discussion whether this should be included or not? Is it meant to stay separate for clarity, or be squashed eventually? If kept separate, the commit message should likely be more explicit (it should be "standalone" too).

@AThousandShips
Copy link
Member Author

Will squash it if everything is approved, once CI has finished and will clarify the original commit message as well, I think the nested hierarchy (adding headings for constructors, methods, etc.) is good to keep

@akien-mga
Copy link
Member

Tested briefly, it looks good overall, but I'm not sure I understand what triggers showing the "methods"/"signals"/etc. headers like this:
image

I get this by searching "c" (why is it narrowing down on "Curve" specifically btw?), but not if I search "curve".
image

@AThousandShips
Copy link
Member Author

The showing of the sub-headers is done if there's more than one entry of that kind, with a short search only classes are filtered at the present

It picks Curve specifically because of the scoring system which picks short results over long results with equal score otherwise (this is unchanged)

Filtering on members on short searches could be added but I think it's of limited use as searching for just c is not going to find much of value (at least compared to the cost of doing so), so for short searches it just filters classes that are found and shows all their members

Adding filtering on member items would take a little more work so if we decide to add that as well I'd prefer to do it separately

@AThousandShips
Copy link
Member Author

AThousandShips commented Aug 28, 2024

Just gonna push a usability improvement for navigation after I've tested it, not directly relevant at the moment because of a limitation in Tree but will implement that in a PR that I'll open soon (and it still helps with navigation generally)

* Adds all member types to empty search
* Nests hierarchy adding constructors, methods, operators, etc. under a
  nested entry to reduce clutter

Co-authored-by: MewPurPur <mew.pur.pur@gmail.com>
@akien-mga akien-mga merged commit c1605e4 into godotengine:master Aug 29, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the help_search branch August 29, 2024 08:43
@AThousandShips
Copy link
Member Author

Thank you! My first ever PR finally merged! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search Help: Empty results list when nothing is in the filter box
7 participants