Skip to content

[Win32] Represent GC#getClipping(Region) operation in Region #2346 #2348

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 24, 2025
Merged

[Win32] Represent GC#getClipping(Region) operation in Region #2346 #2348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 47 additions & 8 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3790,6 +3790,44 @@ public void getClipping (Region region) {
checkNonDisposed();
if (region == null) SWT.error (SWT.ERROR_NULL_ARGUMENT);
if (region.isDisposed()) SWT.error (SWT.ERROR_INVALID_ARGUMENT);
storeAndApplyOperationForExistingHandle(new GetClippingOperation(region));
}

private class GetClippingOperation extends Operation {
private final Map<Integer, Long> zoomToRegionHandle = new HashMap<>();

public GetClippingOperation(Region region) {
region.set(zoom -> {
if (!zoomToRegionHandle.containsKey(zoom)) {
System.err.println("No clipping handle for zoom " + zoom + " has been created on this GC");
return zoomToRegionHandle.values().iterator().next();
Comment on lines +3801 to +3803
Copy link
Contributor

@arunjose696 arunjose696 Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@HeikoKlare HeikoKlare Jul 24, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@arunjose696 arunjose696 Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@HeikoKlare HeikoKlare Jul 24, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@arunjose696 arunjose696 Jul 24, 2025

Choose a reason for hiding this comment

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

Tested the latest changes the issue is resolved

}
return zoomToRegionHandle.get(zoom);
}, getZoom());
}

// Whenever the GC handle is recalculated for a new zoom, we compute and store the clipping
// at the times when getClipping(Region) was originally called, such that the region to which
// that clipping is set can retrieve it from the storage when required.
@Override
void apply() {
zoomToRegionHandle.computeIfAbsent(getZoom(), __ -> getClippingRegion());
}

@Override
void disposeAll() {
for (long handle : zoomToRegionHandle.values()) {
OS.DeleteObject(handle);
}
super.disposeAll();
}
}

/**
* @return a region handle with the current clipping region of this GC
*/
private long getClippingRegion () {
long regionHandle = OS.CreateRectRgn(0, 0, 0, 0);
long gdipGraphics = data.gdipGraphics;
if (gdipGraphics != 0) {
long rgn = Gdip.Region_new();
Expand All @@ -3799,7 +3837,7 @@ public void getClipping (Region region) {
Gdip.Graphics_SetPixelOffsetMode(gdipGraphics, Gdip.PixelOffsetModeNone);
Gdip.Graphics_GetVisibleClipBounds(gdipGraphics, rect);
Gdip.Graphics_SetPixelOffsetMode(gdipGraphics, Gdip.PixelOffsetModeHalf);
OS.SetRectRgn(Region.win32_getHandle(region, getZoom()), rect.X, rect.Y, rect.X + rect.Width, rect.Y + rect.Height);
OS.SetRectRgn(regionHandle, rect.X, rect.Y, rect.X + rect.Width, rect.Y + rect.Height);
} else {
long matrix = Gdip.Matrix_new(1, 0, 0, 1, 0, 0);
long identity = Gdip.Matrix_new(1, 0, 0, 1, 0, 0);
Expand All @@ -3812,26 +3850,26 @@ public void getClipping (Region region) {
POINT pt = new POINT ();
OS.GetWindowOrgEx (handle, pt);
OS.OffsetRgn (hRgn, pt.x, pt.y);
OS.CombineRgn(Region.win32_getHandle(region, getZoom()), hRgn, 0, OS.RGN_COPY);
OS.CombineRgn(regionHandle, hRgn, 0, OS.RGN_COPY);
OS.DeleteObject(hRgn);
}
Gdip.Region_delete(rgn);
return;
return regionHandle;
}
POINT pt = new POINT ();
OS.GetWindowOrgEx (handle, pt);
int result = OS.GetClipRgn (handle, Region.win32_getHandle(region, getZoom()));
int result = OS.GetClipRgn (handle, regionHandle);
if (result != 1) {
RECT rect = new RECT();
OS.GetClipBox(handle, rect);
OS.SetRectRgn(Region.win32_getHandle(region, getZoom()), rect.left, rect.top, rect.right, rect.bottom);
OS.SetRectRgn(regionHandle, rect.left, rect.top, rect.right, rect.bottom);
} else {
OS.OffsetRgn (Region.win32_getHandle(region, getZoom()), pt.x, pt.y);
OS.OffsetRgn (regionHandle, pt.x, pt.y);
}
long metaRgn = OS.CreateRectRgn (0, 0, 0, 0);
if (OS.GetMetaRgn (handle, metaRgn) != 0) {
OS.OffsetRgn (metaRgn, pt.x, pt.y);
OS.CombineRgn (Region.win32_getHandle(region, getZoom()), metaRgn, Region.win32_getHandle(region, getZoom()), OS.RGN_AND);
OS.CombineRgn (regionHandle, metaRgn, regionHandle, OS.RGN_AND);
}
OS.DeleteObject(metaRgn);
long hwnd = data.hwnd;
Expand All @@ -3848,10 +3886,11 @@ public void getClipping (Region region) {
}
OS.MapWindowPoints (0, hwnd, pt, 1);
OS.OffsetRgn (sysRgn, pt.x, pt.y);
OS.CombineRgn (Region.win32_getHandle(region, getZoom()), sysRgn, Region.win32_getHandle(region, getZoom()), OS.RGN_AND);
OS.CombineRgn (regionHandle, sysRgn, regionHandle, OS.RGN_AND);
}
OS.DeleteObject(sysRgn);
}
return regionHandle;
}

