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 getters and setters to control nodes' theme items #2000

Open
chucklepie opened this issue Dec 18, 2020 · 12 comments
Open

Add getters and setters to control nodes' theme items #2000

chucklepie opened this issue Dec 18, 2020 · 12 comments

Comments

@chucklepie
Copy link

chucklepie commented Dec 18, 2020

Describe the project you are working on

Platform game

Describe the problem or limitation you are having in your project

I had to search on the internet just to simply figure out how to change a font's colour and other settings because the popup help nor documentation made it easy to figure out (that there was no getter/setter and in the Label in my case, there is literally nothing in the documentation for any of the 'custom' values), but that's not the point really.

set("custom_colors/font_color",Color(1,0,0))

Having to call a set (or get) method with a string is not really an ideal method, it's a bit like taking every single getter/setter from every node and saying, sorry please use hard coded strings and triple check you didn't use the wrong slash or use the American and not British spelling :)

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Make Control node custom entries first class citizens somehow please, at least for the most commonly used controls and properties...

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

$Label.font_colour=XXX
$Label.custom_colours.font_colour=XXX

If this enhancement will not be used often, can it be worked around with a few lines of script?

Only if you know how and remember not to make any spelling mistakes

Is there a reason why this should be core and not an add-on in the asset library?

Setting things like label colours, etc should be simple and intuitive

@Calinou Calinou changed the title Add getters and setters to control node 'custom' items Add getters and setters to control nodes' theme items Dec 18, 2020
@Calinou
Copy link
Member

Calinou commented Dec 18, 2020

For the record, "Custom" items are called theme items in Godot's GUI system (see the class reference).

@chucklepie
Copy link
Author

@Calinou . Thanks, I did not know that. I know themes are there but that section is called 'Theme' in the inspector. The 'Custom Styles', 'Custom Fonts', 'Custom Colours', 'Custom Constants' are separate to 'Theme' so I presumed them separate.

btw, you do an awesome job maintaining the issues system :)

@KoBeWi
Copy link
Member

KoBeWi commented Dec 18, 2020

You can do add_color_override(), it should autocomplete font_color for you.
image

@chucklepie
Copy link
Author

Ok, that's good to know, thanks. But there are so many other useful properties hidden by this theming/custom system that require hard-coded string changes, and even this method you provided is still hidden behind a string and an obscure sounding method, making it difficult to document and for people to actually code with.

What you've supplied is now a second way of accessing the same property, which kind of makes it sound a bit wrong (as in why are there two different ways of doing the same thing), and there still is no direct way of setting the values.

I'm guessing there is a reason for this lack of accessor methods?

@KoBeWi
Copy link
Member

KoBeWi commented Dec 18, 2020

I'm guessing there is a reason for this lack of accessor methods?

The reason is that they are not real properties. add_override methods are the intended way to interact with them, but they are also kind of hacked to be serializable (i.e. saved in the scene) and editable in the inspector. Similar thing is with e.g. ShaderMaterial and set_material_param() - you can also set/get the parameters manually.

@chucklepie
Copy link
Author

Ok, I guess they're not going to change anytime soon to make them more usable or even documented.

Seems like there isn't a massive difference between setting these 'themed' properties than say Process Material colours, but this resource is fully kitted out with getters/setters.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 18, 2020

or even documented.

They are documented though. They are listed under methods.
image
It was added quite recently.

EDIT:
But this is only built-in docs. Online docs don't include descriptions for the theme items.

@chucklepie
Copy link
Author

chucklepie commented Dec 18, 2020

I'm using the built in docs 👍

Maybe you need to look from the perspective of a 'typical' user (I hope I'm one :) ).

I am at the inspector and I see the entry 'Custom Colors'. I hover over 'font color' and it says:

Property: custom_colors/font_color.

So in code I I type "$Label." and expect 'custom_colors' to popup, but it doesn't.

So I go to the help and I can see 'Theme Properties' and there is a 'font_color'. But in my head two questions:

  1. is theme properties font_color the same as 'Custom Colors' 'font_color'?
  2. if it is, how do I set it because there is nothing telling me that 'theme property' requires some special coding magic as I treat this as just another heading like Methods and Properties. I'm just stuck here looking, wondering what to do as nothing is there to aid me...

Also,
3. I appreciate you say 'Custom XXX' are Themes. This probably doesn't ring with anyone other than the Godot developers because if Custom Fonts is part of Themes, why is there a separate section called 'Theme'?

Hope this helps :)

@KoBeWi
Copy link
Member

KoBeWi commented Dec 18, 2020

I think the problem is more about making theme properties/overrides be less hacked and more obvious what they are, than adding "proper" setters/getters. Or maybe we could improve the tooltip or documentation (like, by mentioning the add_override methods under theme properties section and clarifying its description more).

Although if setters/getters are enough, they could be better depending on what is easier to implement.

@Zylann
Copy link

Zylann commented Dec 19, 2020

I'm guessing there is a reason for this lack of accessor methods?

Properties are always there. Theme properties, however, have a "not enabled" state. This can be seen as the checkbox in the editor. When a theme property is turned off, the control then fallback to the theme.
If we make these "regular" properties, we loose this feature, unless we use nullable types I guess. Otherwise they would have to be all Variant. Besides, we'll be forced to consider null as "not overriden". "overriden with null" won't be expressible.

@chucklepie
Copy link
Author

chucklepie commented Dec 20, 2020 via email

@YuriSizov
Copy link
Contributor

For the record, they've been renamed to restore the association with the theme, the part of which they actually are. The section is now called "Theme Overrides" in both 3.x and master, with property paths also being renamed in master to reflect the change.

And that's the problem, those theme items are not a part of Control nodes. They are indeed just string keys fetched from the default theme, whatever that might be. So unless something changes drastically about theming in Godot, the only solution here is to better educate users, improve docs, and promote better practices (i.e. using the methods provided by API instead of get/set on the property name).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants