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

Make sure that data.last_minimum_size is consistent with get_combined_minimum_size() at the same time (reverted) #77651

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented May 30, 2023

Move the code for switching data.minimum_size_valid in Control::update_minimum_size() to Control::_update_minimum_size(), make sure to switch data.minimum_size_valid to false only before updating data.last_minimum_size , so that it will remain consistent with get_combined_minimum_size().

Fixes #74052.
Supersedes #77573. In the method connected to the minimum_size_changed signal, the result obtained by using get_combined_minimum_size() is consistent with data.last_minimum_size.

Works fine with #77280.

@Rindbee Rindbee requested a review from a team as a code owner May 30, 2023 08:27
@Chaosus Chaosus added this to the 4.1 milestone Jun 1, 2023
scene/gui/box_container.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Jun 1, 2023

I can understand what this PR does, but I didn't delve why it works. But it fixes the linked issue; I didn't test with any project though (yet).
However seems like the code needs some cleanup (see my comments). It feels like you tried a couple of solutions until it worked and there are unnecessary leftover changes.

@@ -1620,6 +1604,21 @@ void Control::update_minimum_size() {
return;
}

Size2 minsize = get_combined_minimum_size();
Copy link
Contributor Author

@Rindbee Rindbee Jun 1, 2023

Choose a reason for hiding this comment

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

Ensuring that data.minimum_size_valid is changed from false to true only happens here.

Previously switched to false in update_minimum_size(), get_combined_minimum_size() may have been called multiple times before getting here. So the value of data.last_minimum_size may be unreliable, causing the necessary minimum_size_changed signal to not be emitted the second time the change actually occurs.

…ned_minimum_size()` at the same time

Move the code for switching `data.minimum_size_valid` in `Control::update_minimum_size()`
to `Control::_update_minimum_size()`, make sure to switch `data.minimum_size_valid` to
`false` only before updating `data.last_minimum_size` , so that it will remain consistent
with `get_combined_minimum_size()`.
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

I tested some GUI projects and didn't see any problems. It fixes the issue, so it's probably fine.

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.

I'm a bit concerned that we are now calling all this invalidation logic on visibility changes. The idea is fine, but it also signals to the owning window that controls have changed, multiple times, from every child of the node that has changed its visibility. At the moment, child_controls_changed has guards, so it's probably not going to making anything worse, but it's still a bit concerning.

@YuriSizov
Copy link
Contributor

Thanks!

@Rindbee Rindbee deleted the fix-update-minimum-size branch June 2, 2023 12:44
@akien-mga akien-mga changed the title Make sure that data.last_minimum_size is consistent with get_combined_minimum_size() at the same time Make sure that data.last_minimum_size is consistent with get_combined_minimum_size() at the same time (reverted) Jun 12, 2023
@akien-mga
Copy link
Member

Reverted by #78009.

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.

Label + [autowrap] in VBoxContainer changes its size at startup (like line_spacing)
5 participants