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 scaling for offset and glyph offset #7145

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

simonfall
Copy link

@simonfall simonfall commented Aug 15, 2023

Issue

Currently, offsets and glyph offsets are not scaled with the display scale factor and applied as-is. When using a single display this is not an issue because the offset values can be manually set to achieve the desired results. When using both low-DPI and high-DPI displays however, the missing scaling leads to inconsistent cell size proportions (see attached screenshot).

Screenshot showing cell size inconsistency

Changes

This PR adds scaling for both offsets (applied in cell size calculation) and glyph offsets (applied in glyph cache). The changes require the glyph cache to be aware of the scale factor.

@simonfall
Copy link
Author

simonfall commented Aug 15, 2023

While the fix results in consistent cell size proportions, the effective offsets for users which previously set the offset and glyph_offset options to target a display with a scale_factor > 1.0 will change. Is this acceptable?

@kchibisov
Copy link
Member

The change is a bit questionable, because some may desire to adjust based on physical pixels and not be constrained by logical pixel grid. It could also weirdly work with non integer scale factors.

@simonfall
Copy link
Author

I agree that the simplicity of adjusting based on physical pixels is great and should be kept. I still think that there is some value in having an option which allows to use offsets and move a window between displays without changing the window's content (I've attached a screenshot with the same window moved between two displays with different scale factors). Would it be OK to add an option for logical pixels and keep the default based on physical pixels?

high_dpi_vs_low_dpi

Regarding non-integer scale factors: The current fix truncates the calculated physical offsets towards zero but this could also be changed to rounding instead.

@kchibisov
Copy link
Member

I agree that the simplicity of adjusting based on physical pixels is great and should be kept. I still think that there is some value in having an option which allows to use offsets and move a window between displays without changing the window's content (I've attached a screenshot with the same window moved between two displays with different scale factors). Would it be OK to add an option for logical pixels and keep the default based on physical pixels?

Maybe due to us scaling the font sizes, actually scaling the offsets is more desirable, but it could lead to situation where you can't offset the way you want. I guess we also have something similar when it comes to padding, and maybe some other metrics, since those are generally using the physical pixels.

@simonfall
Copy link
Author

Yes, but if that's important there could be a config option to offset based on logical vs on physical pixels. What's your suggested way of proceeding with this, @kchibisov?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants