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.

Sign up for GitHub

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

Jump to bottom

Calculate chevron size in CTabFolderRenderer without Device#getDPI() #1754

Merged
merged 1 commit into from
Jan 25, 2025
Merged

Calculate chevron size in CTabFolderRenderer without Device#getDPI() #1754

merged 1 commit into from
Jan 25, 2025
+12 −5

Conversation

ShahzaibIbrahim
Copy link
Contributor

getDPI always returns the points for primary monitor and can produce faulty behavior. Replacing the logic to calculate the chevron font size. Visibly 70% size of the font used for CTabItem is now used. Also adjusting the indent and adding the ZoomChange event to trigger redraw during DPI change.

NOTE: Needs to be tested on linux and macOS

Steps to Reproduce

  • Run the CustomControlExample with following arguments in 150% primary monitor

-Dswt.autoScale.updateOnRuntime=true
-Dswt.autoScale=quarter

  • Change the tab to "CTabFolder"
  • Change the Size to 100x100
  • Move the shell to other monitor with (100 or 200%)
  • The number along with chevron image should look fine.
  • Additional: The tabs should be aligned correctly as well.

Before Fix

image

After Fix

image

Scenarios Tested yet:
Case Quarter no args
150 -> 100 OK OK
150 -> 200 OK OK

@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as draft January 22, 2025 14:20
@ShahzaibIbrahim ShahzaibIbrahim linked an issue Jan 22, 2025 that may be closed by this pull request
Replace Device:getDPI() usage in CTabFolderRenderer vi-eclipse/Eclipse-Platform#208
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 22, 2025 •
edited
Loading

Test Results

   494 files  ±0     494 suites  ±0   9m 52s ⏱️ +33s
 4 331 tests ±0   4 318 ✅ ±0   13 💤 ±0  0 ❌ ±0 
16 572 runs  ±0  16 464 ✅ ±0  108 💤 ±0  0 ❌ ±0 

Results for commit 164c0f4. ± Comparison against base commit 88d5b53.

♻️ This comment has been updated with latest results.

HeikoKlare
HeikoKlare reviewed Jan 23, 2025
View reviewed changes
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.

Looks good on Linux/GTK. I don't see any noticable difference to existing appearance.

In the following screenshot, the left is existing and the right is with this PR:
image

Screenshots are taken at 100% monitor scaling. Looks the same (blurry scaled) with other display scalings, both before and after the change.

HeikoKlare
HeikoKlare reviewed Jan 23, 2025
View reviewed changes
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.

@BeckerWdf I just saw that you originally introduced the scaling logic for the chevrons several years ago. Do you still remember that code and have an opinion on these changes or should we just move on?

...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java Show resolved Hide resolved
...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java Outdated Show resolved Hide resolved
HeikoKlare
HeikoKlare reviewed Jan 23, 2025
View reviewed changes
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.

Also fine on MacOS:
image

@BeckerWdf
Copy link
Contributor

@BeckerWdf I just saw that you originally introduced the scaling logic for the chevrons several years ago. Do you still remember that code and have an opinion on these changes or should we just move on?

You mean this change: f25be5b?

@HeikoKlare
Copy link
Contributor

You mean this change: f25be5b?

Yes, that change.

@BeckerWdf
Copy link
Contributor

You mean this change: f25be5b?

Yes, that change.

Sorry. I don't remember the details. Just also read the bugzilla.
If it looks good with this PR on windows, mac and linux I think you can continue with this.

@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as ready for review January 24, 2025 10:36
HeikoKlare
HeikoKlare approved these changes Jan 24, 2025
View reviewed changes
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.

Looks good now. Can you squash the commits please?

@HeikoKlare HeikoKlare force-pushed the master-208 branch 5 times, most recently from cdf8c61 to d47b00b Compare January 25, 2025 08:01
@ShahzaibIbrahim @HeikoKlare
Calculate chevron size in CTabFolderRenderer without Device#getDPI()
164c0f4 
The chevron font size is currently determined using Device#getDPI().
This value is static across the application lifecyle, always returns the
DPI for the primary monitor at application startup. The font does thus
not adapt properly to other zoom values.

This change replaces the logic to calculate the chevron font size to use
a scaled version of the font used for CTabItem. It also adds a listener
for the zoom change event to trigger redraw.
@HeikoKlare HeikoKlare changed the title Getting rid of getDPI to calculate chevron size Calculate chevron size in CTabFolderRenderer without Device#getDPI() Jan 25, 2025
@HeikoKlare HeikoKlare merged commit 6d374d9 into eclipse-platform:master Jan 25, 2025
14 checks passed
@HeikoKlare HeikoKlare deleted the master-208 branch January 25, 2025 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akoch-yatta akoch-yatta akoch-yatta left review comments

@BeckerWdf BeckerWdf BeckerWdf left review comments

@HeikoKlare HeikoKlare HeikoKlare approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace Device:getDPI() usage in CTabFolderRenderer
4 participants
@ShahzaibIbrahim @BeckerWdf @HeikoKlare @akoch-yatta