Implementation of GC to enable multi zoom level support for win32

[amartya4256]

Addressed issues

• HiDPI improvements in SWT with single and multiple monitor setup. #62
• [Multi-Monitor] SWT applications to support and work with different zoom values per Shell. #131

Requires

• added native bindings for win32 #1258

Note: Only the last commit in this PR is to be reviewed. Previous commit(s) belong to the prerequisite PR(s)

Unlocks

• Implementation of Image to enable Multi Zoom Level Support for Win32 #1236
• Implementation of Subclasses of Widgets to enable multi zoom level support for win32 #1247
• Implementation of FontMetrics to enable multi zoom level support for win32 #1243

Description

This pull request is a base pull request for several following PRs which implement multi zoom level support for different components (resources, widgets, etc).

In this PR, the focus is on GC to enable it to use the zoom level information which is added to widget. Also, it provides zoom level information in GCData which can later be used by resources like Image, Region, etc which don't have the zoom information since they are not a widget. Also, GC now uses the DPIUtil methods with zoom for getting the size as per the zoom level. This zoom in GC is derived from the GCData where we add the zoom information.

[github-actions]

Test Results

   418 files  ±0     418 suites  ±0   9m 15s ⏱️ +27s
 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 f23c721. ± Comparison against base commit 567a8b4.

♻️ This comment has been updated with latest results.

[fedejeanne]

Hi @amartya4256 , thank you for your contribution.

I noticed some problems with the visibility of some fields (the most serious one is allowing to set the deviceZoom and nativeDeviceZoom separately.

Could you please look into it?
Thank you.

[laeubi]

I just wanted to note that (platform dependent) package private or even public fields / methods are quite common, they should just be documented in this case as platform dependent.

[fedejeanne]

I just wanted to note that (platform dependent) package private or even public fields / methods are quite common, they should just be documented in this case as platform dependent.

@laeubi Is it really necessary to specify that a package private field/method inside a fragment is platform-dependent? I mean the fragment is already platform-dependent and the field/method can't be seen outside the fragment. Or am I missing something?

My requests were addressed.

[laeubi]

@fedejeanne I have only seen it on public field, so maybe in my sentence it should read as an XOR so:

Either the field is package private or instead public but clearly documented to be platform dependent.

:-)

[fedejeanne]

The tests are broken

[HeikoKlare]

The tests are broken

Note that new test classes in a test plug-in usually need to be added to the plug-ins test suite. In the win32 SWT tests, this is the AllWin32Tests test suite. If you do so for the new GCWin32Tests, the PR build will fail on Windows because of the test failures mentioned by @fedejeanne.

If you cannot provide reasonable, sufficient tests via public API only, you need to add the test cases in the win32 fragments, as prepared for the DefaultSWTFontRegistry and ScalingSWTFontRegistry in #1203.

[HeikoKlare]

I have some remarks and questions regarding the current design for which I need some clarification before I can approve this. The essential problem I have here is with respect to responsibilities. My understanding is that in the proposed implementation Controls are reponsible for updating zoom data used by a GC indirectly via changing values in GCData. This leads to visibility problems, duplicated logic for setting these values across package borders etc.. Since these values only seem to be a cache of information provided by the underlying Drawable of a GC, I do not understand why (1) this caching is necessary and (2) why GC cannot simply listen for zoom change events at the Control and then update the data itself but requires the Control to take care of it.
And even if the information really have to be provided to GCData (e.g., because they will be required by other Resources, as mentioned in the PR description), I would still expect the GC to be responsible for updating them and not the Controls. Then you should also get rid of the current visibility problems.

Please also extend the commit description with information about

• why these changes are necessary, and
• what they do,

so that people are able to understand the changes only by the commit, even without the PR.

[fedejeanne]

It seems that some fields added here will not be necessary in future PRs (see Shell and Widget), please remove them.

Also, the method GCData::setNativeZoomForGCData suffers from a code smell, it would be nice if it's not necessary or if it can be done differently.

Have the tests been addressed yet?

[amartya4256]

It seems that some fields added here will not be necessary in future PRs (see Shell and Widget), please remove them.

Also, the method GCData::setNativeZoomForGCData suffers from a code smell, it would be nice if it's not necessary or if it can be done differently.

Have the tests been addressed yet?

I updated this PR with respect to the PR #1229. It should look more consistent now.

My change requests addressed

[HeikoKlare]

The changes look good now. Only the "actual" commit of this PR seems to contain the changes that have been extracted into #1258. Could you please rebase this PR onto its dependencies (i.e., the PRs that need to be merged before)?

Removing my request for changes as concerns have been addressed, but cannot give approval because commit trace has to be adapted.

[HeikoKlare]

@amartya4256 merging the required PR introduced a conflict in this PR, so please resolve and rebase on master once you find the time

[amartya4256]

@amartya4256 merging the required PR introduced a conflict in this PR, so please resolve and rebase on master once you find the time

This PR now depends on #1258 because of splitting the OS bindings to a separate PR. I have rebased it to the PR and I would update the description.

[HeikoKlare]

The changes themselves still look good.
@amartya4256 Could you please improve the commit message with some more details on how GC is changed to enhance multi-zoom support?

[fedejeanne]

LGTM. I'm going to test it tomorrow and merge it.