Skip to content

[Win32] Properly encapsulate cursor handle creation and disposal #2403

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 2 commits into from
Aug 11, 2025
Merged

[Win32] Properly encapsulate cursor handle creation and disposal #2403

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be1dfac
[Win32] Extract mask-specific cursor handle creation to new provider
HeikoKlare Aug 10, 2025
6cd670a
[Win32] Assign cursor handle disposal logic in CursorHandle type
HeikoKlare Aug 10, 2025
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
151 changes: 92 additions & 59 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Cursor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,9 @@ public final class Cursor extends Resource {
*/
private static final int DEFAULT_ZOOM = 100;

private HashMap<Integer, Long> zoomLevelToHandle = new HashMap<>();
private HashMap<Integer, CursorHandle> zoomLevelToHandle = new HashMap<>();

private final CursorHandleProvider cursorHandleProvider;
boolean isIcon;

/**
* Constructs a new cursor given a device and a style
Expand Down Expand Up @@ -120,7 +119,7 @@ public final class Cursor extends Resource {
public Cursor(Device device, int style) {
super(device);
this.cursorHandleProvider = new StyleCursorHandleProvider(style);
this.handle = this.cursorHandleProvider.createHandle(this, DEFAULT_ZOOM);
this.handle = this.cursorHandleProvider.createHandle(device, DEFAULT_ZOOM).getHandle();
Copy link
Contributor

@arunjose696 arunjose696 Aug 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

just a Nit: The handle field still stores the raw long, while zoomLevelToHandle stores the CursorHandle. This introduces a risk of the two becoming out-of-sync if the logic changes in the future. IMO It might be cleaner to store the CursorHandle directly in handle and call this.handle.getHandle() (or rename the method to getNativeHandle() for clarity) wherever the long handle is needed @HeikoKlare

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true that the two values may get out of sync but that's actually not different from the current state. CursorHandle is immutable, so it will be as consistent to the current handle value as it was before. We need to get rid of the handle, but that's something that will be done with the lazy generation of handles implemented by Shahzaib.

init();
this.device.registerResourceWithZoomSupport(this);
}
Expand Down Expand Up @@ -160,13 +159,13 @@ public Cursor(Device device, int style) {
*/
public Cursor(Device device, ImageData source, ImageData mask, int hotspotX, int hotspotY) {
super(device);
this.cursorHandleProvider = new ImageDataCursorHandleProvider(source, mask, hotspotX, hotspotY);
this.handle = this.cursorHandleProvider.createHandle(this, DEFAULT_ZOOM);
this.cursorHandleProvider = new ImageDataWithMaskCursorHandleProvider(source, mask, hotspotX, hotspotY);
this.handle = this.cursorHandleProvider.createHandle(device, DEFAULT_ZOOM).getHandle();
init();
this.device.registerResourceWithZoomSupport(this);
}

private static long setupCursorFromImageData(ImageData source, ImageData mask, int hotspotX, int hotspotY) {
private static CursorHandle setupCursorFromImageData(ImageData source, ImageData mask, int hotspotX, int hotspotY) {
if (source == null) SWT.error(SWT.ERROR_NULL_ARGUMENT);
if (mask == null) {
if (source.getTransparencyType() != SWT.TRANSPARENCY_MASK) {
Expand Down Expand Up @@ -195,7 +194,7 @@ private static long setupCursorFromImageData(ImageData source, ImageData mask, i
long hInst = OS.GetModuleHandle(null);
long handle = OS.CreateCursor(hInst, hotspotX, hotspotY, source.width, source.height, sourceData, maskData);
if (handle == 0) SWT.error(SWT.ERROR_NO_HANDLES);
return handle;
return new CustomCursorHandle(handle);
}

/**
Expand Down Expand Up @@ -229,14 +228,13 @@ private static long setupCursorFromImageData(ImageData source, ImageData mask, i
*/
public Cursor(Device device, ImageData source, int hotspotX, int hotspotY) {
super(device);
this.cursorHandleProvider = new ImageDataCursorHandleProvider(source, null, hotspotX, hotspotY);
isIcon = true;
this.handle = this.cursorHandleProvider.createHandle(this, DEFAULT_ZOOM);
this.cursorHandleProvider = new ImageDataCursorHandleProvider(source, hotspotX, hotspotY);
this.handle = this.cursorHandleProvider.createHandle(device, DEFAULT_ZOOM).getHandle();
init();
this.device.registerResourceWithZoomSupport(this);
}

private static long setupCursorFromImageData(Device device, ImageData source, int hotspotX, int hotspotY) {
private static CursorHandle setupCursorFromImageData(Device device, ImageData source, int hotspotX, int hotspotY) {
if (source == null) SWT.error(SWT.ERROR_NULL_ARGUMENT);
/* Check the hotspots */
if (hotspotX >= source.width || hotspotX < 0 ||
Expand Down Expand Up @@ -307,7 +305,7 @@ private static long setupCursorFromImageData(Device device, ImageData source, in
OS.DeleteObject(hMask);
if (handle == 0) SWT.error(SWT.ERROR_NO_HANDLES);

return handle;
return new IconCursorHandle(handle);
}

/**
Expand Down Expand Up @@ -343,8 +341,7 @@ public Cursor(Device device, ImageDataProvider imageDataProvider, int hotspotX,
super(device);
if (imageDataProvider == null) SWT.error(SWT.ERROR_NULL_ARGUMENT);
this.cursorHandleProvider = new ImageDataProviderCursorHandleProvider(imageDataProvider, hotspotX, hotspotY);
isIcon = true;
this.handle = this.cursorHandleProvider.createHandle(this, DEFAULT_ZOOM);
this.handle = this.cursorHandleProvider.createHandle(device, DEFAULT_ZOOM).getHandle();
init();
this.device.registerResourceWithZoomSupport(this);
}
Expand All @@ -369,18 +366,18 @@ public static Long win32_getHandle (Cursor cursor, int zoom) {
return cursor.handle;
}
if (cursor.zoomLevelToHandle.get(zoom) != null) {
return cursor.zoomLevelToHandle.get(zoom);
return cursor.zoomLevelToHandle.get(zoom).getHandle();
}

long handle = cursor.cursorHandleProvider.createHandle(cursor, zoom);
CursorHandle handle = cursor.cursorHandleProvider.createHandle(cursor.device, zoom);
cursor.setHandleForZoomLevel(handle, zoom);

return cursor.zoomLevelToHandle.get(zoom);
return cursor.zoomLevelToHandle.get(zoom).getHandle();
}

private void setHandleForZoomLevel(long handle, Integer zoom) {
private void setHandleForZoomLevel(CursorHandle handle, Integer zoom) {
if (this.handle == 0) {
this.handle = handle; // Set handle for default zoom level
this.handle = handle.getHandle(); // Set handle for default zoom level
}
if (zoom != null && !zoomLevelToHandle.containsKey(zoom)) {
zoomLevelToHandle.put(zoom, handle);
Expand All @@ -406,29 +403,13 @@ void destroy () {
}

private void destroyHandle () {
for (Long handle : zoomLevelToHandle.values()) {
destroyHandle(handle);
for (CursorHandle handle : zoomLevelToHandle.values()) {
handle.destroy();
}
zoomLevelToHandle.clear();
handle = 0;
}

private void destroyHandle(long handle) {
if (isIcon) {
OS.DestroyIcon(handle);
} else {
/*
* The MSDN states that one should not destroy a shared
* cursor, that is, one obtained from LoadCursor.
* However, it does not appear to do any harm, so rather
* than keep track of how a cursor was created, we just
* destroy them all. If this causes problems in the future,
* put the flag back in.
*/
OS.DestroyCursor(handle);
}
}

/**
* Compares the argument to the receiver, and returns true
* if they represent the <em>same</em> object using a class
Expand Down Expand Up @@ -494,15 +475,59 @@ void destroyHandlesExcept(Set<Integer> zoomLevels) {
zoomLevelToHandle.entrySet().removeIf(entry -> {
final Integer zoom = entry.getKey();
if (!zoomLevels.contains(zoom) && zoom != DPIUtil.getZoomForAutoscaleProperty(DEFAULT_ZOOM)) {
destroyHandle(entry.getValue());
entry.getValue().destroy();
return true;
}
return false;
});
}

private static abstract class CursorHandle {
private final long handle;

public CursorHandle(long handle) {
this.handle = handle;
}

long getHandle() {
return handle;
}

abstract void destroy();
}

private static class IconCursorHandle extends CursorHandle {
public IconCursorHandle(long handle) {
super(handle);
}

@Override
void destroy() {
OS.DestroyIcon(getHandle());
}
}

private static class CustomCursorHandle extends CursorHandle {
public CustomCursorHandle(long handle) {
super(handle);
}

@Override
void destroy() {
/*
* The MSDN states that one should not destroy a shared
* cursor, that is, one obtained from LoadCursor.
* However, it does not appear to do any harm, so rather
* than keep track of how a cursor was created, we just
* destroy them all. If this causes problems in the future,
* put the flag back in.
*/
OS.DestroyCursor(getHandle());
}
}

private static interface CursorHandleProvider {
long createHandle(Cursor cursor, int zoom);
CursorHandle createHandle(Device device, int zoom);
}

private static class StyleCursorHandleProvider implements CursorHandleProvider {
Expand All @@ -513,12 +538,12 @@ public StyleCursorHandleProvider(int style) {
}

@Override
public long createHandle(Cursor cursor, int zoom) {
public CursorHandle createHandle(Device device, int zoom) {
// zoom ignored, LoadCursor handles scaling internally
return setupCursorFromStyle(this.style);
}

private static final long setupCursorFromStyle(int style) {
private static final CursorHandle setupCursorFromStyle(int style) {
long lpCursorName = 0;
switch (style) {
case SWT.CURSOR_HAND:
Expand Down Expand Up @@ -594,7 +619,7 @@ private static final long setupCursorFromStyle(int style) {
if (handle == 0) {
SWT.error(SWT.ERROR_NO_HANDLES);
}
return handle;
return new CustomCursorHandle(handle);
}
}

Expand Down Expand Up @@ -625,41 +650,49 @@ public ImageDataProviderCursorHandleProvider(ImageDataProvider provider, int hot
}

@Override
public long createHandle(Cursor cursor, int zoom) {
public CursorHandle createHandle(Device device, int zoom) {
ImageData source;
if (zoom == DEFAULT_ZOOM) {
source = this.provider.getImageData(DEFAULT_ZOOM);
} else {
Image tempImage = new Image(cursor.device, this.provider);
Image tempImage = new Image(device, this.provider);
source = tempImage.getImageData(zoom);
tempImage.dispose();
}
return setupCursorFromImageData(cursor.device, source, getHotpotXInPixels(zoom), getHotpotYInPixels(zoom));
return setupCursorFromImageData(device, source, getHotpotXInPixels(zoom), getHotpotYInPixels(zoom));
}
}

private static class ImageDataCursorHandleProvider extends HotspotAwareCursorHandleProvider {
private final ImageData source;
private final ImageData mask;
protected final ImageData source;

public ImageDataCursorHandleProvider(ImageData source, ImageData mask, int hotspotX, int hotspotY) {
public ImageDataCursorHandleProvider(ImageData source, int hotspotX, int hotspotY) {
super(hotspotX, hotspotY);
this.source = source;
}

@Override
public CursorHandle createHandle(Device device, int zoom) {
ImageData scaledSource = DPIUtil.scaleImageData(device, this.source, zoom, DEFAULT_ZOOM);
return setupCursorFromImageData(device, scaledSource, getHotpotXInPixels(zoom),
getHotpotYInPixels(zoom));
}
}

private static class ImageDataWithMaskCursorHandleProvider extends ImageDataCursorHandleProvider {
private final ImageData mask;

public ImageDataWithMaskCursorHandleProvider(ImageData source, ImageData mask, int hotspotX, int hotspotY) {
super(source, hotspotX, hotspotY);
this.mask = mask;
}

@Override
public long createHandle(Cursor cursor, int zoom) {
ImageData scaledSource = DPIUtil.scaleImageData(cursor.device, this.source, zoom, DEFAULT_ZOOM);
if (cursor.isIcon) {
return setupCursorFromImageData(cursor.device, scaledSource, getHotpotXInPixels(zoom),
getHotpotYInPixels(zoom));
} else {
ImageData scaledMask = this.mask != null ? DPIUtil.scaleImageData(cursor.device, mask, zoom, DEFAULT_ZOOM)
: null;
return setupCursorFromImageData(scaledSource, scaledMask, getHotpotXInPixels(zoom),
getHotpotYInPixels(zoom));
}
public CursorHandle createHandle(Device device, int zoom) {
ImageData scaledSource = DPIUtil.scaleImageData(device, this.source, zoom, DEFAULT_ZOOM);
ImageData scaledMask = this.mask != null ? DPIUtil.scaleImageData(device, mask, zoom, DEFAULT_ZOOM)
: null;
return setupCursorFromImageData(scaledSource, scaledMask, getHotpotXInPixels(zoom), getHotpotYInPixels(zoom));
}
}

Expand Down
Loading