[win32] Apply Operation pattern on GC

[akoch-yatta]

This PR provides a minimal implementation to apply the operation pattern on GC that is used on most other resources to re-create resources on zoom changes. A difference in this implementation is that there is always exactly one OS handle per GC. On zoom change the existing handle is cleaned up a created for the new zoom. There is no use case that requires managing of multiple handles per GC.

This PR does not change any existing behavior. It encapsulated all actions inside the GC into operations. A different PR will utilize this to trigger a re-creation of an existing GC for a different zoom level.

[github-actions]

Test Results

   545 files  + 6     545 suites  +6   27m 9s ⏱️ - 2m 27s
 4 403 tests +37   4 385 ✅ +35   18 💤 +3  0 ❌  - 1 
16 733 runs  +37  16 593 ✅ +35  140 💤 +3  0 ❌  - 1 

Results for commit 5e002fd. ± Comparison against base commit e2cbda9.

♻️ This comment has been updated with latest results.

[amartya4256]

I think we could call it ForCurrentHandle - it sounds more proper in this case, as compared to Resources.
By the way, what handle is it? Handle of image or something else? It seems to be different for different methods
Sometimes the operation applies to the gdipGraphics, lineStyle, foreground, brush, state, etc
Maybe we should just call it currentGCDAta or currentData?

[HeikoKlare]

I have taken a rough look at this and also tested the proposed change. The general concept is fine, it aligns with the other Resource implementations we have.

I did not have time yet to have an in-depth look into this and will unfortunately not be able to do so for the next week (before M1) due to being on vacation. Still, this should go into M1 to have as much testing of it as possible. So consider the concept approved from my side, but it would be nice if @fedejeanne can make an in-depth review of the proposal and then merge the final version for M1.

[fedejeanne]

I started looking into it and made some minor nit-picky changes in 4f1ff0f. I am moving to an in-depth review now and also testing.

[fedejeanne]

The PR looks ok, I have some comments/concerns regarding some non-immutable parameters (Points, Rectangles and arrays) and also one suggestion: if the 2 new fields (originalData and operations) are only needed in a follow-up PR/commit, I would put them there and not in this PR. The reason for that is that reverting the commits should leave the code in a "clean" state and having this commit on its own (imagine the follow-up commit is reverted) inserts 2 fields that only hold data but are never read.

Other than that, I see no issues with this PR and also no regressions

[akoch-yatta]

@fedejeanne Addressed all the comments in a984ff1

[fedejeanne]

Thank you for addressing my comments. I squashed the commits (I had added one yesterday) and rebased on master.

The code looks fine and I saw no regressions when running the Runtime Workspace application, but I also tested using the org.eclipse.swt.examples.graphics.GraphicsExample and I noticed that clicking in some of the examples on the left panel yields this exception:

Exception in thread "main" java.lang.NullPointerException: Cannot invoke "org.eclipse.swt.graphics.Transform.getElements(float[])" because "transform" is null
	at org.eclipse.swt.graphics.GC$SetTransformOperation.<init>(GC.java:5597)
	at org.eclipse.swt.graphics.GC.setTransform(GC.java:5589)
	at org.eclipse.swt.examples.graphics.PathTab.paint(PathTab.java:161)
	at org.eclipse.swt.examples.graphics.GraphicsExample.lambda$2(GraphicsExample.java:228)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4360)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1214)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1238)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1223)
	at org.eclipse.swt.widgets.Composite.WM_PAINT(Composite.java:1636)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:4836)
	at org.eclipse.swt.widgets.Canvas.windowProc(Canvas.java:336)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:5128)
	at org.eclipse.swt.internal.win32.OS.DispatchMessage(Native Method)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3744)
	at org.eclipse.swt.examples.graphics.GraphicsExample.main(GraphicsExample.java:639)

These 2 trigger it:

Could you please look into that?

[akoch-yatta]

Thank you for addressing my comments. I squashed the commits (I had added one yesterday) and rebased on master.

The code looks fine and I saw no regressions when running the Runtime Workspace application, but I also tested using the org.eclipse.swt.examples.graphics.GraphicsExample and I noticed that clicking in some of the examples on the left panel yields this exception:

Exception in thread "main" java.lang.NullPointerException: Cannot invoke "org.eclipse.swt.graphics.Transform.getElements(float[])" because "transform" is null
	at org.eclipse.swt.graphics.GC$SetTransformOperation.<init>(GC.java:5597)
	at org.eclipse.swt.graphics.GC.setTransform(GC.java:5589)
	at org.eclipse.swt.examples.graphics.PathTab.paint(PathTab.java:161)
	at org.eclipse.swt.examples.graphics.GraphicsExample.lambda$2(GraphicsExample.java:228)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4360)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1214)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1238)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1223)
	at org.eclipse.swt.widgets.Composite.WM_PAINT(Composite.java:1636)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:4836)
	at org.eclipse.swt.widgets.Canvas.windowProc(Canvas.java:336)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:5128)
	at org.eclipse.swt.internal.win32.OS.DispatchMessage(Native Method)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3744)
	at org.eclipse.swt.examples.graphics.GraphicsExample.main(GraphicsExample.java:639)

Could you please look into that?

Thanks for the pointer. This was an issue yes. When looking through the full code again, I found two more places, that had a null pointer risk as well. I hope, I found all now.