Skip to content

Conversation

@akoch-yatta
Copy link
Contributor

@akoch-yatta akoch-yatta commented Feb 25, 2025

This PR refactors Transform in the win32 implementation to better support multiple handles for different zoom settings of a Transform when monitor-specific scaling is enabled. The previous implementation only applied adaptions to the initial handle of the transform and relied on the transform not to be changed afterwards. This doesn't cover all scenarios and can lead to unexpected behavior when re-using Transform objects over different zoom settings.

How to test

You can test this adaption with the new "Transform Re-usage"-Tab in the GraphicsExample. You need to:

  1. Set up at least two monitors with different zoom, e.g. 100% and 200%
  2. Start the GraphicsExample with additonal VM arguments:
-Dswt.autoScale.updateOnRuntime=true
-Dswt.autoScale=quarter
  1. Move to the "Transform Re-usage"-Tab, play around with the buttons and switch monitors in between. Without the changes in Transform you will loose operations or don't see some off them on the monitors the Transform was not created on.

@akoch-yatta akoch-yatta force-pushed the win32-refactor-transform branch from 8be37d6 to d0d5699 Compare February 25, 2025 15:47
storeAndApplyOperationForAllHandles(new TranslateOperation(offsetX, offsetY));
}

private class TransformHandle {
Copy link
Contributor

@laeubi laeubi Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private class TransformHandle {
private static final class TransformHandle {

or use a record

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, used a record here

@github-actions
Copy link
Contributor

github-actions bot commented Feb 25, 2025

Test Results

   506 files  + 4     506 suites  +4   9m 43s ⏱️ +57s
 4 338 tests + 4   4 324 ✅ + 4   14 💤 ±0  0 ❌ ±0 
16 591 runs  +16  16 482 ✅ +16  109 💤 ±0  0 ❌ ±0 

Results for commit 822860e. ± Comparison against base commit 4fecda7.

♻️ This comment has been updated with latest results.

@akoch-yatta akoch-yatta force-pushed the win32-refactor-transform branch from d0d5699 to 8b770c3 Compare February 25, 2025 16:11
@akoch-yatta akoch-yatta force-pushed the win32-refactor-transform branch 2 times, most recently from e357142 to 2177652 Compare February 26, 2025 14:43
@akoch-yatta akoch-yatta changed the title [win32] Dynamic handle creation for transform [win32] Use operations for transform manipulation Feb 26, 2025
}

long getHandle(int zoomLevel) {
return getTransformHandle(zoomLevel).handle;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be removed and getTransformHandle(zoomLevel).handle can be used everywhere instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this only be possible if we expose the TransformHandle class?
Hmm, thinking about it I probably would not like to do that. TransformHandle is intended as an internal class to simplify the management of different OS handles. From external calls only the OS handle is relevant

}
}

private interface Operation {
Copy link
Contributor

@amartya4256 amartya4256 Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought. Since we will use Operation for all the resource, does it make sense to extract this as a FunctionalInterface in a separate file and use it everywhere? I can see similar usage in Path and Region as well.

Copy link
Contributor

@amartya4256 amartya4256 Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For records like TransformHandle, RegionHandle, we can have a ResourceHandle instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about the same. But I am hesitant to introduce some external class (even if package protected or internal package), because for now it is only used in win32.

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.

Change looks overall good, thanks! Only minor comments from my side.

@akoch-yatta akoch-yatta force-pushed the win32-refactor-transform branch 2 times, most recently from 1641e2e to 54ca587 Compare February 28, 2025 16:07
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.

For me, the changes are fine now. I have tested them with the GraphicsExample. I have also executed GEF Classic's Draw2d tests against the changes as an additional means of validation: it's SWTGraphics also makes use of SWT's Transform.

@HeikoKlare HeikoKlare force-pushed the win32-refactor-transform branch from 54ca587 to 1766b59 Compare March 6, 2025 09:38
@akoch-yatta akoch-yatta force-pushed the win32-refactor-transform branch from e84e7a1 to 7c10c23 Compare March 6, 2025 12:41
This commit refactors Transform in the win32 implementation to better
support multiple handles for different zoom settings of a Transform when
monitor-specific scaling is enabled. The previous implementation
only applied adaptions to the initial handle of the transform and relied
on the transform not to be changed afterwards. This doesn't cover all
scenarios and can lead to unexpected behavior when re-using Transform
objects over different zoom settings.
@akoch-yatta akoch-yatta force-pushed the win32-refactor-transform branch from 7c10c23 to e681af1 Compare March 6, 2025 12:47
@HeikoKlare HeikoKlare force-pushed the win32-refactor-transform branch from e681af1 to cdca916 Compare March 6, 2025 12:58
@HeikoKlare HeikoKlare force-pushed the win32-refactor-transform branch from cdca916 to 822860e Compare March 6, 2025 13:04
@HeikoKlare HeikoKlare merged commit ccc69c2 into eclipse-platform:master Mar 6, 2025
14 checks passed
@HeikoKlare HeikoKlare deleted the win32-refactor-transform branch March 6, 2025 13:19
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.

Refactor Transform to use Operations for all manipulations

4 participants