-
-
Notifications
You must be signed in to change notification settings - Fork 22k
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
Remove code duplication in Button #93389
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, good improvement reducing the risk of errors
Just left a potential improvement suggestion
theme_cache.style_margin_right = 0; | ||
theme_cache.style_margin_top = 0; | ||
theme_cache.style_margin_bottom = 0; | ||
|
||
const bool rtl = is_layout_rtl(); | ||
if (rtl && has_theme_stylebox(SNAME("normal_mirrored"))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have theme cache here I wonder if we can replace this check with a check against:
if (rtl && has_theme_stylebox(SNAME("normal_mirrored"))) { | |
if (rtl && theme_cache.normal_mirrored.is_valid()) { |
I'm not extremely well versed with the theme system but looking at the code it seems it should be equivalent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about that, see #84944 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! Though I realized (hence unresolving) that if they aren't equivalent this code will segfault, so even if it's not a 100% drop in it does handle potential crashes, hasn't been a problem in the past though it seems
No description provided.