long getFgBrush() {
Expand Down
88 changes: 87 additions & 1 deletion bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Region.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ public final class Region extends Resource {

private boolean isDestroyed;

private int temporaryHandleZoomHint = 0;

/**
* Constructs a new empty region.
* <p>
Expand Down Expand Up @@ -171,11 +173,18 @@ public void add (Region region) {
if (region == null) SWT.error(SWT.ERROR_NULL_ARGUMENT);
if (region.isDisposed()) SWT.error(SWT.ERROR_INVALID_ARGUMENT);
if (!region.operations.isEmpty()) {
adoptTemporaryHandleZoomHint(region);
final Operation operation = new OperationWithRegion(Operation::add, region.operations);
storeAndApplyOperationForAllHandles(operation);
}
}

private void adoptTemporaryHandleZoomHint(Region region) {
if (temporaryHandleZoomHint == 0 && region.temporaryHandleZoomHint != 0) {
this.temporaryHandleZoomHint = region.temporaryHandleZoomHint;
}
}

/**
* Returns <code>true</code> if the point specified by the
* arguments is inside the area specified by the receiver,
Expand Down Expand Up @@ -373,6 +382,7 @@ public void intersect (Region region) {
if (region == null) SWT.error(SWT.ERROR_NULL_ARGUMENT);
if (region.isDisposed()) SWT.error(SWT.ERROR_INVALID_ARGUMENT);
if (!region.operations.isEmpty()) {
adoptTemporaryHandleZoomHint(region);
final Operation operation = new OperationWithRegion(Operation::intersect, region.operations);
storeAndApplyOperationForAllHandles(operation);
}
Expand Down Expand Up @@ -468,6 +478,21 @@ public boolean isEmpty () {
});
}

/**
* Specific method for {@link GC#getClipping(Region)} because the current GC
* clipping settings at that specific point in time of executing the getClipping
* method need to be stored.
* <p>
* The context zoom is used as a hint for the case of creating temporary
* handles, such that they can be created for a zoom for which we know that the
* supplier is capable of providing a proper handle.
*/
void set(Function<Integer, Long> handleForZoomSupplier, int contextZoom) {
this.temporaryHandleZoomHint = contextZoom;
final Operation operation = new OperationWithRegionHandle(Operation::set, handleForZoomSupplier);
storeAndApplyOperationForAllHandles(operation);
}

/**
* Subtracts the given polygon from the collection of polygons
* the receiver maintains to describe its area.
Expand Down Expand Up @@ -559,6 +584,7 @@ public void subtract (Region region) {
if (region == null) SWT.error(SWT.ERROR_NULL_ARGUMENT);
if (region.isDisposed()) SWT.error(SWT.ERROR_INVALID_ARGUMENT);
if (!region.operations.isEmpty()) {
adoptTemporaryHandleZoomHint(region);
final Operation operation = new OperationWithRegion(Operation::subtract, region.operations);
storeAndApplyOperationForAllHandles(operation);
}
Expand Down Expand Up @@ -612,7 +638,8 @@ private void storeAndApplyOperationForAllHandles(Operation operation) {

private <T> T applyUsingAnyHandle(Function<RegionHandle, T> function) {
if (zoomToHandle.isEmpty()) {
return applyUsingTemporaryHandle(device.getDeviceZoom(), operations, function);
int temporaryHandleZoom = temporaryHandleZoomHint != 0 ? temporaryHandleZoomHint : device.getDeviceZoom();
return applyUsingTemporaryHandle(temporaryHandleZoom, operations, function);
}
return function.apply(zoomToHandle.values().iterator().next());
}
Expand Down Expand Up @@ -646,6 +673,7 @@ private RegionHandle getRegionHandle(int zoom) {

Region copy() {
Region region = new Region();
region.temporaryHandleZoomHint = temporaryHandleZoomHint;
region.operations.addAll(operations);
return region;
}
Expand Down Expand Up @@ -705,6 +733,8 @@ void apply(RegionHandle regionHandle) {
operationStrategy.apply(this, regionHandle.handle(), regionHandle.zoom());
}

abstract void set(long handle, int zoom);

abstract void add(long handle, int zoom);

abstract void subtract(long handle, int zoom);
Expand All @@ -722,6 +752,11 @@ private static class OperationWithRectangle extends Operation {
this.data = data;
}

@Override
void set(long handle, int zoom) {
throw new UnsupportedOperationException();
}

@Override
void add(long handle, int zoom) {
Rectangle bounds = getScaledRectangle(zoom);
Expand Down Expand Up @@ -780,6 +815,11 @@ public OperationWithArray(OperationStrategy operationStrategy, int[] data) {
this.data = data;
}

@Override
void set(long handle, int zoom) {
throw new UnsupportedOperationException();
}

@Override
void add(long handle, int zoom) {
int[] points = getScaledPoints(zoom);
Expand Down Expand Up @@ -827,6 +867,11 @@ public OperationWithPoint(OperationStrategy operationStrategy, Point data) {
this.data = data;
}

@Override
void set(long handle, int zoom) {
throw new UnsupportedOperationException();
}

@Override
void add(long handle, int zoom) {
throw new UnsupportedOperationException();
Expand Down Expand Up @@ -858,6 +903,11 @@ private static class OperationWithRegion extends Operation {
this.operations = List.copyOf(operations);
}

@Override
void set(long handle, int zoom) {
throw new UnsupportedOperationException();
}

@Override
void add(long handle, int zoom) {
applyUsingTemporaryHandle(zoom, operations, regionHandle -> {
Expand All @@ -883,5 +933,41 @@ void intersect(long handle, int zoom) {
void translate(long handle, int zoom) {
throw new UnsupportedOperationException();
}

}

private static class OperationWithRegionHandle extends Operation {
private final Function<Integer, Long> handleForZoomProvider;

OperationWithRegionHandle(OperationStrategy operationStrategy, Function<Integer, Long> handleForZoomSupplier) {
super(operationStrategy);
this.handleForZoomProvider = handleForZoomSupplier;
}

@Override
void set(long handle, int zoom) {
OS.CombineRgn(handle, handleForZoomProvider.apply(zoom), 0, OS.RGN_COPY);
}

@Override
void subtract(long handle, int zoom) {
throw new UnsupportedOperationException();
}

@Override
void intersect(long handle, int zoom) {
throw new UnsupportedOperationException();
}

@Override
void translate(long handle, int zoom) {
throw new UnsupportedOperationException();
}

@Override
void add(long handle, int zoom) {
throw new UnsupportedOperationException();
}

}
}
Loading