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

Add search keywords to the class reference #78990

Merged
merged 2 commits into from
Feb 12, 2024

Conversation

RedMser
Copy link
Contributor

@RedMser RedMser commented Jul 3, 2023

Fixes godotengine/godot-proposals#5085

Allows for finding methods, properties, signals, constants, theme items and annotations more easily.

  • Allow "keywords" attribute in aforementioned locations in the class reference XMLs.
  • Extends doctool, to preserve these attributes.
  • Update the XSD schema for the class reference.
  • Update the RST generator to include a meta tag for class keywords.
  • Update editor help search to allow filtering for these keywords.
  • Add some exemplary keywords attributes in the class reference, but more can be added in future PRs. They are merely included to not be forgotten in the proposal, and to be able to test the feature.

image

Ideas and Future

  • Should it be possible to add a @keywords ... comment in GDScript to define keywords for script classes and members? Implementation would look like GDScript: Add @deprecated and @experimental doc comment tags #78941
  • Idea for the future: add keyword search to GDScript autocompletion
  • Idea for the future: add keyword search to the "create new node/resource" dialog
  • Render the keywords list in a few places, such as
    • Next to a matching search result (so you know why this result is showing)
    • In the documentation (both RST and in the editor), to get a full list of related keywords

@RedMser RedMser requested review from a team as code owners July 3, 2023 16:21
@RedMser RedMser force-pushed the class-reference-keywords branch from 38cf495 to 4747fe9 Compare July 3, 2023 16:23
@Calinou Calinou added this to the 4.x milestone Jul 3, 2023
editor/editor_help_search.cpp Outdated Show resolved Hide resolved
@RedMser RedMser force-pushed the class-reference-keywords branch 2 times, most recently from a42398d to 643c2e2 Compare July 3, 2023 16:33
@YuriSizov
Copy link
Contributor

Update editor help search with a toggle button to allow filtering for these keywords.

What's the use case for not wanting to search by keywords?

@RedMser
Copy link
Contributor Author

RedMser commented Jul 3, 2023

What's the use case for not wanting to search by keywords?

Right now, the keywords do not show up anywhere in the UI, so I'm not sure if power users are gonna be happy that seemingly unrelated results suddenly show up in the search results.

If I had more time to work on this PR, I'd show the keyword that matched next to the search result, with greyed out text. Might take me a bit before I can get that implemented.

But I'm fine with taking the button out if there's no apparent need for it.

@YuriSizov
Copy link
Contributor

I'm not sure if power users are gonna be happy that seemingly unrelated results suddenly show up in the search results

Well the point of having keywords would be to make sure we show more related results than we do now. Hiding them behind a toggle makes them harder to access and less useful.

@RedMser RedMser force-pushed the class-reference-keywords branch from 92497d4 to 325c8a7 Compare July 5, 2023 12:02
@RedMser
Copy link
Contributor Author

RedMser commented Jul 5, 2023

Removed the toggle so keyword searching is always enabled now.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works. Code looks good to me.

There's a small usability issue however:

  • When you search for a node directly, it's automatically focused:

image

  • When you search for a keyword, the node that matches the keyword isn't automatically selected:

image

Getting this working is important to reduce the number of keystrokes you need to open the documentation page, so I suggest looking into this if possible.

@RedMser RedMser force-pushed the class-reference-keywords branch from 325c8a7 to aaf7ee9 Compare July 5, 2023 14:56
@RedMser
Copy link
Contributor Author

RedMser commented Jul 5, 2023

@Calinou Done! Updated _match_item and calling it for each keyword.

Though entering kill will make it prefer Node.queue_free over e.g. Tween.kill, not sure if I should leave it like that or reduce the weight of keyword matches.

@YuriSizov
Copy link
Contributor

Reducing the weight makes sense, as it's an indirect match.

