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

Calculate Scaling for Font without Device#getDPI() #1802

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

ShahzaibIbrahim
Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim commented Feb 4, 2025

getDPI always returns points for primary monitor and can produce faulty behavior. Printing the code is broken because Printer DPI (400) is different from the Monitor DPI (100). Now we compute points from the zoom level instead of currentFontDPI.

How to Test

  1. Run the runtime Workspace
  2. Move the shell from 100% primary monitor to 200% secondary monitor
  3. Press Ctrl + P
  4. Save the preview as pdf
  5. See if the code is printed with correct formatting.

Before Fix

image

After Fix

image

Copy link
Contributor

github-actions bot commented Feb 4, 2025

Test Results

   502 files  ±0     502 suites  ±0   12m 31s ⏱️ + 2m 31s
 4 334 tests ±0   4 320 ✅ ±0   14 💤 ±0  0 ❌ ±0 
16 575 runs  ±0  16 466 ✅ ±0  109 💤 ±0  0 ❌ ±0 

Results for commit fbc1b3a. ± Comparison against base commit 54dad6c.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

FWIW I tested this in Mac and it doesn't produce any regressions. See vi-eclipse/Eclipse-Platform#159 (comment)

This commit solves two interconneted issues. Wrong layouting when text is
printed and wrongly scaled fonts for printing. Device#getDPI returns values
for Display and Printer that lead to wrong assumption using getDPI e.g.
in the ScalingSWTFontRegistry. Printing text is broken because Printer
DPI (e.g. 600) is way different from the Monitor DPI (96) that lead to
inconsistent Font behavior in the different scenarios. Now font scaling
is utilizing the zoom level instead of DPI in relavant scenarios.
@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as ready for review February 6, 2025 11:57
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

The code looks fine and my previous test (on Mac) was green. I can't test on my Mac at the moment since I'm at the office, but once it's tested then this PR is good to go on my account.

@fedejeanne
Copy link
Contributor

@fedejeanne fedejeanne merged commit cd8bb13 into eclipse-platform:master Feb 10, 2025
14 checks passed
@fedejeanne fedejeanne deleted the master-210 branch February 10, 2025 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants