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

Docs: Update Control class to properly reflect behavior of Themes on Control Children #99620

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

tetrapod00
Copy link
Contributor

Supersedes #90289. Apologies if this kind of salvage is frowned upon in the main repo; I've been doing this to clear the backlog in the docs repo recently.

Closes #88534.

The wording "as long as a chain of [Control]s is uninterrupted" is copied from the Theme docs.

I made some additional changes:

  • Don't call theme items "parameters" - I believe either "items" or "properties" is correct here.
  • Don't mention that you can override the theme with the inspector, since this is implicit from the fact that this is a class with a theme property.
  • Link to the [member theme].

@tetrapod00 tetrapod00 added this to the 4.x milestone Nov 24, 2024
@tetrapod00 tetrapod00 requested a review from a team as a code owner November 24, 2024 09:14
@@ -12,7 +12,7 @@
Call [method accept_event] so no other node receives the event. Once you accept an input, it becomes handled so [method Node._unhandled_input] will not process it.
Only one [Control] node can be in focus. Only the node in focus will receive events. To get the focus, call [method grab_focus]. [Control] nodes lose focus when another node grabs it, or if you hide the node in focus.
Sets [member mouse_filter] to [constant MOUSE_FILTER_IGNORE] to tell a [Control] node to ignore mouse or touch events. You'll need it if you place an icon on top of a button.
[Theme] resources change the Control's appearance. If you change the [Theme] on a [Control] node, it affects all of its children. To override some of the theme's parameters, call one of the [code]add_theme_*_override[/code] methods, like [method add_theme_font_override]. You can override the theme with the Inspector.
[Theme] resources change the [Control]'s appearance. The [member theme] of a [Control] node affects all of its direct and indirect children (as long as a chain of [Control]s is uninterrupted). To override some of the theme items, call one of the [code]add_theme_*_override[/code] methods, like [method add_theme_font_override].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
[Theme] resources change the [Control]'s appearance. The [member theme] of a [Control] node affects all of its direct and indirect children (as long as a chain of [Control]s is uninterrupted). To override some of the theme items, call one of the [code]add_theme_*_override[/code] methods, like [method add_theme_font_override].
[Theme] resources change the control's appearance. The [member theme] of a [Control] node affects all of its direct and indirect children (as long as a chain of controls is uninterrupted). To override some of the theme items, call one of the [code]add_theme_*_override[/code] methods, like [method add_theme_font_override].

I think this is a little better personally, but since the salvaged PR already had an applied suggestion to use "[Control]" instead of "control" I left it in (and added brackets to the untagged capitalized "Control").

Copy link
Contributor

Choose a reason for hiding this comment

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

Doing this has always been better in my opinion, yes.

Copy link
Contributor Author

@tetrapod00 tetrapod00 Nov 24, 2024

Choose a reason for hiding this comment

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

Applied this change, but willing to revert if it's contentious.

We should probably formalize some actual guidelines for when to use "control" vs "[Control]" at some point, otherwise ambiguous situations like this are likely to be changed back and forth repeatedly.

@Mickeon
Copy link
Contributor

Mickeon commented Nov 24, 2024

The documentation is sometimes torn on what to call these. Sometimes it's "theme items" and sometimes it's "theme properties". I think the former is more common.

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.

This is an improvement over the existing PR. When closing as superseded, let's remember to mention that the original PR has technically been accepted and credited.

@@ -12,7 +12,7 @@
Call [method accept_event] so no other node receives the event. Once you accept an input, it becomes handled so [method Node._unhandled_input] will not process it.
Only one [Control] node can be in focus. Only the node in focus will receive events. To get the focus, call [method grab_focus]. [Control] nodes lose focus when another node grabs it, or if you hide the node in focus.
Sets [member mouse_filter] to [constant MOUSE_FILTER_IGNORE] to tell a [Control] node to ignore mouse or touch events. You'll need it if you place an icon on top of a button.
[Theme] resources change the Control's appearance. If you change the [Theme] on a [Control] node, it affects all of its children. To override some of the theme's parameters, call one of the [code]add_theme_*_override[/code] methods, like [method add_theme_font_override]. You can override the theme with the Inspector.
[Theme] resources change the [Control]'s appearance. The [member theme] of a [Control] node affects all of its direct and indirect children (as long as a chain of [Control]s is uninterrupted). To override some of the theme items, call one of the [code]add_theme_*_override[/code] methods, like [method add_theme_font_override].
Copy link
Contributor

@Mickeon Mickeon Nov 24, 2024

Choose a reason for hiding this comment

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

I think it's worth lightly mentioning they can be overridden in the Inspector dock, as per the original description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I restore the original phrasing " You can override the theme with the Inspector.", then?
Or were you thinking incorporate it into another of the sentences

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like "It's also possible to override theme items in the Inspector." or "You can override theme items in the Inspector". The prior sentence is exactly about this, and it's very much for granted that theme can be changed in the Inspector like most properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with the second suggestion.

…Control Children

Document fact that themes only propagate to control children
(not Node2D, etc). Wording is copied from Theme docs.
Also clarifies line somewhat.

Co-Authored-By: Allyson Chan <[email protected]>
@Mickeon Mickeon modified the milestones: 4.x, 4.4 Nov 24, 2024
@Repiteo Repiteo merged commit ec01ad6 into godotengine:master Nov 25, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 25, 2024

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.

Further document that theme does not propagate through Node2D nodes
3 participants