-
Notifications
You must be signed in to change notification settings - Fork 189
Deprecating Display#getDPI #2872
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
Deprecating Display#getDPI #2872
Conversation
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/Device.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Device.java
Show resolved
Hide resolved
3fbcfca to
a4430dd
Compare
HeikoKlare
left a 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.
f679da6 to
3c222e0
Compare
Having the scale factor being based on the screen DPI leads to unexpected result e.g. Image too big/small. Having a screen dpi independent factor leads to consistent results.
3c222e0 to
fba56a0
Compare
akoch-yatta
left a 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.
LGTM, some required (internal) usages are guarded with SupressWarnings - I did not find any missed places.
The usages in PDFDocument must be removed and are not working properly in all combinations of zooms and autoscale modes, but simply removing it does not solve all issues. Therefor, it makes sense to have a holistic view on it in a second step to properly solve the existing issues.
|
We access If this is deprecated can you suggest what to use instead? Edit - I opened #2925 with more details. |
| * this method may still be validly used when called on a {@code Printer} instance, | ||
| * where a single global DPI value is meaningful and expected.</p> | ||
| */ | ||
| @Deprecated |
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 deprecation on Device is then wrong here in my opinion and Display should override the method and deprecate it individually. One might even define this for a display to return the smallest DPI setting of all monitors, or return the one of the primary monitor.
|
Sorry for being late to this discussion—I somehow missed the PR initially and its broader implications. After reviewing the changes, I have serious concerns about the scope of this deprecation. The Problem with Deprecating While I understand the rationale regarding multi-monitor scenarios for
Suggested Alternative Approach Instead of deprecating at the
Migration Path This approach provides:
Given that this PR has already been merged, I'd recommend considering a follow-up PR to refine the deprecation strategy along these lines. |

Having the scale factor being based on the screen DPI leads to unexpected result e.g. Image too big/small. Having a screen dpi independent factor leads to consistent results.