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

Extend DPIUtil to provide scale up and down methods with target zoom as parameter #1192

Merged

Conversation

akoch-yatta
Copy link
Contributor

For our ongoing work to improve the HiDPI support in the Windows implementation, we are currently working on #131. We need adaptions on DPIUtil to be able to directly pass the target zoom for the up and down scaling utility methods. To provide small PRs for different components, I'd like to contribute our changes on DPIUtil seperately and in advance because they will be needed in all following PRs.
In summary, this contribution adds additional methods to scale different datatypes up or down by passing the target zoom level as parameter. All existing methods will delegate to the added methods and pass DPIUtil::deviceZoom as zoom.

Copy link
Contributor

github-actions bot commented Apr 24, 2024

Test Results

   302 files  + 3     302 suites  +3   6m 16s ⏱️ +39s
 4 121 tests +10   4 113 ✅ +10   8 💤 ±0  0 ❌ ±0 
12 255 runs  +30  12 180 ✅ +30  75 💤 ±0  0 ❌ ±0 

Results for commit 35f1484. ± Comparison against base commit 313757e.

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

LGTM.

One nit-pick: I would have tried to model the tests as parameterized tests and pass the input and the expected output as parameters to improve readability.

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 a rather nitpicky comment: this change replaces most of the calls to getScalingFactor() with calls to getScalingFactor(int), i.e., it almost completely moves the responsibility of processing the proper zoom value to the callers of getScalingFactor(). Would it make sense to replace the last two remaining calls as well and completely remove the (unparameterized) getScalingFactor() method, such that the responsibility for using the proper zoom value is always at the caller side instead of having a mixture?

@akoch-yatta
Copy link
Contributor Author

Also a rather nitpicky comment: this change replaces most of the calls to getScalingFactor() with calls to getScalingFactor(int), i.e., it almost completely moves the responsibility of processing the proper zoom value to the callers of getScalingFactor(). Would it make sense to replace the last two remaining calls as well and completely remove the (unparameterized) getScalingFactor() method, such that the responsibility for using the proper zoom value is always at the caller side instead of having a mixture?

Good point, I just did that and removed the unused getScalingFactor() method

@akoch-yatta
Copy link
Contributor Author

LGTM.

One nit-pick: I would have tried to model the tests as parameterized tests and pass the input and the expected output as parameters to improve readability.

I thought about that as well, but there are some slight differences between some of them, that I didn't see that much advantages in that. Differences are e.g. the float values need the delta in the checks and in some methods I wanna check for assertSame and in some for assertEquals

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.

Thank you! I don't see any risk in these changes, as in the existing methods only the retrieval of the deviceZoom is pulled up from the getScalingFactor() method to its caller and in addition to that only methods are added. The changes themselves are sound.

The reason for adding currently unused methods has been properly explained in the PR description. In order to achieve consistent methods and avoid regressions, reviewing all these changes to DPIUtil together even though the added methods are currently unused is far easier and less risky, which is why merging them makes sense. Once all the adaptations for further HiDPI support are made, we can check whether there are any unused methods that may be cleaned up. But since they not part of public API, this is not an issue.

…as parameter

This contribution adds additional methods to scale different datatypes up or down by passing the target zoom level as parameter. All existing methods will delegate to the added methods and pass DPIUtil::deviceZoom as zoom.

Contributes to eclipse-platform#62
and eclipse-platform#131
@HeikoKlare HeikoKlare merged commit dbaa422 into eclipse-platform:master May 3, 2024
13 checks passed
@HeikoKlare HeikoKlare deleted the add-dpiutil-methods branch May 3, 2024 14:47
@HeikoKlare HeikoKlare restored the add-dpiutil-methods branch May 3, 2024 14:47
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

3 participants