Multi Zoom Support for region for win32 #1278

amartya4256 · 2024-06-12T13:58:41Z

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

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

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

Description

This pull request is based on the implementations of PR #1214. It extends to the native zoom which is provided within widget and propagated to GC. In this PR, the multiple zoom level functionality is attained by using a map to maintain the handle of the region scale as per zoom level. The handle can then be obtained by the method win32_getHandle by passing the zoom information from the client.

contributes to #62 and #127

github-actions · 2024-06-12T14:06:58Z

Test Results

478 files +8 478 suites +8 7m 33s ⏱️ -24s
4 137 tests +2 4 129 ✅ +2 8 💤 ±0 0 ❌ ±0
16 341 runs +5 16 249 ✅ +5 92 💤 ±0 0 ❌ ±0

Results for commit 210e208. ± Comparison against base commit 69af8e1.

♻️ This comment has been updated with latest results.

fedejeanne

The code doesn't compile.

Also, there's a typo.

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

fedejeanne

I tested the Snippet219 and when I move from the 100% monitor to the 200% monitor I see that the region (content) is resized but the shell isn't

100%

200% (ignore the background, it's my wallpaper)

Can you please look into that @amartya4256 ?

laeubi · 2024-06-19T09:19:58Z

ignore the background, it's my wallpaper

In such case I tend to open a text editor to get a white screen for the screenshots :-)

vogella · 2024-06-19T10:38:19Z

I moved the shell not correctly sized example to #62

fedejeanne · 2024-06-19T11:06:19Z

@amartya4256 just to be on the same page: the error I reported was there before this PR.

This behavior is in the current master branch (I switch from the 100% monitor on the left to the 200% monitor on the right):

amartya4256 · 2024-06-20T11:50:59Z

@amartya4256 just to be on the same page: the error I reported was there before this PR.

This behavior is in the current master branch (I switch from the 100% monitor on the left to the 200% monitor on the right):

You can retest now. The problem was: the image in the region was scaled but not the region itself and the scaling of image has already been added to master, you could produce such behaviour.

bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/RegionWin32Tests.java

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

HeikoKlare

I like the proposed way of scaling regions by tracking the operations and applying them for a different scaling on demand rather than scaling a region itself.
However, I now see a duplication of all operations on regions, once implemented in the Region class and once in the Operation class. I would expect that everything related to executing an operation is moved to the Operation class and thus completely removed from the Region class. So I would expect all the add/addInPixels, translate/... methods to only just use the Operation class for applying an operation.

...se.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Shell.java

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

amartya4256 · 2024-06-28T11:17:30Z

@HeikoKlare I have adapted the Region class as per you asked and separated the changes between the 2 commits. Please have a look and let me know if it looks fien to you.

HeikoKlare

Overall, the PR looks quite good, but there are two major points that need to be addressed, one functional and one design error, for which you find more detailed information in the comments.

Functional: operations are not applied to all already created handles
Design: OO violation by implicit dependency between argument types in the Operation class.

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

amartya4256 · 2024-07-02T15:14:01Z

@HeikoKlare I have addressed all the comments and implemented it accordingly. Please have a look again.

HeikoKlare

The redesigned implementation looks really good. I only have nipicky comments left.

I have tested the patch and did not find any regressions when using regions (when starting an SDK product with a single zoom value).

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

HeikoKlare

Thanks for addressing my comments! The changes look good now.

amartya4256 · 2024-07-04T12:00:15Z

Thanks for addressing my comments! The changes look good now.

Thanks, I have rebased the branch to the master as well.

This commit adds the feature of scaling region based on the zoom level of the monitor it is drawn on. The mentioned functionality is attained by using a map to maintain the handle of the region scale as per zoom level. The handle can then be obtained by the method win32_getHandle by passing the zoom information from the client. The region object stores a history of all the operations performed on it so that it can be used to create handles for different zoom level. Additionally, this commit removes the public handle field from Region for win32 since it is now replaced by win32_getHandle method for the clients and internally the class uses a hashMap zoomToHandle to get the handle for the particular zoomlevel. The default handle is stored in zoomToHandle mapped to the initialZoom. contributes eclipse-platform#62 and eclipse-platform#127

HeikoKlare · 2024-07-04T15:43:39Z

I tested the Snippet219 and when I move from the 100% monitor to the 200% monitor I see that the region (content) is resized but the shell isn't

100%

