Unify editor class and reference search

[RayKoopa]

Merges the class hierarchy tree and flat documentation reference search in one list.

The hierarchy can be toggled on or off. This setting is stored.

• When off, it is very similar to the previous flat documentation search.
• When on, it resembles the previous class list, but with added member search. Classes required to build the hierarchy but not actually matching any part of the search term when searching for members are grayed out.

Usability changes / improvements

• Case sensitivity can be toggled (setting is stored).
• The first full match is automatically selected.
• Removed now redundant buttons to open the separate class documentation.
• List can be filtered by item type. This setting is not stored to start new searches later on over all types.
• Tooltip on classes now strips unneccessary newlines at start and end.
• Results without an icon get the "ArrowRight" icon to make the text indented logically. Actual icons to differentiate between methods / constants / etc. would be interesting.
• Window size is stored and restored.
• Class names are matched like members, i.e. by subsequences of the search term. Previously, class names only containing the letters in any order of the search term were found.
• Drawing root lines can be toggled with the already existing scene dock settings.
• Tree no longer draws buggy lines when hierarchy lines should be rendered while the root item is hidden.

Code related changes

• Search is now only executed when the window is actually visible. It previously executed even at initial filling, for example when the editor was started.
• Deleted EditorHelpIndex. EditorHelpSearch (now in separate file) handles both.
• Removed open_class signal, now handled by go_to_help.
• Removed EditorNode signal request_help_index, now handled by request_help_search.
• Unified ScriptEditorBase signals request_help_index and request_help_search in request_help (tell me if you want to keep the name request_help_search instead).
• Rewrote so many parts of EditorHelpSearch::IncrementalSearch that I also renamed it to EditorHelpSearch::Runner as it isn't just an "incremental" search anymore (more a phased search).
• ClassDB::is_parent_class now uses get_parent_class_nocheck rather than get_parent_class internally since it didn't seem to care about empty strings, causing a lot of logged errors when it was used.

[djrm]

looks good, but it seems a bit confusing since in every other place the left panel (or tree) has some relation or dictates the content on the right side.

[RayKoopa]

Yeah, thought the same. Open for ideas. Mine so far are:

• Give them both the same width (and split exactly at the middle of the window). Problem: Reference items generally require more width, and when resizing the window, it wouldn't stay in the middle due to being a HSplitContainer.
• Unify both lists in one with "Classes" and "Reference" header items. Problem: Requires user to scroll through Classes first to reach the Reference
• Align lists vertically rather than horizontally. Problem: Requires quite a tall window.

[akien-mga]

Sorry, clang-format is quite strict on the alphabetical ordering of includes :P
https://travis-ci.org/godotengine/godot/jobs/314530991

[groud]

To be honest, I find this interface quite bad. As the gif suggests, you will very likely end-up with redundant results on the left and right side.

Maybe I don't understand perfectly how it should work, but instead of that, I would mix them into a single tree, where matching results are shown, whether it is a class, a method, or anything else. Something like that:

When a method is matching, it is shown as a child (in the tree) of its class. If only the class name matches, no method is displayed. (the class inheritance is still displayed)

[RayKoopa]

Yeah, I don't like it much either. I wanted to unify it into one list already and thought about the same. The reference list is already a Tree. I can try that if it finds acceptance.

• The problem is that you'll find a lot of class items in the tree about which you don't care (if you're searching for a method available in many classes, set_name for example...
• Thinking about it, if I display it only on the base class providing it, it should work...?).
• Another idea would be to have two tabs at the bottom - one listing by hierarchy (but with methods as you suggested), one flat like before...
• Yet another idea: Only display classes on the left, and only members on the right. Might give the impression you can further filter the members by selecting a class on the left though.
• Do we even need to display the hierarchy? If someone's interested in it, he can still doube click that class. If we get rid of it, we only have a 2-level deep results list: Classes -> Members.
• On another node (lol typo), I found this more readable for items in general. Maybe there should be specific icons for methods / properties / signals rather than the generic circle one which is already used for classes without icons (funnily only in the reference list, the class list uses no icon at all instead). A search filter could also exclude specific member types.
  

[groud]

In that case maybe could more option to filter the results ? Like checkboxes or something like that.

[RayKoopa]

