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

regard external program images if zoom value is > 100% #295

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

ptief
Copy link
Contributor

@ptief ptief commented Aug 25, 2022

If scaling is >100% the external program image should be used - currently it always uses the default object file image if the external program images is bigger than the default image with 100% scale.

@ptief ptief force-pushed the externalprogram_scaling branch 2 times, most recently from 71906e5 to 1bc1879 Compare August 25, 2022 15:50
@mickaelistria mickaelistria force-pushed the externalprogram_scaling branch from 1bc1879 to 240f6e3 Compare September 15, 2022 13:16
@mickaelistria
Copy link
Contributor

Can you provide steps to reproduce the issue and to verify the patch? Have you tested it on all 3 supported OSs?

@ptief
Copy link
Contributor Author

ptief commented Sep 15, 2022

I only tested it on Windows 10.
Steps to reproduce:

  1. Check your scaling settings and set it to 100%.
  2. Create a new project or open an existing project and add some files, i.e. excel or word files.
    image
    -> the external program icons are displayed correctly.
  3. set your scaling settings to 125%.
  4. Restart the eclipse application
  5. Check the icons in the package explorer
    image
    -> you will get the default object image icon instead of the external program image.

@mickaelistria
Copy link
Contributor

@lshanmug Would you be able to verify it on macos?

@lshanmug
Copy link
Member

@lshanmug Would you be able to verify it on macos?

Sure will test it out today.

@lshanmug
Copy link
Member

The original problem is not seen on mac, but the icons are blurred.
Tested the changes on macOS with retina display (zoom = 200), the icon size becomes small (1/4th). Please see the screenshot of before and after:

image

@ptief
Copy link
Contributor Author

ptief commented Sep 16, 2022

@lshanmug Thanks for testing on macOS.
It seems that the cocoa HiDPI implementation does not work very well, because I changed it to the new HiDPI-aware getImageData method.
image
Is this maybe the problem in the cocoa org.eclipse.swt.program.Program implementation ?

@lshanmug
Copy link
Member

@ptief Have you tried the patch on Windows at 200% scaling? Does it render as expected?

@mickaelistria Does it render correctly on linux at 100 and 200% scaling with the patch?

Is this maybe the problem in the cocoa org.eclipse.swt.program.Program implementation ?

AFAICS Program.getImageData() always returns image data (16 x16) for 100% zoom without considering the device scaling. From the Program.getImageData() code, I think this is the same for Windows and Cocoa.

@ptief
Copy link
Contributor Author

ptief commented Sep 21, 2022

@lshanmug
You are right, at 175% and 200% scaling I have the same problem and get 16x16 images.
For 100, 125 and 150% the zoom value is 100, for 175, 200, ... the zoom value is 200 (calculcated by org.eclipse.swt.internal.DPIUtil).

For win32 and cocoa org.eclipse.swt.program.Program.getImageData() uses the old method for getting the image data org.eclipse.swt.graphics.Image.getImageData() without zoom, which means it uses zoom value 100.

I added the zoom support for org.eclipse.swt.program.Program, can you take a look please?
swt: https://github.com/ptief/eclipse.platform.swt/tree/program_imagedata
ui: https://github.com/ptief/eclipse.platform.ui/tree/externalprogram_scaling_2

@lshanmug
Copy link
Member

For win32 and cocoa org.eclipse.swt.program.Program.getImageData() uses the old method for getting the image data org.eclipse.swt.graphics.Image.getImageData() without zoom, which means it uses zoom value 100.

I added the zoom support for org.eclipse.swt.program.Program, can you take a look please? swt: https://github.com/ptief/eclipse.platform.swt/tree/program_imagedata ui: https://github.com/ptief/eclipse.platform.ui/tree/externalprogram_scaling_2

Creating new API for Program.getImageData(int zoom) looks like the right approach here.
SWT APIs have to be available and consistent across the three platforms - Windows, Linux and Mac, so please add the new API in Program.java in Linux as well. We need to test the behavior on Linux as well with all the changes to make sure it's working as expected.
The SWT change has to be merged first then the Platform UI change.
Can you please create PRs with the changes?

@ptief ptief force-pushed the externalprogram_scaling branch from 661155b to f7bea05 Compare September 26, 2022 11:29
@ptief
Copy link
Contributor Author

ptief commented Sep 26, 2022

I created a PR for SWT and updated this PR with the new API for Program.getImageData(int zoom).

@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023

Test Results

   918 files     918 suites   49m 27s ⏱️
 7 439 tests  7 287 ✅ 152 💤 0 ❌
23 466 runs  22 958 ✅ 508 💤 0 ❌

Results for commit 5fd4ea5.

♻️ This comment has been updated with latest results.

Comment on lines -81 to -85
// The images in GNOME are too big. Scaling them does not give nice result so
// return defaultImage;
if (data.height > defaultImage.height || data.width > defaultImage.width) {
return defaultImage;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I have verified that completely removing this fallback behavior for GNOME does not produce any regressions. My environment uses GNOME version 42.9.
Result: External program images look exactly the same with and without this code on GNOME. Here are example screenshots for 100% and 200% scaling, which are exactly the same with and without that code:
image
image
Since the code has been introduced more than 20 yeras ago, it was probably some problem with GNOME environments back then.

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.

Changes look good and have been tested in combination with eclipse-platform/eclipse.platform.swt#409.
This should be merged right after eclipse-platform/eclipse.platform.swt#409

@HeikoKlare HeikoKlare force-pushed the externalprogram_scaling branch 2 times, most recently from 2178c04 to 67d53d4 Compare January 27, 2024 08:38
@HeikoKlare
Copy link
Contributor

@ptief can you please squash the three commits into a single one and add a short description according to https://github.com/eclipse-platform/.github/blob/main/CONTRIBUTING.md#commit-message-recommendations to the commit message?

@HeikoKlare
Copy link
Contributor

For the record: this will also produce sharp icons on Mac.

Left is current state, right is with this contribution:
image

@ptief ptief force-pushed the externalprogram_scaling branch from 67d53d4 to 229a53e Compare January 29, 2024 10:35
@HeikoKlare HeikoKlare force-pushed the externalprogram_scaling branch from 229a53e to 04cdd39 Compare January 29, 2024 11:15
add support for zoom level of program images
@HeikoKlare HeikoKlare force-pushed the externalprogram_scaling branch from 04cdd39 to 5fd4ea5 Compare January 29, 2024 14:47
@HeikoKlare HeikoKlare merged commit f0b1071 into eclipse-platform:master Jan 29, 2024
16 checks passed
@ptief ptief deleted the externalprogram_scaling branch February 1, 2024 10:06
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.

4 participants