@RedMser RedMser force-pushed the class-reference-keywords branch 3 times, most recently from 660771a to 5f97bb1 Compare July 8, 2023 15:41
@akien-mga
Copy link
Member

I'm not too fond of having keywords as XML tag attributes.

Attributes are usually things generated from the codebase, and human-written content should be filled within tags (e.g. <description>Human written</description>).

So it might make more sense IMO to add an optional <keywords> tag, like we do with <link> for the <tutorials> section.

@RedMser
Copy link
Contributor Author

RedMser commented Jul 10, 2023

So it might make more sense IMO to add an optional <keywords> tag

@akien-mga While I agree with your reasoning, it seems like it would have to be a breaking XSD schema change.
<member>, <constant> and <theme_item> are tags that only have a string content, so all of their descriptions would have to be moved into a <description> element.

Maintaining a backwards compatible parser sounds problematic (especially now that the version attribute is gone) and it would make new documentation features hard to discover.
Going through with the breaking change would (besides a large diff) break doctools of user-maintained modules, which might not be too bad?

Let me know what you think!

@akien-mga
Copy link
Member

akien-mga commented Jul 10, 2023

I don't understand, how would <keywords>3d, spatial</keywords> differ from the existing optional <link>https://example.com</link> tag?

Edit: Oh nevermind, re-read and checked the link, I see the problem. Hm...

I guess using an attribute is the least bad solution then.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 57919be), it works as expected with correct focus behavior now.

I can do a pass over the class reference to add keywords once this PR is merged.

@akien-mga akien-mga self-requested a review August 2, 2023 08:56
@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Aug 4, 2023
@Mickeon
Copy link
Contributor

Mickeon commented Jan 24, 2024

Would be nice to address the conflicts. Reception on this seems fairly positive. Although the current implementation seems out of reach for translators, if that was ever a concern.

@RedMser RedMser force-pushed the class-reference-keywords branch from 5f97bb1 to f0f60c6 Compare January 24, 2024 17:29
@RedMser
Copy link
Contributor Author

RedMser commented Jan 24, 2024

Rebased onto latest master. Thanks for letting me know! ^^

Although the current implementation seems out of reach for translators, if that was ever a concern.

Since the API is not translated (so method names, property names, etc.), I don't think it matters. You wouldn't search for a Spanish keyword when all the possible results are only ever in English.

@YuriSizov YuriSizov self-requested a review January 24, 2024 18:22
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Took a deep look. You can get a lot of mileage out of this feature in the future.

It is kind of confusing to get search results from terms that are not directly visible, although that can be figured out later.

@Calinou
Copy link
Member

Calinou commented Feb 5, 2024

Note for the future: Vector2/3's limit_length could have a truncate keyword as mentioned in godotengine/godot-proposals#9020.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Looks good otherwise

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
@akien-mga
Copy link
Member

akien-mga commented Feb 9, 2024

It is kind of confusing to get search results from terms that are not directly visible, although that can be figured out later.

Yeah that's my main concern with the current implementation. I'd like to see something that makes it obvious to the user why the filtered entry matches their search. There could e.g. be a "(keyword: ragdoll)" added after the node name in the list, in the OP's example.

It shouldn't necessarily be blocking for this PR though, it could be worked on in a follow-up, possibly by someone else if wanted.

Allows for finding methods, properties, signals, constants,
theme items and annotations more easily.

- Allow "keywords" attribute in aforementioned locations
  in the class reference XMLs
- Extends doctool, to preserve these attributes
- Update the XSD schema for the class reference
- Update the RST generator to include a meta tag for class keywords
- Update the editor help to support filtering by keywords
More should be added in future PRs, wherever there is demand.
@RedMser RedMser force-pushed the class-reference-keywords branch from f0f60c6 to 5911a12 Compare February 9, 2024 17:13
@akien-mga akien-mga merged commit 62fcc7e into godotengine:master Feb 12, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Add keywords for easier searching of methods/properties/… in the class reference
6 participants