I edited my previous comment, you mean something like that?
Could also have a checkbox "display in class hierarchy" and if not, it shows a flat list.

[groud]

Hum, this takes a lot of space. Maybe a dropdown list to filter the content instead then ?
And a button to switch between a tree view and a list view.

[RayKoopa]

Yeah, that's better. I try that.

[groud]

Nice, thanks :)

[letheed]

I opened an issue #14319 about better search in Search Help a few days ago.

I'd personally prefer filtering just by typing. Since it's a search box, your hands are already on the keyboard. It's fast to just type in "signal" or "method". Checkboxes and menus force me to grab the mouse again.

[RayKoopa]

You can tab and configure the filter like in most other applications. I could still add shortcuts. If you want to filter, you have to do more than typing (except someone wants to add keywords).

[letheed]

You mean use Tab to select the checkboxes and Space to toggle them?

[RayKoopa]

Something like that, or whatever you need to press to typically use the UI keyboard wise. There can still be hotkeys assigned to toggle each filter.

[groud]

Well IMHO, no need for obscure keyboard shortcuts for now. In most cases you'll find the result you are looking for by just typing its name.
The mouse will be needed quite rarely.

[letheed]

@RayKoopa I'd rather avoid a plethora of ad-hoc new keyboard shortcuts. As for using Tab, that'd almost unusable. Imagine having to disable all the boxes but the Signal one using Tab and space. At this point using the mouse would be less of a sore.

@groud Well when you're looking for a Signal, anything other than a signal showing up is just noise. Same if you're looking for a Class, or a Property. It's not that you won't find it. It's just that the signals are drowned in irrelevant methods and classes.

The problem with the design you're going for, is that you want all of them included by default, but when you don't you usually want just one. With this design, getting from "all included" to "just signals" means unticking every single other box aside from "signals". I think that's bad and tedious.

[groud]

Not if you have a drop-down list as what I suggested. You select either "display all", "signals only", "methods only" etc....

[letheed]

Yeah the dropdown solution is a bit better, but then you can't have two at a time.

[groud]

you can't have two at a time.

This is not important, either you know what you are looking for and you select it, or you don't and the "display all" option is ok.

[letheed]

Sometimes you don't know if something's going to be a method/getter or a property.

It's all right if you think that deserves to wait for another PR. But I still think that this is not a really great design. It's be much more convenient to be able to select types through search terms

[groud]

The proposal seems ok to me. Let's just keep the interface simple by now, if more people complain we will improve it. But for now it does not seems really necessary to me.

[RayKoopa]

Do we agree on how the results should look like? For example, the search for "set_name" with the hierarchy displayed would

• show all classes having "*set_name*" in their own name (unlikely for the example).
• show all classes being part of the hierarchy up to the child class having any "*set_name*" member
• show all the matching members (ohrly)
• not show other classes which have no matching members or don't include the search term in their class name.

If that's how you'd like it, I have some trouble implementing it wisely.

• I'd require "looking down" the class hierarchy to filter out upper-level nodes, e.g. from base classes into child classes. The current DocData only allows looking upwards via String ClassDoc::inherits.
• I'd have to build a tree storing base down to child classes first to logically wander through it. (The current class tree filter doesn't have to bother about this as it filters out only by class names themselves, not members.)
• The inheritance chain should be stored inside DocData itself however (as Vector<ClassDoc> ClassDoc::child_classes probably) rather than being (re-)created only for the help dialog, and I'm not sure if anyone wants me to add that to DocData.

[groud]

Do we agree on how the results should look like?

For me, the logic you describes looks ok.

If we can avoid recording a link to children classes it's better, avoiding redundancy in data structures is better, unless it's for performance reasons. I would just list the matching results then reconstruct the hierachical structure afterwards.

[RayKoopa]

Looking fine so far. I'll add remembering the selection when toggling the hierarchy (if possible) before I push.

Different icons for methods / properties / classes without their own icon would be interesting.

[groud]

It looks awesome! Maybe adding the function arguments could be good too?

[RayKoopa]

I guess they should still get the Object icon then?

[djrm]

some are not even godot Objects Like the ResourceLoader, the others im not sure

[RayKoopa]

According to DocData, they inherit from Object, which is also why they're listed in that indentation there (under Object).

