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.

Sign up for GitHub

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

Jump to bottom

Refactor and remove excessive calls of NOTIFICATION_THEME_CHANGED #62845

Merged
merged 1 commit into from
Aug 26, 2022
Merged

Refactor and remove excessive calls of NOTIFICATION_THEME_CHANGED #62845

merged 1 commit into from
Aug 26, 2022

Conversation

AaronRecord
Copy link
Contributor

@AaronRecord AaronRecord commented Jul 8, 2022 •
edited
Loading

Fixes (partially, see #62846) #50743
Fixes #62844
Fixes #61765

See the updated NOTIFICATION_THEME_CHANGED documentation for when it should be called.

After this is merged, #62846 can be merged, and #50743 can be closed.

@AaronRecord AaronRecord mentioned this pull request Jul 8, 2022
Merged
@Calinou Calinou added this to the 4.0 milestone Jul 8, 2022
@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch from b73b6b6 to ce57f31 Compare July 9, 2022 18:48
@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch from ce57f31 to ad1e6a7 Compare July 27, 2022 00:21
@AaronRecord
Copy link
Contributor Author

Okay, originally the plan was to wait to set theme_owner's until when the control/window enters the tree, and set it to nullptr when the control/window leaves the scene tree, but the problem is there is a lot of code in the editor that doesn't fetch it's theme data in NOTIFICATION_THEME_CHANGED like it should, and so for now theme_owner will still get assigned outside of the scene tree and everything works as before.

@AaronRecord AaronRecord marked this pull request as ready for review July 27, 2022 01:00
@AaronRecord AaronRecord requested review from a team as code owners July 27, 2022 01:00
@AaronRecord AaronRecord marked this pull request as draft July 27, 2022 01:02
@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch from ad1e6a7 to 14b0754 Compare July 27, 2022 01:16
@AaronRecord AaronRecord marked this pull request as ready for review July 27, 2022 01:32
@YuriSizov
Copy link
Contributor

Can you please rebase this now that #63318 is merged?

@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch from 14b0754 to 88facb5 Compare July 27, 2022 18:12
@AaronRecord AaronRecord marked this pull request as draft July 27, 2022 18:12
@AaronRecord AaronRecord marked this pull request as ready for review July 27, 2022 18:25
@YuriSizov YuriSizov self-requested a review July 27, 2022 18:33
@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch 2 times, most recently from c10e018 to 4bd2b01 Compare July 27, 2022 22:03
@AaronRecord AaronRecord requested a review from a team as a code owner July 27, 2022 22:03
@AaronRecord
Copy link
Contributor Author

AaronRecord commented Jul 27, 2022 •
edited
Loading

Remembered I should update the documentation as well

@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch from 4bd2b01 to 93cba19 Compare July 27, 2022 22:25
@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch 2 times, most recently from f642dad to ff69395 Compare July 27, 2022 23:49
@AaronRecord AaronRecord marked this pull request as draft July 29, 2022 00:20
@AaronRecord
Copy link
Contributor Author

Marking as a draft temporarily, I want to tweak the documentation a bit and double check this is the best way to do things before this is reviewed/merged

@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch from ff69395 to bf568e3 Compare July 30, 2022 04:06
@AaronRecord AaronRecord changed the title Fix excessive calls to NOTIFICATION_THEME_CHANGED Refactor and remove excessive calls of NOTIFICATION_THEME_CHANGED Jul 30, 2022
@AaronRecord
Copy link
Contributor Author

Ready for review :P

YuriSizov
YuriSizov suggested changes Aug 3, 2022
View reviewed changes
Copy link
Contributor

@YuriSizov YuriSizov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some changes required, some nitpicks. This looks good overall. I'm yet to test this in practice, but the code looks solid! Great work.

PS. If I didn't mention some of the notes on window but did on control in the similar code, assume it applies to both.

scene/gui/control.cpp Outdated Show resolved Hide resolved
doc/classes/Control.xml Outdated Show resolved Hide resolved
doc/classes/Window.xml Outdated Show resolved Hide resolved
doc/classes/Control.xml Outdated Show resolved Hide resolved
doc/classes/Control.xml Outdated Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Show resolved Hide resolved
scene/main/window.cpp Outdated Show resolved Hide resolved
scene/main/window.cpp Outdated Show resolved Hide resolved
@YuriSizov YuriSizov mentioned this pull request Aug 3, 2022
Closed
@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch from 65a66e5 to eda1dce Compare August 3, 2022 18:50
@AaronRecord
Copy link
Contributor Author

@YuriSizov changes made :)

@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch from eda1dce to 05f0538 Compare August 3, 2022 19:18
YuriSizov
YuriSizov approved these changes Aug 5, 2022
View reviewed changes
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Changes make sense to me and make the code's flow clearer IMO. I can't spot any immediate regressions and can't think of a possible regression in logic. The two linked issues are indeed fixed now.

