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

Fix some small bugs in the Window node #71470

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

YeldhamDev
Copy link
Member

@YeldhamDev YeldhamDev commented Jan 15, 2023

@YeldhamDev YeldhamDev added this to the 4.0 milestone Jan 15, 2023
@YeldhamDev YeldhamDev requested a review from a team as a code owner January 15, 2023 16:58
@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 16, 2023

  • Made child_controls_changed() check if wrap_controls is enabled within itself.

I guess this breaks this again, so this PR needs to add a way to have a debounced call to _update_window_size() without checking for wrap_controls. Either a dedicated set of methods, or a flag for child_controls_changed() to disable this specific check.

@YeldhamDev
Copy link
Member Author

@YuriSizov The correct way to update that would be with embedder->_sub_window_update(this). Does that need a delay as well?

@YuriSizov
Copy link
Contributor

@YeldhamDev I'm not sure if this would cover all the cases with windows and derivatives. Namely, this won't update a detached window. It doesn't matter for the title color, but styleboxes can affect the size of it still, which needs recomputing. _update_window_size(), while not ideal, handles all the cases.

A delay is likely needed either way, because the values can change very quickly from the inspector.

@YeldhamDev
Copy link
Member Author

I'm not sure if this would cover all the cases with windows and derivatives.

Altering theme properties of Window inherited nodes doesn't seem to work at all currently, with or without my PR.

@YuriSizov
Copy link
Contributor

A repro would be appreciated. It used to work in December, when my PR was made. I'm not aware of any regressions since.

@YeldhamDev
Copy link
Member Author

Just create a FileDialog node and alter the properties in the "Theme Overrides" section.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 18, 2023

Just create a FileDialog node and alter the properties in the "Theme Overrides" section.

Its properties are related to the list of files, which doesn't get regenerated on theme changes. I think it's a specific issue with that node. You can see it gets updated on scene save, probably because the list is reconstructed then.

AcceptDialog for example updates the button separation value as you change it.

@YeldhamDev
Copy link
Member Author

YeldhamDev commented Jan 19, 2023

@YuriSizov Alright, made some changes:

  • I removed the wrap_controls requirement from child_controls_changed(), as the documentation indicates it as alternative for when wrap_controls is disabled.
  • Made so that theme changes only trigger a full window size update if wraps_controls is enabled. From what I tested, the theme elements still update just fine, and the ones that make the minimum size bigger shouldn't update the size if wrap_controls is disabled anyways.
  • Added a function for when only the embedded borders need to be updated, with a delay.

@akien-mga akien-mga requested review from bruvzg and YuriSizov January 19, 2023 08:03
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.

Doesn't look like there are any regressions now. The rest of the changes look sensible too.
(Tested with PR artifacts)

@akien-mga akien-mga merged commit aae9694 into godotengine:master Jan 27, 2023
@akien-mga
Copy link
Member

Thanks!

@YeldhamDev YeldhamDev deleted the window_fixes branch January 27, 2023 18:59
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.

3 participants