[akien-mga]

Thanks a lot for your contribution!
We are now entering a strict release freeze for Godot 3.0 and will only consider major bug fixes. We won't merge new features and enhancements anymore until 3.0 is released.

Moving this PR to the 3.1 milestone, to be reviewed once the release freeze is lifted. It could eventually be cherry-picked for a future 3.0.1 maintenance release if it doesn't change the user-facing APIs and doesn't compromise the engine's stability.

[ghost]

i like this a lot, but i don't think an extra click should be needed (to get to help menu). can't we just click on search help as before, to bring this window up?

[RayKoopa]

yes?

[ghost]

thank you

[aaronfranke]

Is the behavior of this feature still the same after the discussions? If not, please update the GIF.

[RayKoopa]

Yes it is.

[RayKoopa]

How am I supposed to ever resolve these conflicts? Why hasn't this been merged before accepting additional (redundant?) work on this?

[aaronfranke]

If you are having trouble with Git, just backup your versions, accept all rebase conflicts in favor of Godot, then add your changes back.

[RayKoopa]

Yeah except adding them back causes conflicts over conflicts, and I don't know what's been changed over time which I should try to keep / put into my changes.

[akien-mga]

How am I supposed to ever resolve these conflicts? Why hasn't this been merged before accepting additional (redundant?) work on this?

I'm sorry about that, but you made this contribution at an inconvenient time as we were in release freeze for the massive 3.0 release. Since similarly big contributions had piled up on the master branch during December/January, and more kept coming, it took us months to review and merge them while staying on top of new changes, and some PRs from that time are still waiting, like this one.

Newer and potentially redundant work has been merged because it had a scope easier to review and manage. Big PRs are always harder to handle and can end up having conflicts to resolve before they're eventually merged or rejected.

I'm still interested in the UX improvements made in this PR, for the reference.

[RayKoopa]

Yeah I understand, and sorry I couldn't get back earlier to this before all the changes happened. I just found a little spare time recently, but I'm not sure it's enough to review all the changes by now. Would be happy if someone feels free to fix that branch up.

[fire]

https://github.com/fire/godot/tree/unified_help_search Is rebased on top of master, but it's using your search rather than the current search. I hope you find this useful.

It needs to be using the current search code with your changes to be complete.

EDITED:

The rebased version only works for class names, the other items in the list don't work.

[akien-mga]

We've discussed this on IRC today and it would be a "go" once rebased on the current master branch:

14:18 <Akien> Next one is #14535
14:18 <IssueBot> #14535: Unify editor class and reference search | https://git.io/vbRPq
14:18 <Akien> Also needs a rebase, and I guess we could ask iFire if he wants to do it since OP is busy - but we need a "go" for the proposed feature first
14:19 <reduz> are we ok with this? I personaly like it, but not sure if there is consense on this
14:19 <Akien> There's been a lot of iteration but I think the gif in the OP is the latest status
14:19 <Akien> I think it's pretty cool myself, and a net improvement over the current dialogs
14:20 <Akien> Usage will tell if it's good enough or needs some more usability changes, but it's a good start
14:22 <Akien> Given the reactions on the PR I guess the community likes it
14:22 <reduz> ok
14:23 <reduz> if iFire wants to give it a try, or OP finds time, would be great
14:24 <Groud[m]> I think it's good
14:25 <Groud[m]> the clean and "case sensitive" buttons are weird though

[RayKoopa]

To refer to what was said in the IRC, I can confirm that the GIF in the OP is up-to-date with the latest changes I commited back then.
And about the clean button: it would be nice to have a search box dynamically changing the magnifying glass to an X-clear button in case text is entered. I don't think i saw it anywhere though (back then).

[moiman100]

And about the clean button: it would be nice to have a search box dynamically changing the magnifying glass to an X-clear button in case text is entered.

There is a PR for it #20155

[akien-mga]

Closing as the PR has conflicts and no update so it can't be merged for the time being. It's a nice feature though and a new rebased PR would be welcome, either from OP or from another contributor extending the work in this PR (which is what the salvageable label means).

Thanks for the initial work, I hope it will make it in the master branch eventually.

[groud]

Too bad, this was a great improvement. Hope someone will be interested in rebasing it.