KoBeWi
KoBeWi reviewed Aug 18, 2022
View reviewed changes
scene/gui/control.cpp Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

Also needs a rebase, then should be good to go

@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch from 05f0538 to 1e112fb Compare August 18, 2022 23:23
@AaronRecord
Copy link
Contributor Author

██████╗░███████╗██████╗░░█████╗░░██████╗███████╗██████╗░
██╔══██╗██╔════╝██╔══██╗██╔══██╗██╔════╝██╔════╝██╔══██╗
██████╔╝█████╗░░██████╦╝███████║╚█████╗░█████╗░░██║░░██║
██╔══██╗██╔══╝░░██╔══██╗██╔══██║░╚═══██╗██╔══╝░░██║░░██║
██║░░██║███████╗██████╦╝██║░░██║██████╔╝███████╗██████╔╝
╚═╝░░╚═╝╚══════╝╚═════╝░╚═╝░░╚═╝╚═════╝░╚══════╝╚═════╝░

@AaronRecord
Copy link
Contributor Author

AaronRecord commented Aug 19, 2022 •
edited
Loading

... and now a check is failing 🙃

okay, thankfully can reproduce the issue locally, will look into it later

@YuriSizov
Copy link
Contributor

Poke-poke. Would be great to merge this soon!

@AaronRecord
Copy link
Contributor Author

Poke-poke. Would be great to merge this soon!

Sorry 😅
I'll try and get this fixed today. I'd love to spend more time contributing to Godot, I've just been really busy with work and other projects I'm working on 😃

@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch from 1e112fb to 8fd07e4 Compare August 25, 2022 20:41
@AaronRecord AaronRecord force-pushed the dont_update_theme_outside_of_tree branch from 8fd07e4 to 74eb2a7 Compare August 25, 2022 20:50
@akien-mga akien-mga merged commit 7bb92bc into godotengine:master Aug 26, 2022
@akien-mga
Copy link
Member

Thanks!

@AaronRecord AaronRecord deleted the dont_update_theme_outside_of_tree branch August 26, 2022 14:46
akien-mga added a commit to akien-mga/godot that referenced this pull request Aug 29, 2022
@akien-mga
Revert "Remove NOTIFICATION_ENTER_TREE when paired with NOTIFICATION_…
fd6453c 
…THEME_CHANGED"

This reverts commit 4b817a5.

Fixes godotengine#64988.
Fixes godotengine#64997.

This caused several regressions (godotengine#64988, godotengine#64997,
godotengine#64997 (comment))
which point at a flaw in the current logic:

- `Control::NOTIFICATION_ENTER_TREE` triggers a *deferred* notification with
  `NOTIFCATION_THEME_CHANGED` as introduced in godotengine#62845.
- Some classes use their `THEME_CHANGED` to cache theme items in
  member variables (e.g. `style_normal`, etc.), and use those member
  variables in `ENTER_TREE`, `READY`, `DRAW`, etc. Since the `THEME_CHANGE`
  notification is now deferred, they end up accessing invalid state and this
  can lead to not applying theme properly (e.g. for EditorHelp) or crashing
  (e.g. for EditorLog or CodeEdit).

So we need to go back to the drawing board and see if `THEME_CHANGED` can be
called earlier so that the previous logic still works?

Or can we refactor all engine code to make sure that:
- `ENTER_TREE` and similar do not depend on theme properties cached in member
  variables.
- Or `THEME_CHANGE` does trigger a general UI update to make sure that any
  bad theme handling in `ENTER_TREE` and co. gets fixed when `THEME_CHANGE`
  does arrive for the first time. But that means having a temporary invalid
  (and possibly still crashing) state, and doing some computations twice
  which might be heavy (e.g. `EditorHelp::_update_doc()`).
@akien-mga akien-mga mentioned this pull request Aug 29, 2022
Merged
@YuriSizov YuriSizov mentioned this pull request Aug 29, 2022
Closed
@YuriSizov YuriSizov mentioned this pull request Aug 31, 2022
Merged
Rindbee
Rindbee reviewed Sep 1, 2022
View reviewed changes
scene/main/window.cpp Show resolved Hide resolved
scene/main/window.cpp Show resolved Hide resolved
scene/main/window.cpp Show resolved Hide resolved
@akien-mga akien-mga mentioned this pull request Sep 1, 2022
Closed
@YuriSizov YuriSizov mentioned this pull request Sep 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KoBeWi KoBeWi KoBeWi left review comments

@YuriSizov YuriSizov YuriSizov approved these changes

@Rindbee Rindbee Rindbee left review comments

@reduz reduz reduz approved these changes

Assignees
No one assigned
Labels
bug topic:gui
Projects
None yet
Milestone
4.0
7 participants
@AaronRecord @YuriSizov @akien-mga @KoBeWi @reduz @Rindbee @Calinou