Skip to content

Conversation

@ShahzaibIbrahim
Copy link
Contributor

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.

@ShahzaibIbrahim ShahzaibIbrahim linked an issue Dec 10, 2025 that may be closed by this pull request
1 task
@github-actions
Copy link
Contributor

github-actions bot commented Dec 10, 2025

Test Results

  176 files  ±0    176 suites  ±0   26m 23s ⏱️ +9s
4 672 tests ±0  4 650 ✅ ±0  22 💤 ±0  0 ❌ ±0 
  482 runs  ±0    476 ✅ ±0   6 💤 ±0  0 ❌ ±0 

Results for commit fba56a0. ± Comparison against base commit 383ed2a.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the remaining usages of Device#getDPI(), this PR needs to adds according warnings suppressions.
On Windows, I currently see these three remaining usages of the method, of which I am not sure if the first two are expected:
image

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-224-deprecation branch 3 times, most recently from f679da6 to 3c222e0 Compare January 2, 2026 14:15
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.
Copy link
Contributor

@akoch-yatta akoch-yatta left a 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.

@akoch-yatta akoch-yatta merged commit 42c3226 into eclipse-platform:master Jan 2, 2026
31 checks passed
@akoch-yatta akoch-yatta deleted the master-224-deprecation branch January 2, 2026 17:43
@Phillipus
Copy link
Contributor

Phillipus commented Jan 2, 2026

We access getDPIor scaling fonts depending on OS:

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
Copy link
Contributor

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.

@laeubi
Copy link
Contributor

laeubi commented Jan 2, 2026

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 Device.getDPI()

While I understand the rationale regarding multi-monitor scenarios for Display, deprecating getDPI() at the Device level is overly broad and problematic. This method remains critically important for other Device subclasses:

  1. Printer - DPI is essential for print operations (e.g., rasterizing images at the correct printer resolution)
  2. PDFDocument - DPI matters for proper PDF rendering and scaling
  3. Platform-specific font scaling - As @Phillipus demonstrated in #2925, applications rely on this for cross-platform font calculations

Suggested Alternative Approach

Instead of deprecating at the Device level, I propose:

  1. Keep Device.getDPI() non-deprecated since it's valid and necessary for Printer and PDFDocument use cases

  2. Override and deprecate specifically in Display with clear documentation:

    /**
     * @deprecated In multi-monitor environments, this method returns the DPI 
     * of the primary monitor only, which may not reflect the actual DPI of 
     * the monitor where your window is displayed.  
     * Use {@link #getDPI(Monitor)} to get monitor-specific DPI values.
     */
    @Override
    @Deprecated
    public Point getDPI() { ...  }
  3. Add a new method to Display:

    /**
     * Returns the DPI for the specified monitor.
     * @param monitor the monitor to query
     * @return the dots per inch of the specified monitor
     * @since 3.XXX
     */
    public Point getDPI(Monitor monitor) { ... }
  4. Document consistent behavior: Clarify that Display.getDPI() returns the DPI of the primary monitor to maintain backward compatibility while adding the new API for multi-monitor awareness.

Migration Path

This approach provides:

  • ✅ Clear migration path for Display users to monitor-specific APIs
  • ✅ No false deprecation warnings for legitimate Printer/PDFDocument use cases
  • ✅ Backward compatibility with well-defined semantics
  • ✅ Future-proof API for multi-monitor scenarios

Given that this PR has already been merged, I'd recommend considering a follow-up PR to refine the deprecation strategy along these lines.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mark Device::getDPI() method as deprecated

5 participants