Implementation of GC to enable multi zoom level support for win32 #1214

amartya4256 · 2024-05-06T13:06:27Z

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

Store nativeDeviceZoom in widgets instead of zoom for win32 #1229

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

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 · 2024-05-07T13:46:42Z

Test Results

418 files ±0 418 suites ±0 7m 27s ⏱️ -53s
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 ddd2019. ± Comparison against base commit 2ff635c.

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

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Shell.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Widget.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GCData.java

laeubi · 2024-05-08T10:01:06Z

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 · 2024-05-10T07:21:09Z

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 · 2024-05-10T08:07:34Z

@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

tests/org.eclipse.swt.tests.win32/JUnit Tests/org/eclipse/swt/graphics/GCWin32Tests.java

HeikoKlare · 2024-05-12T17:35:16Z

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.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GCData.java

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.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GCData.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GCData.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java

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?

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Widget.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Shell.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Control.java

amartya4256 · 2024-05-15T14:13:10Z

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.

tests/org.eclipse.swt.tests.win32/JUnit Tests/org/eclipse/swt/graphics/GCWin32Tests.java

fedejeanne · 2024-05-16T11:46:40Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java

@@ -3887,6 +3909,7 @@ long identity() {

 void init(Drawable drawable, GCData data, long hDC) {
 int foreground = data.foreground;
+ this.data = data;


Why move it up?

My change requests addressed

HeikoKlare

In general, these changes now look fine to me. With the preparation in #1229, my recent concerns have been addressed.
I have some remaining comments on naming and test methodology, all of them being minor.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GCData.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Widget.java

tests/org.eclipse.swt.tests.win32/JUnit Tests/org/eclipse/swt/graphics/GCWin32Tests.java

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

This commit contributes to multi zoom level support of GC which can be extended by other resources and widgets. Contributes to eclipse-platform#62 and eclipse-platform#131

HeikoKlare · 2024-05-29T14:44:34Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java

@@ -3886,6 +3908,7 @@ long identity() {
 }

 void init(Drawable drawable, GCData data, long hDC) {
+ this.data = data; // Need to set GCData first because it is needed for getDeviceZoom calls


In which line(s) will these getDeviceZoom() calls happen? It feels kind of dangerous to set the field before it's actual values are properly filled, as it's unclear when you can assume which property to have the expected value.

HeikoKlare · 2024-05-29T14:50:02Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java

@@ -4582,8 +4618,9 @@ void setLineAttributesInPixels (LineAttributes attributes) {
 }
 if (changed) {
 float[] newDashes = new float[dashes.length];
+ int deviceZoom = getDeviceZoom();


Why is it necessary/beneficial to store the result of getDeviceZoom() in a local variable? At many other places this value is used directly.

HeikoKlare · 2024-05-29T14:54:59Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java

@@ -5120,4 +5158,12 @@ private static int sin(int angle, int length) {
 return (int)(Math.sin(angle * (Math.PI/180)) * length);
 }

+private int getNativeDeviceZoom() {
+ return data.nativeZoom != 0 ? data.nativeZoom : DPIUtil.getNativeDeviceZoom();


I don't understand the intended invariant for nativeZoom. In the init() method, the value is extracted from the handle if nativeZoom would be zero otherwise, which suggests that the value should never be zero. Here, the assumption is that nativeZoom might be zero and implements another fallback functionality for a zero nativeZoom, namely using the general, potentially wrong native zoom from DPIUtil. I would expect a that only one of them is necessary and implemented, implying a proper invariant for when nativeZoom is expected to (not) be zero.

amartya4256 requested review from niraj-modi and iloveeclipse as code owners May 6, 2024 13:06

iloveeclipse removed their request for review May 7, 2024 14:02

fedejeanne previously requested changes May 8, 2024

View reviewed changes

amartya4256 force-pushed the multi_zoom_level_GC branch from 3a3458a to 18b420d Compare May 8, 2024 09:54

fedejeanne requested changes May 10, 2024

View reviewed changes

HeikoKlare reviewed May 13, 2024

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GCData.java Outdated Show resolved Hide resolved

amartya4256 force-pushed the multi_zoom_level_GC branch 2 times, most recently from 27f3996 to 40f7c63 Compare May 13, 2024 16:17

amartya4256 requested review from HeikoKlare and fedejeanne May 13, 2024 16:29

amartya4256 force-pushed the multi_zoom_level_GC branch from 40f7c63 to 90e57ec Compare May 14, 2024 07:20

HeikoKlare requested changes May 14, 2024

View reviewed changes

amartya4256 mentioned this pull request May 14, 2024

Store nativeDeviceZoom in widgets instead of zoom for win32 #1229

Open

fedejeanne previously requested changes May 15, 2024

View reviewed changes

amartya4256 force-pushed the multi_zoom_level_GC branch from 90e57ec to ffac213 Compare May 15, 2024 14:11

amartya4256 force-pushed the multi_zoom_level_GC branch 2 times, most recently from 6d41ade to ddd2019 Compare May 15, 2024 14:18

fedejeanne reviewed May 16, 2024

View reviewed changes

tests/org.eclipse.swt.tests.win32/JUnit Tests/org/eclipse/swt/graphics/GCWin32Tests.java Outdated Show resolved Hide resolved

fedejeanne reviewed May 16, 2024

View reviewed changes

amartya4256 force-pushed the multi_zoom_level_GC branch from ddd2019 to 67e2821 Compare May 17, 2024 11:30

amartya4256 mentioned this pull request May 17, 2024

Implementation of Image to enable Multi Zoom Level Support for Win32 #1236

Open

1 task

amartya4256 mentioned this pull request May 21, 2024

Implementation of FontMetrics to enable multi zoom level support for win32 #1243

Open

1 task

amartya4256 force-pushed the multi_zoom_level_GC branch 2 times, most recently from edffc3c to 6d4785d Compare May 21, 2024 15:25

HeikoKlare reviewed May 22, 2024

View reviewed changes

amartya4256 force-pushed the multi_zoom_level_GC branch from 6d4785d to 4945f06 Compare May 22, 2024 09:17

amartya4256 mentioned this pull request May 22, 2024

Implementation of Subclasses of Widgets to enable multi zoom level support for win32 #1247

Open

1 task

amartya4256 force-pushed the multi_zoom_level_GC branch 3 times, most recently from af9ada0 to 7adc15f Compare May 23, 2024 14:29

amartya4256 requested review from HeikoKlare, fedejeanne and akoch-yatta May 24, 2024 09:01

amartya4256 force-pushed the multi_zoom_level_GC branch 2 times, most recently from a4a2c67 to 34ba421 Compare May 27, 2024 12:31

amartya4256 added 2 commits May 29, 2024 11:16

Multi zoom level support of GC for win32

8b87c03

This commit contributes to multi zoom level support of GC which can be extended by other resources and widgets. Contributes to eclipse-platform#62 and eclipse-platform#131

amartya4256 force-pushed the multi_zoom_level_GC branch from 34ba421 to 8b87c03 Compare May 29, 2024 11:41

HeikoKlare reviewed May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of GC to enable multi zoom level support for win32 #1214

Implementation of GC to enable multi zoom level support for win32 #1214

amartya4256 commented May 6, 2024 •

edited by fedejeanne

github-actions bot commented May 7, 2024 •

edited

fedejeanne left a comment

laeubi commented May 8, 2024

fedejeanne commented May 10, 2024

laeubi commented May 10, 2024 •

edited

fedejeanne left a comment

HeikoKlare commented May 12, 2024

HeikoKlare left a comment

fedejeanne left a comment

amartya4256 commented May 15, 2024

fedejeanne May 16, 2024

amartya4256 May 23, 2024

HeikoKlare left a comment

HeikoKlare May 29, 2024

HeikoKlare May 29, 2024

HeikoKlare May 29, 2024

Implementation of GC to enable multi zoom level support for win32 #1214

Are you sure you want to change the base?

Implementation of GC to enable multi zoom level support for win32 #1214

Conversation

amartya4256 commented May 6, 2024 • edited by fedejeanne

Addressed issues

Requires

Unlocks

Description

github-actions bot commented May 7, 2024 • edited

Test Results

fedejeanne left a comment

Choose a reason for hiding this comment

laeubi commented May 8, 2024

fedejeanne commented May 10, 2024

laeubi commented May 10, 2024 • edited

fedejeanne left a comment

Choose a reason for hiding this comment

HeikoKlare commented May 12, 2024

HeikoKlare left a comment

Choose a reason for hiding this comment

fedejeanne left a comment

Choose a reason for hiding this comment

amartya4256 commented May 15, 2024

fedejeanne May 16, 2024

Choose a reason for hiding this comment

amartya4256 May 23, 2024

Choose a reason for hiding this comment

HeikoKlare left a comment

Choose a reason for hiding this comment

HeikoKlare May 29, 2024

Choose a reason for hiding this comment

HeikoKlare May 29, 2024

Choose a reason for hiding this comment

HeikoKlare May 29, 2024

Choose a reason for hiding this comment

amartya4256 commented May 6, 2024 •

edited by fedejeanne

github-actions bot commented May 7, 2024 •

edited

laeubi commented May 10, 2024 •

edited