-
Notifications
You must be signed in to change notification settings - Fork 177
Make fonts use autoscaled zoom when swt.autoScale is fixed #2377
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 fonts use autoscaled zoom when swt.autoScale is fixed #2377
Conversation
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/DPIUtil.java
Outdated
Show resolved
Hide resolved
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.
The changes look sound to me. I have some proposals and implicit questions whether I understood everything correctly.
return DPIUtil.getNativeZoomForAutoscaleProperty(gc.data.nativeZoom); | ||
} | ||
return nativeZoom; | ||
return DPIUtil.getNativeZoomForAutoscaleProperty(nativeZoom); |
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.
I wonder if this is really the right place/change to TextLayout
. We should clarify what the field nativeZoom
in TextLayout
is supposed to be: is it really a nativeZoom
(such as the native zoom of a GC of a StyledText) or it is rather a fontZoom
? If it was really a nativeZoom
, I would find the change weird because computeRuns
calls this method to set the nativeZoom
field, which would make it effectively some fontZoom
. When I take a look at the usages of nativeZoom
, it seems to rather be a fontZoom
. Then, however, calling DPIUtil.getNativeZoomForAutoscaleProperty(nativeZoom)
would not really make sense here, as nativeZoom
already is such an adapted value.
In my understanding nativeZoom
should be renamed to fontZoom
and then only the change to the first return statement should be applied.
this really the right place for the adaptation in TextLayout()
? I see three callers of this method:
computeRuns()
calls it to set a newnativeZoom
: shouldn't that native zoom
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 the native zoom is set based on a font when a font is applied to a TextLayout, it makes sense to rename nativeZoom to fontZoom.
I reviewed all usages of the variable. There is a minor ambiguity in the getZoom() method, where nativeZoom
(now fontZoom
) used. This method is called for line and offset operations, which I am slightly not sure if it is directly tied to font . However, since line height and offsets are also influenced by the font zoom, I believe the renaming still makes sense.
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/DPIUtil.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/DPIUtil.java
Outdated
Show resolved
Hide resolved
1799f06
to
057a824
Compare
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.
Thank you for the update, looks even cleaner now!
I have only minor comments/proposals left and one question regarding the initialization of the fontZoom
(former nativeZoom
) in TextLayout
.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/TextLayout.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/DPIUtil.java
Outdated
Show resolved
Hide resolved
a21a988
to
d0abdb3
Compare
The change now looks fine to me. It just has conflicts now because we meanwhile merged #2376. |
d0abdb3
to
f34f607
Compare
swt.autoScale is fixed This change ensures that images and fonts share the same zoom level when a fixed autoscale value is used. Previously, when different GCs were created for images and widgets, their native zoom levels could differ, causing inconsistencies. For example, in LineNumberRuler, one GC is created from an image and another from a widget for text measurement. The differing native zooms led to incorrect text width calculations when applied across GCs. This update aligns fontZoom to the autoscaled zoom value when autoScale value is fixed Fixes eclipse-platform#2311
f34f607
to
518533c
Compare
done |
@arunjose696 since we found that this PR was created under a wrong assumption (fonts to be scaled according to autoscale zoom instead of native zoom when using a fixed autoscale value), I think unfortunately this PR cannot be merged/adapted in a reasonable way. Can we close it or do you see anything of value in it that we should extract into another PR? |
Closing this PR |
This change ensures that images and fonts share the same zoom level when a fixed autoscale value is used.
Previously, when different GCs were created for images and widgets, their native zoom(GC.data.nativeZoom) levels could differ, causing inconsistencies.
For example, in
LineNumberRuler
, one GC is created from an image (used for rendering), and another is created from a widget (used for text measurement). The differing native zoom levels between these GCs led to incorrect text width calculations. Specifically, one GC was used to measure the text extent, while the other used those measurements to calculate the width of the columns where the text would be drawn and this resulted in #2311Steps to reproduce
1)Checkout the first commit. Run steps in #2306
2)Checkout the second commit, this solves the issue with lineNumberRuler
First commit which reverts changes from #2306 Fixes #2361
Second commit fixes the original issue in #2311