[Win32] Represent GC#getClipping(Region) operation in Region #2346 #2348
Conversation
HeikoKlare
force-pushed
the
issue-2346
branch
2 times, most recently
from
2c12ddf to
b587241
Compare
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Region.java
Show resolved
Hide resolved
|
I have tested the behaviour and it seems to fix the regression by storing getClipping operation in gc and calling the getClippingRegion on opertaion apply, and also registering as a set operation inside the region which references the gc getClipping operation object's zoomToHandleStorage to get the manipulated handle as per the operation. However, I have added a few comments here and there. |
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java
Outdated
Show resolved
Hide resolved
| if (!zoomToRegionHandle.containsKey(zoom)) { | ||
| System.err.println("No clipping handle for zoom " + zoom + " has been created on this GC"); | ||
| return zoomToRegionHandle.values().iterator().next(); |
There was a problem hiding this comment.
I tried going through it just have a question, could this lead to visual inconsistencies if the returned handle doesn't match the requested zoom? Since iterator().next() returns an arbitrary value for the region when GetClippingOperation has not been run for this zoom
There was a problem hiding this comment.
Yes, it can. In such a situation we cannot create a proper handle (i.e., one at the correct zoom), which is why the error message is printed. This situation should actually not occur at all, as it would not make any sense to use the region in any other zoom context than in the context of the GC from which the clipping is applied.
However, I just found that this leads to issues when some operation is executed on a region that has no handle yet (e.g., for only retrieving its bounds). Then a temporary handle for an arbitrary zoom (more precisely the device zoom) is used. If that device zoom does not fit to the GC context, we run into the error case. I currently try to store a zoom hint inside region so that a temporary handle is created at a proper zoom if such a hint is given. I will update the PR soon.
There was a problem hiding this comment.
This situation should actually not occur at all, as it would not make any sense to use the region in any other zoom context than in the context of the GC from which the clipping is applied. However, I just found that this leads to issues when some operation is executed on a region that has no handle yet (e.g., for only retrieving its bounds).
I get this error in my console when I move the search window across monitors.
There was a problem hiding this comment.
I get this error in my console when I move the search window across monitors.
Yes, exactly. You may test again with the recent change, which should resolve that issue.
There was a problem hiding this comment.
Tested the latest changes the issue is resolved
…platform#2346 The GC#getClipping(Region) call sets the passed region to the current clipping region of the GC. Currently, this is not represented as an operation inside the passed Region, such that when calculating a handle for a different zoom of that region or when copying that region the according application of GC#getClipping(Region) to that region is missing. This change adds the missing operation to the Region. Since the clipping needs to be calculated at the of executing the GC#getClipping(Region) method and not at the time of applying the region operation when retrieving a handle, the GC#getClipping(Region) stores an operation inside GC that maintains handles for the according clipping region at the time of executing the GC#getClipping(Region) method for every zoom at which the GC is requested. This ensures that the proper clipping state is available to the Region. Fixes eclipse-platform#2346
|
I have extended the PR so that a region stores a zoom hint for temporary handles, in order to ensure that when a temporary handle is necessary, its zoom fits to the GC context such that a potential clipping region can be retrieved. This is the according change: The behavior can be tested as follows:
Before the recent change you will have seen |
I’ve tested the changes , and they seem to have resolved the regression and the issue where No clipping handle for zoom ... errors were previously being printed. I’ve briefly reviewed the code, but I’m not confident approving it as I’m not familiar with the Region implementation or the need for temporary handles. |
|
Thank you for testing the PR!
Can give explain which kind of issue that was? It that inconsistent behavior present in current master state or was it only in the "intermediate" state of this PR? |
It was in intermediate state of the PR before the latest push. |
|
Alright, thank you! Then I'll merge this to have the |
3 participants
The GC#getClipping(Region) call sets the passed region to the current clipping region of the GC. Currently, this is not represented as an operation inside the passed Region, such that when calculating a handle for a different zoom of that region or when copying that region the according application of GC#getClipping(Region) to that region is missing.
This change adds the missing operation to the Region. Since the clipping needs to be calculated at the of executing the GC#getClipping(Region) method and not at the time of applying the region operation when retrieving a handle, the GC#getClipping(Region) stores an operation inside GC that maintains handles for the according clipping region at the time of executing the GC#getClipping(Region) method for every zoom at which the GC is requested. This ensures that the proper clipping state is available to the Region.
Fixes #2346
Supercedes and thus closes #2347