-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 theme_custom_type
property to Control
and Window
#47544
Add theme_custom_type
property to Control
and Window
#47544
Conversation
5182dbd
to
6875bab
Compare
theme_type_override
property to Control
and Window
theme_custom_type
property to Control
and Window
Property renamed after Groud's suggestion. |
6875bab
to
5bf095a
Compare
Apparently those issues with theme propagation I've described are documented here: So this hopefully improves the overall situation and we can remove that note next 🙂 |
5bf095a
to
88d5379
Compare
88d5379
to
9eaa139
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks ok, some refactoring is always welcome.
As for the custom type feature, it looks handy, but personally I don't see much use for it. Well, maybe it helps to avoid some overrides. Also it's rather small, so it could be ok if some people requested it.
@KoBeWi When designing a UI that tries to give hints to the user on the where to go, differently themed elements are a must. And depending on the quantity, it can clutter your scripts quite a lot. This PR could make things far more easier from the get-go. |
Thanks! |
Backported from godotengine#47544 where it was originally identified
Part of godotengine/godot-proposals#2505.
Edit: Property was renamed from
theme_type_override
totheme_custom_type
after Groud's suggestion. Screenshots and clips refer to it by the old name, so be aware.This introduces a new
Control
propertytheme_custom_type
which allows users to define a higher priority theme type to be used whenControl
s fetch their own theme items (e.g. by callingget_theme_color("font_color")
without a type specified). By default the class name of theControl
is used, and then the class names of all of the parent classes. Iftheme_custom_type
is defined, it is considered before the class names. As before, local theme item overrides are considered first.This system allows to add subclasses for
Control
s without creating scripts or manually overriding each property, which means all definitions can be stored inside of a singleTheme
resource and, for example, can be swapped for an alternativeTheme
with minimal effort. Overriding types can be partial, and in that case they fallback on the class name definitions.In the example below
AcceptButton
andCancelButton
havetheme_custom_type
property set to a value of the same name, whileRegularButton
has it empty.Theme
that is applied to the entire branch defines normal and hover states forButton
and overrides normal states forAcceptButton
andCancelButton
types. Title label also has its own custom type.2021-04-01_14-36-07.mp4
I've tried to make this PR backwards compatible, however current code has at least 2 copy'n'paste errors as well as a big inconsistency to how theming is applied. I had to address those, so the way theming works is a bit different now. It is more correct and consistent, but it may lead to things looking differently if particular features were used.
Bug 1:
Control
's internalhas_icons
method checked wrong theme items for the Project-wide Theme.Bug 2:
Control
'shas_theme_constant
method didn't correctly fall back on class name when no type was specified.These issues were addressed in the process of making this PR. To ensure that it is less likely to happen in future I've refactored parts of
Control
to use the newTheme
methods that rely onDataType
enum and reduced the number of replicated code snippets between similar methods.The inconsistency in the current theming process is related to which types are used when looking for theme item definitions. The process currently is as follows:
Theme
, and if it has, whether there is an item we seek with that type name.Button
we first check if the item exists in the typeButton
, thenBaseButton
, thenControl
).Control
s has ended we check the Project default Theme and the engine default Theme.The problem with step 4 is that in those default themes only the base type name is checked (e.g.
Button
), but parent class' names are not checked. This means that applying your customTheme
to the topmostControl
node is not the same as applying it in the Project Settings. This also means that the defaultTheme
works differently than user createdTheme
s. I've tried to work around it, but ultimately to allow custom types which was the goal of this PR I has to address the issue and fix the behavior.I think this was an incorrect behavior and that the fix should be backported to
3.x
, but it is definitely not strictly compatible. So there is that.Note, that having an custom type like this is a desirable feature and was requested by users in the feedback to godotengine/godot-proposals#2505. This PR is required for that proposal to be implemented fully, but if rejected for whatever reason, some of the related functionality can be omitted. I've decided to put this forward ahead of time to properly evaluate that particular enhancement, as it touches core of the theming system and is not directly related to the Theme editor itself.
Oh, and this also renames
node_type
arguments totheme_type
, as they aren't strictly related to nodes, they just default to class names a lot of time. And custom types can be absolutely abstract, as seen in the editor itself withEditor
,EditorIcons
, etc.