200% (ignore the background, it's my wallpaper)

I have tested the behavior with the current PR state: it works fine and as expected.

Mentioned concerns have been addressed / are no issue with the most recent state.

amartya4256 requested a review from niraj-modi as a code owner June 12, 2024 13:58

fedejeanne requested changes Jun 14, 2024

View reviewed changes

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

amartya4256 force-pushed the multi_zoom_level_region branch 2 times, most recently from 3147c73 to 6c2d898 Compare June 14, 2024 12:58

akoch-yatta reviewed Jun 17, 2024

View reviewed changes

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

amartya4256 force-pushed the multi_zoom_level_region branch 2 times, most recently from b4dcb80 to b12d2ff Compare June 17, 2024 13:38

amartya4256 mentioned this pull request Jun 18, 2024

Implementation of Pattern to enable multi zoom level support for win32 #1265

Merged

1 task

fedejeanne previously requested changes Jun 19, 2024

View reviewed changes

amartya4256 force-pushed the multi_zoom_level_region branch from de513e9 to e9a0e9a Compare June 19, 2024 13:36

amartya4256 force-pushed the multi_zoom_level_region branch 3 times, most recently from 8233b93 to dd6b4fa Compare June 26, 2024 13:41

HeikoKlare mentioned this pull request Jun 26, 2024

Removal of region constructor with handle param #1309

Merged

amartya4256 force-pushed the multi_zoom_level_region branch 2 times, most recently from 091a7c5 to d3e63d8 Compare June 26, 2024 16:14

akoch-yatta reviewed Jun 27, 2024

View reviewed changes

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

amartya4256 force-pushed the multi_zoom_level_region branch from d3e63d8 to db33c96 Compare June 27, 2024 08:44

akoch-yatta reviewed Jun 27, 2024

View reviewed changes

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

amartya4256 force-pushed the multi_zoom_level_region branch 4 times, most recently from 06220b2 to c00441f Compare June 27, 2024 11:47

HeikoKlare requested changes Jun 28, 2024

View reviewed changes

...se.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Shell.java Outdated Show resolved Hide resolved

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

amartya4256 force-pushed the multi_zoom_level_region branch from c00441f to 3849f96 Compare June 28, 2024 09:39

amartya4256 force-pushed the multi_zoom_level_region branch from 3849f96 to 58e85eb Compare June 28, 2024 11:16

HeikoKlare requested changes Jun 28, 2024

View reviewed changes

amartya4256 force-pushed the multi_zoom_level_region branch 3 times, most recently from 9598424 to 52b17bc Compare July 2, 2024 15:11

HeikoKlare reviewed Jul 4, 2024

View reviewed changes

amartya4256 force-pushed the multi_zoom_level_region branch 2 times, most recently from b0a1585 to 78fdeb7 Compare July 4, 2024 10:58

HeikoKlare approved these changes Jul 4, 2024

View reviewed changes

amartya4256 force-pushed the multi_zoom_level_region branch from 78fdeb7 to 6c817c2 Compare July 4, 2024 11:59

HeikoKlare force-pushed the multi_zoom_level_region branch from 6c817c2 to 210e208 Compare July 4, 2024 14:42

HeikoKlare merged commit 80086c9 into eclipse-platform:master Jul 4, 2024
14 checks passed

akoch-yatta mentioned this pull request Jul 5, 2024

Prepare PR for adaptions on Region in the context of multiple zoom levels vi-eclipse/Eclipse-Platform#44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi Zoom Support for region for win32 #1278

Multi Zoom Support for region for win32 #1278

amartya4256 commented Jun 12, 2024

github-actions bot commented Jun 12, 2024 •

edited

Loading

fedejeanne left a comment

fedejeanne left a comment

laeubi commented Jun 19, 2024

vogella commented Jun 19, 2024

fedejeanne commented Jun 19, 2024

amartya4256 commented Jun 20, 2024 •

edited

Loading

HeikoKlare left a comment

amartya4256 commented Jun 28, 2024

HeikoKlare left a comment

amartya4256 commented Jul 2, 2024

HeikoKlare left a comment

HeikoKlare left a comment

amartya4256 commented Jul 4, 2024

HeikoKlare commented Jul 4, 2024

Multi Zoom Support for region for win32 #1278

Multi Zoom Support for region for win32 #1278

Conversation

amartya4256 commented Jun 12, 2024

Addressed issues

Requires

Description

github-actions bot commented Jun 12, 2024 • edited Loading

Test Results

fedejeanne left a comment

Choose a reason for hiding this comment

fedejeanne left a comment

Choose a reason for hiding this comment

laeubi commented Jun 19, 2024

vogella commented Jun 19, 2024

fedejeanne commented Jun 19, 2024

amartya4256 commented Jun 20, 2024 • edited Loading

HeikoKlare left a comment

Choose a reason for hiding this comment

amartya4256 commented Jun 28, 2024

HeikoKlare left a comment

Choose a reason for hiding this comment

amartya4256 commented Jul 2, 2024

HeikoKlare left a comment

Choose a reason for hiding this comment

HeikoKlare left a comment

Choose a reason for hiding this comment

amartya4256 commented Jul 4, 2024

HeikoKlare commented Jul 4, 2024

github-actions bot commented Jun 12, 2024 •

edited

Loading

amartya4256 commented Jun 20, 2024 •

edited

Loading