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

Moving icon updates to THEME_CHANGED notification #11096

Closed
toger5 opened this issue Sep 9, 2017 · 1 comment
Closed

Moving icon updates to THEME_CHANGED notification #11096

toger5 opened this issue Sep 9, 2017 · 1 comment

Comments

@toger5
Copy link
Contributor

toger5 commented Sep 9, 2017

currently icons are updated when on ENTER_TREE + SETTINGS_CHANGED since the controls are not inside the node tree get_icon would return the wrong / no icon. (not part of gui_base).

So in ENTER_TREE the icons (for buttons...) are available and get set. What the user switches to the white theme the icons need to get set again... since the icons color changed.
This was done in SETTINGS_CHANGED.

So a lot of icon update code is duplicated... which sucks... of course.

The proposal is to move everything int THEME_CHANGED since that is called in enter tree. (from default theme to Gui_base theme) and obviously when the theme gets changed in the settings. that also results in icons only updating when the settings change the theme not for every settings change.

one issue with that approach is add_style_override + add_constant_override. these functions themselves trigger the THEME_CHANGED notification. as a result there is an endless loop...

We could keep those functions in enter tree + settings changed. But that would eliminate the performance increase since the than any settings change would again trigger THEME_CHANGED -> and the icons update...

those functions (add_style_override + add_constant_override) could also have a property to trigger theme_changed or not... but I don't know if that would work than... most style overrides are loaded in draw methods anyways so there it does not matter.

Maybe there could be THEME_WILL_CHANGE + THEME_CHANGED.
Will change gets called before and changed after...

I don't know what the best approach is. moving them into THEME_CHAGNED seems like a good idea, but how to handle overrides ?

ping @djrm

@Calinou
Copy link
Member

Calinou commented Jun 18, 2020

Feature and improvement proposals for the Godot Engine are now being discussed and reviewed in a dedicated Godot Improvement Proposals (GIP) (godotengine/godot-proposals) issue tracker. The GIP tracker has a detailed issue template designed so that proposals include all the relevant information to start a productive discussion and help the community assess the validity of the proposal for the engine.

The main (godotengine/godot) tracker is now solely dedicated to bug reports and Pull Requests, enabling contributors to have a better focus on bug fixing work. Therefore, we are now closing all older feature proposals on the main issue tracker.

If you are interested in this feature proposal, please open a new proposal on the GIP tracker following the given issue template (after checking that it doesn't exist already). Be sure to reference this closed issue if it includes any relevant discussion (which you are also encouraged to summarize in the new proposal). Thanks in advance!

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

3 participants