[win32] Refactor path to support multiple handles #1856

akoch-yatta · 2025-02-25T10:42:05Z

This PR refactors Path in the win32 implementation to better support multiple handles for different zoom settings of a Path when monitor-specific scaling is enables. The previous implementation only applied adaptions on the path to the initial handle and relied on the path not to be changed after usage. This doesn't cover all scenarios and can lead to incompletely rendered path when reusing Path object over different zoom settings.

How to test

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

Set up at least two monitors with different zoom, e.g. 100% and 200%
Start the GraphicsExample with additonal VM arguments:

-Dswt.autoScale.updateOnRuntime=true
-Dswt.autoScale=quarter

Move to the "Path Re-usage"-Tab, play around with the buttons and switch monitors in between. Without the changes in Path you will loose operations or don't see some off them on the monitors the Path was not created on.

github-actions · 2025-02-25T10:48:52Z

Test Results

505 files ±0 505 suites ±0 8m 7s ⏱️ -27s
4 303 tests +1 4 291 ✅ +1 11 💤 ±0 1 ❌ ±0
16 559 runs +1 16 452 ✅ +1 106 💤 ±0 1 ❌ ±0

For more details on these failures, see this check.

Results for commit b0e34fa. ± Comparison against base commit 523a5d4.

♻️ This comment has been updated with latest results.

amartya4256 · 2025-03-06T13:14:22Z

Design-wise, looks good to me. I would approve it. @HeikoKlare I would request you to test it out as well.

HeikoKlare

Most of the changes look fine to me (see my comments for exception).
I am just wondering what the introduced operations are used for in Path: if I am not mistaken, the stored operations are not used anywhere. Each operation is directly applied to all present handles for all zooms and in case a new handle for a new zoom is created, the existing logic via passing the PathData is used.

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

HeikoKlare · 2025-03-06T14:00:30Z

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

-	Gdip.GraphicsPath_AddPath(getHandle(initialZoom), path.getHandle(initialZoom), false);
-	currentPoint.X = path.currentPoint.X;
-	currentPoint.Y = path.currentPoint.Y;
+	this.operations.addAll(path.operations);


Is it correct to remove the original code here?

Almost. I just missed to apply the new operation to add to the existing handles

I see. We need to be aware that within the overall refactoring this is kind of a behavioral change. Of course the result should be the same, but maybe using Gdip to add a path based on its handle and re-applying its original operations may result in (slightly) different result (for whatever reason). Still, I understand and agree that this is the right thing to do.

We could do something different. More or less:

Cache the old operations

Recreate a path on demand

Use Gdip.GraphicsPath_AddPath(getHandle(initialZoom), newTempHandle, false);

This would be more or less the same as before, but a bit more complex to understand, I would say

I am not sure whether we should do that since actually the new operation does exactly what you would expect it to do. I just wanted to mentioned that we might oversee something that eventually leads to derivations of behavior, so that we are aware this change. But I don't see why this might be anything else than minor changes in result because of limited precision of float values and different ways of applying the operations.

But I just checked out the tab you added to the GraphicsExample, which shows that the old and new implementation of addPath behave different. So we may need to revise this.

Oh damn, you are right. There is a connecting path if you have done something before. That could probably be emulated be an additional move operation, but I would probably favor my approach above more

Sorry for that back and forth. So for this PR I just moved the original logic into an Operation, because in this PR all operations are always applied immediately and no handles are created dynamically yet. I will need to adapt this new opearation in the follow-up PR

That's perfectly fine. It's just that we now have the problem of storing a Path that might be disposed when querying the handle for the encapsulating Path at another zoom level. So we probably need to clone the path to be stored?

No, in this PR the new Path is created on its path data and then scaled. The operations are always applied immediately. But I added a test to ensure it doesn't break later

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

akoch-yatta · 2025-03-06T15:15:14Z

Most of the changes look fine to me (see my comments for exception). I am just wondering what the introduced operations are used for in Path: if I am not mistaken, the stored operations are not used anywhere. Each operation is directly applied to all present handles for all zooms and in case a new handle for a new zoom is created, the existing logic via passing the PathData is used.

They have a use now. The are used in Path::addPath. Without that, we would have needed them only in #1869, because there the handles are completly generated on demand using the operations

HeikoKlare

See comment :

It's just that we now have the problem of storing a Path that might be disposed when querying the handle for the encapsulating Path at another zoom level. So we probably need to clone the path to be stored?

akoch-yatta · 2025-03-07T10:56:12Z

See comment :

It's just that we now have the problem of storing a Path that might be disposed when querying the handle for the encapsulating Path at another zoom level. So we probably need to clone the path to be stored?

Just answered above at the same time 😄 It is not an issue in this PR. I will need to address that in the next one

HeikoKlare

I see. I missed that the operations are not stored (anymore) in this PR. Thanks for the clarification!

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

HeikoKlare · 2025-03-07T13:31:07Z

Failing test is unrelated and documented: #1843

akoch-yatta force-pushed the win32-refactor-path branch 5 times, most recently from 67ea63a to f49bb0c Compare February 25, 2025 13:31

akoch-yatta linked an issue Feb 25, 2025 that may be closed by this pull request

Refactor Path to support creation handles on demand vi-eclipse/Eclipse-Platform#249

Closed

akoch-yatta force-pushed the win32-refactor-path branch 3 times, most recently from 134325e to 85b12bc Compare February 25, 2025 15:44

akoch-yatta mentioned this pull request Feb 25, 2025

[win32] Adapt Region to support creation of handles with operations with disposed resources #1859

Merged

akoch-yatta force-pushed the win32-refactor-path branch 6 times, most recently from ce4beae to f2e3e81 Compare February 28, 2025 18:06

akoch-yatta mentioned this pull request Feb 28, 2025

[win32] Dynamic handle creation for Path #1869

Merged

akoch-yatta linked an issue Feb 28, 2025 that may be closed by this pull request

Refactor Path to use Operations for all manipulations vi-eclipse/Eclipse-Platform#255

Closed

This was unlinked from issues Feb 28, 2025

Refactor Path to support creation handles on demand vi-eclipse/Eclipse-Platform#249

Closed

Refactor Path to use Operations for all manipulations vi-eclipse/Eclipse-Platform#255

Closed

akoch-yatta linked an issue Mar 6, 2025 that may be closed by this pull request

Refactor Path to use Operations for all manipulations vi-eclipse/Eclipse-Platform#255

Closed

amartya4256 approved these changes Mar 6, 2025

View reviewed changes

HeikoKlare force-pushed the win32-refactor-path branch from f2e3e81 to f3eb0e3 Compare March 6, 2025 13:36

HeikoKlare reviewed Mar 6, 2025

View reviewed changes

akoch-yatta force-pushed the win32-refactor-path branch from f3eb0e3 to 3fffd59 Compare March 6, 2025 15:09

akoch-yatta requested review from amartya4256 and HeikoKlare March 6, 2025 15:15

HeikoKlare force-pushed the win32-refactor-path branch from 3fffd59 to 9233c3c Compare March 6, 2025 15:33

akoch-yatta force-pushed the win32-refactor-path branch 2 times, most recently from b9ab37f to 82e393b Compare March 7, 2025 10:54

HeikoKlare requested changes Mar 7, 2025

View reviewed changes

HeikoKlare approved these changes Mar 7, 2025

View reviewed changes

HeikoKlare force-pushed the win32-refactor-path branch from 82e393b to b0e34fa Compare March 7, 2025 11:14

HeikoKlare merged commit 29bae82 into eclipse-platform:master Mar 7, 2025
12 of 14 checks passed

HeikoKlare deleted the win32-refactor-path branch March 7, 2025 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[win32] Refactor path to support multiple handles #1856

[win32] Refactor path to support multiple handles #1856

akoch-yatta commented Feb 25, 2025

github-actions bot commented Feb 25, 2025 •

edited

Loading

amartya4256 commented Mar 6, 2025 •

edited

Loading

HeikoKlare left a comment

HeikoKlare Mar 6, 2025

akoch-yatta Mar 6, 2025

HeikoKlare Mar 6, 2025

akoch-yatta Mar 6, 2025 •

edited

Loading

HeikoKlare Mar 6, 2025

akoch-yatta Mar 6, 2025

akoch-yatta Mar 6, 2025

HeikoKlare Mar 6, 2025

akoch-yatta Mar 7, 2025

akoch-yatta commented Mar 6, 2025

HeikoKlare left a comment

akoch-yatta commented Mar 7, 2025 •

edited

Loading

HeikoKlare left a comment

HeikoKlare commented Mar 7, 2025

[win32] Refactor path to support multiple handles #1856

[win32] Refactor path to support multiple handles #1856

Conversation

akoch-yatta commented Feb 25, 2025

How to test

github-actions bot commented Feb 25, 2025 • edited Loading

Test Results

amartya4256 commented Mar 6, 2025 • edited Loading

HeikoKlare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akoch-yatta Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akoch-yatta commented Mar 6, 2025

HeikoKlare left a comment

Choose a reason for hiding this comment

akoch-yatta commented Mar 7, 2025 • edited Loading

HeikoKlare left a comment

Choose a reason for hiding this comment

HeikoKlare commented Mar 7, 2025

github-actions bot commented Feb 25, 2025 •

edited

Loading

amartya4256 commented Mar 6, 2025 •

edited

Loading

akoch-yatta Mar 6, 2025 •

edited

Loading

akoch-yatta commented Mar 7, 2025 •

edited

Loading