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

Store nativeDeviceZoom in widgets instead of zoom for win32 #1229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amartya4256
Copy link

@amartya4256 amartya4256 commented May 14, 2024

Addressed issues

Unlocks

Description

This contribution is to provide the nativeDeviceZoom to all the widgets so that it can be used later on for e.g. font scaling and more. Currently, the nativeDeviceZoom is only available via the shell, which is not sufficient later when there is no shell available, e.g. GC.

In this following PR #1214 , we had discussed that instead of storing zoom for widgets, we should store nativeDeviceZoom, to make it more accessible where shell is not present. Because in some cases, there was no nativeDeviceZoom present because of the previous implementation and we discussed if a fallback might be necessary. This implementation makes sure that in those scenarios, nativeDeviceZoom would always be available for further usage.

The consumers can now have deviceZoom wherever necessary by using the utility method to translate nativeDeviceZoom to deviceZoom, which is available in DPIUtil.

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.

In general, the changes look sound and storing the native zoom for which the actually used value can always be calculated on demand absolutely makes sense.

Currently, there are several naming and consistency issues, as identifiers have not been renamed, Javadoc has not been adapted etc.
Please have a look that all the identifiers (variable, field, method names) still properly capture their meaning.

Copy link
Contributor

github-actions bot commented May 15, 2024

Test Results

   418 files  ±0     418 suites  ±0   7m 8s ⏱️ -19s
 4 121 tests ±0   4 113 ✅ ±0   8 💤 ±0  0 ❌ ±0 
16 313 runs  ±0  16 221 ✅ ±0  92 💤 ±0  0 ❌ ±0 

Results for commit 600418a. ± Comparison against base commit d3df796.

♻️ 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.

Changes look good, I only have one question and one minor remark for the method Image::setCurrentNativeZoom

Also, just for my understanding (recap of "the big picture"), this PR "merely":

  • Replaces the previously cached device zoom in Image (currentDeviceZoom) by a calculated zoom (Image::getZoom() --> DPIUtil::getZoomForAutoscaleProperty(...))
  • Propagates the native device zoom instead of the (current) device zoom in GC
  • Adapts Image::handleDPIChange(int) to use the newly propagated native device zoom

Did I miss anything?

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.

Thanks for updating the PR. I only have minor comments left.

Note that my review is currently based on GitHub UI, as I don't have a Windows laptop available. So I will not "formally" approve the PR as I would like to validate the changes in Eclipse for that.

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.

Thank you for addressing my previous comment @amartya4256 , I just have one final change request to improve readability. After that, this PR is good to go from my side.

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.

In general, this PR looks good and sound to me. I only have some nitpicky comments regarding naming and one regarding visibilities.

Edit: and of course please address the remaining comments by @fedejeanne

@HeikoKlare
Copy link
Contributor

@amartya4256 I don't see my previous comment (#1229 (comment)) addressed yet. Could you please check before I re-review the PR?

This contribution is to provide the nativeDeviceZoom to all the widgets
so that it can be used later on for e.g. font scaling and more.
Currently, the nativeDeviceZoom is only available via the shell, which
is not sufficient later when there is no shell available, e.g. GC.

Contributes to eclipse-platform#62 and eclipse-platform#127
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.

To me, this looks good now. Thank you!

@amartya4256
Copy link
Author

amartya4256 commented May 29, 2024

Thank you for addressing my previous comment @amartya4256 , I just have one final change request to improve readability. After that, this PR is good to go from my side.

@fedejeanne can you approve the changes? :)

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.

None yet

4 participants