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 Button's content_margin not applying #92625

Closed
wants to merge 1 commit into from

Conversation

exyxz
Copy link
Contributor

@exyxz exyxz commented Jun 1, 2024

Fixes #92602 and fixes #92466

This PR simply moves Control::_update_theme_item_cache all the way at the end of Button::_update_theme_item_cache.

Amateur

@exyxz exyxz requested a review from a team as a code owner June 1, 2024 07:43
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

This was actually a deliberate change (see #92009 and #91022) to prevent twitching buttons. Is there a specific reason why you want this behavior back? In this case, #92009 should be reverted instead (probably only offset caching).

Or we should an extra toggle to the theme constants to allow both options, since both behaviors might be useful.

@exyxz
Copy link
Contributor Author

exyxz commented Jun 1, 2024

What if, instead, revert back to what was originally intended, but add another one at, again, end of that function, is this good idea?

@bruvzg
Copy link
Member

bruvzg commented Jun 1, 2024

What if, instead, revert back to what was originally intended, but add another one at, again, end of that function, is this good idea?

Adding anything to _update_theme_item_cache or (or changing it) is definitely not a correct fix, I'm not sure what you are trying to do with it, it's completely unrelated to the issue.

@exyxz
Copy link
Contributor Author

exyxz commented Jun 1, 2024

Tested on both master [705b7a0] and 4.3-beta1 [557f63d] with #92009 and this PR applied.
The editor crashes, still tinker around.

@exyxz
Copy link
Contributor Author

exyxz commented Jun 1, 2024

Okay, by reverting #92009 only for _notification, this fixes not only the problem but also keeps the improvement intact.

Tested with @passivestar's minimal theme

@exyxz exyxz force-pushed the bad-content-margin-bad branch 2 times, most recently from 966f223 to 4074819 Compare June 1, 2024 09:26
@passivestar
Copy link
Contributor

This PR fixes #92466

@passivestar
Copy link
Contributor

This PR brings back the icon problem

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

A few other parts can be removed as well (do not do anything anymore):

diff --git a/scene/gui/button.cpp b/scene/gui/button.cpp
index 3bf9d79c7f..68c7012fe4 100644
--- a/scene/gui/button.cpp
+++ b/scene/gui/button.cpp
@@ -56,70 +56,30 @@ void Button::_update_theme_item_cache() {
 	const bool rtl = is_layout_rtl();
 	if (rtl && has_theme_stylebox(SNAME("normal_mirrored"))) {
 		theme_cache.max_style_size = theme_cache.normal_mirrored->get_minimum_size();
-		theme_cache.style_margin_left = theme_cache.normal_mirrored->get_margin(SIDE_LEFT);
-		theme_cache.style_margin_right = theme_cache.normal_mirrored->get_margin(SIDE_RIGHT);
-		theme_cache.style_margin_top = theme_cache.normal_mirrored->get_margin(SIDE_TOP);
-		theme_cache.style_margin_bottom = theme_cache.normal_mirrored->get_margin(SIDE_BOTTOM);
 	} else {
 		theme_cache.max_style_size = theme_cache.normal->get_minimum_size();
-		theme_cache.style_margin_left = theme_cache.normal->get_margin(SIDE_LEFT);
-		theme_cache.style_margin_right = theme_cache.normal->get_margin(SIDE_RIGHT);
-		theme_cache.style_margin_top = theme_cache.normal->get_margin(SIDE_TOP);
-		theme_cache.style_margin_bottom = theme_cache.normal->get_margin(SIDE_BOTTOM);
 	}
 	if (has_theme_stylebox("hover_pressed")) {
 		if (rtl && has_theme_stylebox(SNAME("hover_pressed_mirrored"))) {
 			theme_cache.max_style_size = theme_cache.max_style_size.max(theme_cache.hover_pressed_mirrored->get_minimum_size());
-			theme_cache.style_margin_left = MAX(theme_cache.style_margin_left, theme_cache.hover_pressed_mirrored->get_margin(SIDE_LEFT));
-			theme_cache.style_margin_right = MAX(theme_cache.style_margin_right, theme_cache.hover_pressed_mirrored->get_margin(SIDE_RIGHT));
-			theme_cache.style_margin_top = MAX(theme_cache.style_margin_top, theme_cache.hover_pressed_mirrored->get_margin(SIDE_TOP));
-			theme_cache.style_margin_bottom = MAX(theme_cache.style_margin_bottom, theme_cache.hover_pressed_mirrored->get_margin(SIDE_BOTTOM));
 		} else {
 			theme_cache.max_style_size = theme_cache.max_style_size.max(theme_cache.hover_pressed->get_minimum_size());
-			theme_cache.style_margin_left = MAX(theme_cache.style_margin_left, theme_cache.hover_pressed->get_margin(SIDE_LEFT));
-			theme_cache.style_margin_right = MAX(theme_cache.style_margin_right, theme_cache.hover_pressed->get_margin(SIDE_RIGHT));
-			theme_cache.style_margin_top = MAX(theme_cache.style_margin_top, theme_cache.hover_pressed->get_margin(SIDE_TOP));
-			theme_cache.style_margin_bottom = MAX(theme_cache.style_margin_bottom, theme_cache.hover_pressed->get_margin(SIDE_BOTTOM));
 		}
 	}
 	if (rtl && has_theme_stylebox(SNAME("pressed_mirrored"))) {
 		theme_cache.max_style_size = theme_cache.max_style_size.max(theme_cache.pressed_mirrored->get_minimum_size());
-		theme_cache.style_margin_left = MAX(theme_cache.style_margin_left, theme_cache.pressed_mirrored->get_margin(SIDE_LEFT));
-		theme_cache.style_margin_right = MAX(theme_cache.style_margin_right, theme_cache.pressed_mirrored->get_margin(SIDE_RIGHT));
-		theme_cache.style_margin_top = MAX(theme_cache.style_margin_top, theme_cache.pressed_mirrored->get_margin(SIDE_TOP));
-		theme_cache.style_margin_bottom = MAX(theme_cache.style_margin_bottom, theme_cache.pressed_mirrored->get_margin(SIDE_BOTTOM));
 	} else {
 		theme_cache.max_style_size = theme_cache.max_style_size.max(theme_cache.pressed->get_minimum_size());
-		theme_cache.style_margin_left = MAX(theme_cache.style_margin_left, theme_cache.pressed->get_margin(SIDE_LEFT));
-		theme_cache.style_margin_right = MAX(theme_cache.style_margin_right, theme_cache.pressed->get_margin(SIDE_RIGHT));
-		theme_cache.style_margin_top = MAX(theme_cache.style_margin_top, theme_cache.pressed->get_margin(SIDE_TOP));
-		theme_cache.style_margin_bottom = MAX(theme_cache.style_margin_bottom, theme_cache.pressed->get_margin(SIDE_BOTTOM));
 	}
 	if (rtl && has_theme_stylebox(SNAME("hover_mirrored"))) {
 		theme_cache.max_style_size = theme_cache.max_style_size.max(theme_cache.hover_mirrored->get_minimum_size());
-		theme_cache.style_margin_left = MAX(theme_cache.style_margin_left, theme_cache.hover_mirrored->get_margin(SIDE_LEFT));
-		theme_cache.style_margin_right = MAX(theme_cache.style_margin_right, theme_cache.hover_mirrored->get_margin(SIDE_RIGHT));
-		theme_cache.style_margin_top = MAX(theme_cache.style_margin_top, theme_cache.hover_mirrored->get_margin(SIDE_TOP));
-		theme_cache.style_margin_bottom = MAX(theme_cache.style_margin_bottom, theme_cache.hover_mirrored->get_margin(SIDE_BOTTOM));
 	} else {
 		theme_cache.max_style_size = theme_cache.max_style_size.max(theme_cache.hover->get_minimum_size());
-		theme_cache.style_margin_left = MAX(theme_cache.style_margin_left, theme_cache.hover->get_margin(SIDE_LEFT));
-		theme_cache.style_margin_right = MAX(theme_cache.style_margin_right, theme_cache.hover->get_margin(SIDE_RIGHT));
-		theme_cache.style_margin_top = MAX(theme_cache.style_margin_top, theme_cache.hover->get_margin(SIDE_TOP));
-		theme_cache.style_margin_bottom = MAX(theme_cache.style_margin_bottom, theme_cache.hover->get_margin(SIDE_BOTTOM));
 	}
 	if (rtl && has_theme_stylebox(SNAME("disabled_mirrored"))) {
 		theme_cache.max_style_size = theme_cache.max_style_size.max(theme_cache.disabled_mirrored->get_minimum_size());
-		theme_cache.style_margin_left = MAX(theme_cache.style_margin_left, theme_cache.disabled_mirrored->get_margin(SIDE_LEFT));
-		theme_cache.style_margin_right = MAX(theme_cache.style_margin_right, theme_cache.disabled_mirrored->get_margin(SIDE_RIGHT));
-		theme_cache.style_margin_top = MAX(theme_cache.style_margin_top, theme_cache.disabled_mirrored->get_margin(SIDE_TOP));
-		theme_cache.style_margin_bottom = MAX(theme_cache.style_margin_bottom, theme_cache.disabled_mirrored->get_margin(SIDE_BOTTOM));
 	} else {
 		theme_cache.max_style_size = theme_cache.max_style_size.max(theme_cache.disabled->get_minimum_size());
-		theme_cache.style_margin_left = MAX(theme_cache.style_margin_left, theme_cache.disabled->get_margin(SIDE_LEFT));
-		theme_cache.style_margin_right = MAX(theme_cache.style_margin_right, theme_cache.disabled->get_margin(SIDE_RIGHT));
-		theme_cache.style_margin_top = MAX(theme_cache.style_margin_top, theme_cache.disabled->get_margin(SIDE_TOP));
-		theme_cache.style_margin_bottom = MAX(theme_cache.style_margin_bottom, theme_cache.disabled->get_margin(SIDE_BOTTOM));
 	}
 }
 
diff --git a/scene/gui/button.h b/scene/gui/button.h
index eefb690913..477260e6b9 100644
--- a/scene/gui/button.h
+++ b/scene/gui/button.h
@@ -70,10 +70,6 @@ private:
 		Ref<StyleBox> focus;
 
 		Size2 max_style_size;
-		float style_margin_left = 0;
-		float style_margin_right = 0;
-		float style_margin_top = 0;
-		float style_margin_bottom = 0;
 
 		Color font_color;
 		Color font_focus_color;

@exyxz exyxz force-pushed the bad-content-margin-bad branch from 4074819 to d81a639 Compare June 1, 2024 14:54
@AThousandShips
Copy link
Member

This PR brings back the icon problem

I'd say this would have to be resolved if this is to be merged

@akien-mga
Copy link
Member

Superseded by #92701. Thanks for the contribution!

@akien-mga akien-mga closed this Jun 11, 2024
@akien-mga akien-mga removed this from the 4.x milestone Jun 11, 2024
@exyxz exyxz deleted the bad-content-margin-bad branch June 28, 2024 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants