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

Use bound theme properties for documentation #81573

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Sep 12, 2023

Based on #81551 (so drafting for now).

This PR makes use of the new theme item binding information for documentation generation. This means that the decision which properties to document no longer comes from what is defined with the default engine theme. Instead, just like with properties and methods, only what's registered by the class itself (and it's inherited classes) is added.

As a result, a couple of theme properties were removed since they aren't actually used by their respective classes (maybe they should, but for now they aren't, so removing them is a better option).

One open question is what to do with inherited theme properties. Historically, due to our reliance on the default theme, all inherited properties would be displayed in extending classes as if their own. This is handy, since you can always see every property that can affect the GUI node, but that's not how we do the docs for class members, signals, and methods. We can, now, filter these out and remove them from extending classes or add them as stubs without descriptions like we do for "overridden" class members.

For now I'm going with adding them everywhere. This is most consistent with our current documentation, for better or worse. We can reconsider this in a future PR, I think.

PS. Similar PRs will be made for theme overrides, code completion, and the theme editor. Going one by one to slowly figure out if anything is missing from the API (like this PR added data types to the bind information).

@YuriSizov YuriSizov added this to the 4.2 milestone Sep 12, 2023
@YuriSizov YuriSizov force-pushed the docs-use-theme-binds-in-help branch 2 times, most recently from c6ee269 to 1121165 Compare September 14, 2023 14:03
@YuriSizov YuriSizov marked this pull request as ready for review September 14, 2023 14:24
@YuriSizov YuriSizov requested review from a team as code owners September 14, 2023 14:24
@KoBeWi
Copy link
Member

KoBeWi commented Sep 14, 2023

I think displaying every inherited theme property is counter-productive. The documentation ends up with tens of new useless (and empty!) items. I'm not even sure if all of them are functional (some are though, I checked). I think it's better if they did not appear at all, like regular member, and in rare cases when the item's usage differ, it can be noted in the class description (I can only think of ColorPickerButton, which hides most of stuff).

Also entries that got removed should be removed from default theme too I think.

@YuriSizov
Copy link
Contributor Author

Also entries that got removed should be removed from default theme too I think.

Did I miss any? I removed them from both the default theme and the editor theme.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 14, 2023

No, I missed your theme changes 🤦‍♂️

@akien-mga
Copy link
Member

One open question is what to do with inherited theme properties. Historically, due to our reliance on the default theme, all inherited properties would be displayed in extending classes as if their own. This is handy, since you can always see every property that can affect the GUI node, but that's not how we do the docs for class members, signals, and methods. We can, now, filter these out and remove them from extending classes or add them as stubs without descriptions like we do for "overridden" class members.

I think being consistent with regular properties, etc. would be good. The big problem with duplicating them everywhere is that we need to duplicate their documentation everywhere too. So I'd prefer they're documented only in the base class (and overrides are shown like we do for properties).

If we still want a way to easily give an overview over all the theme properties a given class has access to, that's an orthogonal UX problem IMO, we can figure out solutions that don't require duplicating the XML and documentation data. (E.g. a folded-by-default list of all theme properties, below the list of theme properties added in this class and their documentation.)

@YuriSizov YuriSizov force-pushed the docs-use-theme-binds-in-help branch 2 times, most recently from 1a07413 to bf188d6 Compare September 26, 2023 13:38
@YuriSizov
Copy link
Contributor Author

Alright, I removed inherited properties from documentation and unified some descriptions for H/V nodes. Inspector still works correctly and shows descriptions and redirects to correct pages. So I think we're good to go.

@YuriSizov YuriSizov force-pushed the docs-use-theme-binds-in-help branch from bf188d6 to ae6a7d5 Compare September 26, 2023 14:41
@akien-mga
Copy link
Member

You need to copy back the descriptions to the base classes where the theme properties are now being exposed. We seem to be losing quite a bit of documentation otherwise.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 26, 2023

The "new" empty items are properties that weren't documented before and they never had description. The descriptions in inherited classes were copy-paste from the base class (with some changed references).

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Sep 26, 2023

Yes, I copied stuff that was only defined in extending classes or had differences not reflected in the parent class (like H/V variants). Maybe I missed some? But most of the descriptions that are now gone are duplicates of the base class documentation, as @KoBeWi says. Like in the case of a dozen Button types.

Edit: The only thing that we may be losing is default values which override inherited values of some properties. I'm not sure how useful this is for theme items, it's not really something that you reference. But if we want it, it will require the whole "overrides" logic that we use for regular class members.

scene/theme/theme_db.cpp Outdated Show resolved Hide resolved
@YuriSizov YuriSizov force-pushed the docs-use-theme-binds-in-help branch from ae6a7d5 to 03a2472 Compare September 26, 2023 15:55
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks fine, but I left some comments for theme item descriptions that you could copy from the deleted ones.

@YuriSizov YuriSizov force-pushed the docs-use-theme-binds-in-help branch from 03a2472 to 596dd72 Compare September 26, 2023 16:49
@YuriSizov
Copy link
Contributor Author

Added missed descriptions, and modified ThemeDB::get_class_own_items so inherited properties which are cached by extending classes are also properly ignored. This removed a bunch more theme items. I added missing descriptions resulting from that too.

@YuriSizov YuriSizov requested a review from KoBeWi September 26, 2023 16:50
@akien-mga akien-mga merged commit f14ed30 into godotengine:master Sep 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the docs-use-theme-binds-in-help branch October 3, 2023 13:14
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.

3 participants