-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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 EDSCALE
dependency from GUI nodes
#53341
Remove EDSCALE
dependency from GUI nodes
#53341
Conversation
|
||
Theme::set_default(t); | ||
Theme::set_default_base_scale(default_scale); |
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 can create some problems with the default theme looks, or at least change it in some ways. But I feel like it should've already been the thing.
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.
Ping when ready to review. Removing EDSCALE is a good enhancement!
@fire It is ready for review. All GUI nodes are currently handled by this PR. |
Ah, I wasn't sure because of the unresolved review comments. |
Those are mine to highlight some parts that may need a second look 🙃 |
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.
Haven't tested everything, but hiDPI mode on macOS seems to be working fine.
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.
Changes look good to me. I tested locally on 4K laptop and it seems to work well.
The second commit could use a check by @reduz for Control/Window changes.
3ba147e
to
bdbb7b3
Compare
I've addressed concerns that required addressing. Note that the inspector slider for the new property now acts in a buggy way, but it's unrelated to this PR and I already have a patch ready for that (and it would need to be backported for 3.4 too, I think). |
Thanks! |
See #53295 (comment). This PR is split into 3 commits for better readability of individual changes.
Theme
resource's code and added some grouping comments to better explain this huge file and what it consists of. It was a good base for the rest of the work done here. It shouldn't contain any changes to the code, but I removed one unused include.Control
nodes to work with it. Here as well similar treatment was performed on default font and default font size of each theme to make them more usable from controls.EDSCALE
to use theme's base scale factor instead. It was trivial for some, but not for everything. I've also removed it from one place (I'll leave "review" comments below for the interesting parts).Overall, with 3 commits it should be easier to review it, because there are some moving parts in various places involved.
There may be some visible changes in the projects, as the base scale for the default theme now takes HiDPI into account, as that value was already available and in use for generating the theme. Now it's exposed as a Theme property and those same controls which previously used
EDSCALE
will also respect it in projects running on default theme.This is breaking in its nature, so a 3.x port is unlikely. But I can make a dedicated 3.x PR to reorganize the theme resource for easier cherrypicks in future. Can probably expose the new default font methods too.
Incidentally, I was in the area, so this fixes #36761 by superseding #37419. That PR wasn't updated after my review anyway, so this way we will be able to merge it quicker.