[win32] Adapt Region to support creation of handles with operations with disposed resources #1859
Conversation
akoch-yatta
force-pushed
the
win32-refactor-region
branch
2 times, most recently
from
2c1c7df to
f8d4f9e
Compare
akoch-yatta
force-pushed
the
win32-refactor-region
branch
from
f8d4f9e to
95ce29b
Compare
|
|
||
| private <T> T applyOnAnyHandle(Function<RegionHandle, T> function) { | ||
| if (zoomToHandle.isEmpty()) { | ||
| RegionHandle handle = newRegionHandle(DPIUtil.getDeviceZoom()); |
There was a problem hiding this comment.
nit: maybe call this temporaryhandle to improve comprehensibility like done for Transform
There was a problem hiding this comment.
Adapted that. FYI, I additionally replaced DPIUtil.getDeviceZoom() with device.getDeviceZoom()
| * @see #hashCode | ||
| */ | ||
| @Override | ||
| public boolean equals (Object object) { |
There was a problem hiding this comment.
API tools complain about hashcode/equals as public API methods being removed. I am not sure whether this is intended or a corner case that is not properly covered, still consumers may rely on proper implementations of the methods as they have existed before. On the other hand, the existing implementation of equals were effectively the same as the one from Object, as from my understanding the handle (for initial zoom) for two regions can never be the same. And hashCode used a unique memory pointer before and does it now as well (just taken from a different object).
So in my opinion the change is fine, but we should probably do it consistently across the three OS-specific implementations in a preparatory PR. It would probably be cleanest still remove those methods (and not replace them with just calls to the super implementation), but then we need to add API filters such that the API tooling does not complain.
There was a problem hiding this comment.
Adapted it as recommended and added the api filters
akoch-yatta
force-pushed
the
win32-refactor-region
branch
6 times, most recently
from
0bb4be6 to
b700433
Compare
|
@HeikoKlare I needed to adapt the PR a bit more, because tests failed as some operations that take a Region as input param failed because this region was disposed when a temporary handle was creation. As the OS.CombineRgn calls need a proper Region handle, I extracted a applyOnTemporaryHandle method to create and cleanup temporary handles on demand to theoretically encapsulate Region on Region operations as deep as you want. |
akoch-yatta
force-pushed
the
win32-refactor-region
branch
5 times, most recently
from
e1efc67 to
bafdff2
Compare
akoch-yatta
changed the title
akoch-yatta
force-pushed
the
win32-refactor-region
branch
4 times, most recently
from
d697b9a to
fa94196
Compare
HeikoKlare
force-pushed
the
win32-refactor-region
branch
from
fa94196 to
638f612
Compare
HeikoKlare
left a comment
HeikoKlare
left a comment
There was a problem hiding this comment.
Change looks good. I pushed a minor change to also use the introduced newEmptyRegionHandle() method in the constructor.
This commit refactors Region in the win32 implementation to better support multiple handles for different zoom settings by creating all handles only on demand.
HeikoKlare
force-pushed
the
win32-refactor-region
branch
from
638f612 to
9b63419
Compare
2 participants
This PR refactors Region in the win32 implementation to better support multiple handles for different zoom settings by creating all handles only on demand. It applies the same patterns as #1856 and #1858