Skip to content

[win32] Cleanup image wrappers #2036

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
Apr 24, 2025
Merged

[win32] Cleanup image wrappers #2036

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
188 changes: 65 additions & 123 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,23 +780,7 @@ private ImageHandle getImageMetadata(int zoom) {
if (zoomLevelToImageHandle.get(zoom) != null) {
return zoomLevelToImageHandle.get(zoom);
}
if (imageProvider != null) {
return imageProvider.getImageMetadata(zoom);
} else {
ImageData resizedData = getImageData(zoom);
ImageData newData = adaptImageDataIfDisabledOrGray(resizedData);
Copy link
Contributor

Choose a reason for hiding this comment

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

This was removed at the place where this code was moved to. Was that intended, i.e., it is sufficient to only do the adaptation at the remaining places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, it was not completly correct. I want to cover all places, where scaled variants of an ImageData is created and those could be not adapted. My latest commit removed an unnecessary call (calling place already does it) and adds one call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! It looks fine now. I didn't find a path that is not covered properly. Still, we might think about if we could further improve this in the future as the it's quite difficult to understand where this adaptation logic has to be placed and why it is correct right now. And since it's hard to test, it may easily be removed at some of the placed accidentally, leading to errors that are not easily perceived.
But for now, it's fine as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. We should try to better streamline ImageData creation/scaling in Image

if (type == SWT.ICON && newData.getTransparencyType() != SWT.TRANSPARENCY_MASK) {
// If the original type was an icon with transparency mask and re-scaling leads
// to image data without transparency mask, this will create invalid images
// so this fallback will "repair" the image data by explicitly passing
// the transparency mask created from the scaled image data
initIconHandle(this.device, newData, newData.getTransparencyMask(), zoom);
} else {
init(newData, zoom);
}
init();
}
return zoomLevelToImageHandle.get(zoom);
return imageProvider.newImageHandle(zoom);
}


Expand Down Expand Up @@ -999,9 +983,7 @@ public static long win32_getHandle (Image image, int zoom) {
void destroy () {
device.deregisterResourceWithZoomSupport(this);
if (memGC != null) memGC.dispose();
if (this.imageProvider != null) {
this.imageProvider.destroy();
}
this.imageProvider.destroy();
destroyHandle();
memGC = null;
}
Expand Down Expand Up @@ -1055,11 +1037,7 @@ public boolean equals (Object object) {
if (!(object instanceof Image)) return false;
Image image = (Image) object;
if (device != image.device || transparentPixel != image.transparentPixel || getZoom() != image.getZoom()) return false;
if (imageProvider != null && image.imageProvider != null) {
return (styleFlag == image.styleFlag) && imageProvider.equals(image.imageProvider);
} else {
return win32_getHandle(this, getZoom()) == win32_getHandle(image, getZoom());
}
return (styleFlag == image.styleFlag) && imageProvider.equals(image.imageProvider);
}

/**
Expand Down Expand Up @@ -1156,13 +1134,8 @@ Rectangle getBounds(int zoom) {
ImageHandle imageMetadata = zoomLevelToImageHandle.get(zoom);
Rectangle rectangle = new Rectangle(0, 0, imageMetadata.width, imageMetadata.height);
return DPIUtil.scaleBounds(rectangle, zoom, imageMetadata.zoom);
} else if (this.imageProvider != null) {
return this.imageProvider.getBounds(zoom);
} else {
ImageHandle imageMetadata = zoomLevelToImageHandle.values().iterator().next();
Rectangle rectangle = new Rectangle(0, 0, imageMetadata.width, imageMetadata.height);
return DPIUtil.scaleBounds(rectangle, zoom, imageMetadata.zoom);
}
return this.imageProvider.getBounds(zoom);
}

/**
Expand Down Expand Up @@ -1236,25 +1209,7 @@ public ImageData getImageData() {
*/
public ImageData getImageData (int zoom) {
if (isDisposed()) SWT.error(SWT.ERROR_GRAPHIC_DISPOSED);
if (zoomLevelToImageHandle.containsKey(zoom)) {
return zoomLevelToImageHandle.get(zoom).getImageData();
}
if (imageProvider != null) {
return imageProvider.getImageData(zoom);
}

return getScaledImageData(zoom);
}

private ImageData getScaledImageData (int zoom) {
// if a GC is initialized with an Image (memGC != null), the image data must not be resized, because it would
// be a destructive operation. Therefor, always the current image data must be returned
if (memGC != null) {
return getImageDataAtCurrentZoom();
}
TreeSet<Integer> availableZooms = new TreeSet<>(zoomLevelToImageHandle.keySet());
int closestZoom = Optional.ofNullable(availableZooms.higher(zoom)).orElse(availableZooms.lower(zoom));
return scaleImageData(getImageMetadata(closestZoom).getImageData(), zoom, closestZoom);
return imageProvider.getImageData(zoom);
}


Expand Down Expand Up @@ -1294,10 +1249,7 @@ public ImageData getImageDataAtCurrentZoom() {
*/
@Override
public int hashCode () {
if(imageProvider != null) {
return imageProvider.hashCode();
}
return (int)win32_getHandle(this, getZoom());
return imageProvider.hashCode();
}

static long createDIB(int width, int height, int depth) {
Expand Down Expand Up @@ -1794,10 +1746,7 @@ public void internal_dispose_GC (long hDC, GCData data) {
*/
@Override
public boolean isDisposed() {
if (this.imageProvider != null) {
return this.imageProvider.isDisposed();
}
return zoomLevelToImageHandle.isEmpty();
return this.imageProvider.isDisposed();
}

/**
Expand Down Expand Up @@ -1918,10 +1867,42 @@ private abstract class AbstractImageProviderWrapper {
private boolean isDestroyed;

protected abstract Rectangle getBounds(int zoom);
abstract ImageData getImageData(int zoom);
abstract ImageHandle getImageMetadata(int zoom);

protected final ImageData getImageData(int zoom) {
if (zoomLevelToImageHandle.containsKey(zoom)) {
return zoomLevelToImageHandle.get(zoom).getImageData();
}
return newImageData(zoom);
}

abstract ImageData newImageData(int zoom);

abstract AbstractImageProviderWrapper createCopy(Image image);

ImageData getScaledImageData (int zoom) {
// if a GC is initialized with an Image (memGC != null), the image data must not be resized, because it would
// be a destructive operation. Therefor, always the current image data must be returned
if (memGC != null) {
return getImageDataAtCurrentZoom();
}
TreeSet<Integer> availableZooms = new TreeSet<>(zoomLevelToImageHandle.keySet());
int closestZoom = Optional.ofNullable(availableZooms.higher(zoom)).orElse(availableZooms.lower(zoom));
return scaleImageData(getImageMetadata(closestZoom).getImageData(), zoom, closestZoom);
}

protected ImageHandle newImageHandle(int zoom) {
ImageData resizedData = getImageData(zoom);
if (type == SWT.ICON && resizedData.getTransparencyType() != SWT.TRANSPARENCY_MASK) {
// If the original type was an icon with transparency mask and re-scaling leads
// to image data without transparency mask, this will create invalid images
// so this fallback will "repair" the image data by explicitly passing
// the transparency mask created from the scaled image data
return initIconHandle(device, resizedData, resizedData.getTransparencyMask(), zoom);
} else {
return init(resizedData, zoom);
}
}

protected boolean isDisposed() {
return !isInitialized || isDestroyed;
}
Expand Down Expand Up @@ -1955,33 +1936,10 @@ protected Rectangle getBounds(int zoom) {
}

@Override
ImageData getImageData(int zoom) {
if (zoomLevelToImageHandle.isEmpty() || zoomLevelToImageHandle.containsKey(zoom)) {
return getImageMetadata(zoom).getImageData();
}

ImageData newImageData(int zoom) {
return getScaledImageData(zoom);
}

@Override
ImageHandle getImageMetadata(int zoom) {
if (zoomLevelToImageHandle.containsKey(zoom)) {
return zoomLevelToImageHandle.get(zoom);
} else {
ImageData resizedData = getImageData(zoom);
ImageData newData = adaptImageDataIfDisabledOrGray(resizedData);
if (type == SWT.ICON && newData.getTransparencyType() != SWT.TRANSPARENCY_MASK) {
// If the original type was an icon with transparency mask and re-scaling leads
// to image data without transparency mask, this will create invalid images
// so this fallback will "repair" the image data by explicitly passing
// the transparency mask created from the scaled image data
return initIconHandle(device, newData, newData.getTransparencyMask(), zoom);
} else {
return init(newData, zoom);
}
}
}

@Override
AbstractImageProviderWrapper createCopy(Image image) {
return image.new ExistingImageHandleProviderWrapper(handle, zoomForHandle);
Expand All @@ -2001,26 +1959,13 @@ void initImage() {
}

@Override
ImageData getImageData(int zoom) {
if (zoomLevelToImageHandle.containsKey(zoom)) {
return zoomLevelToImageHandle.get(zoom).getImageData();
}
ImageData newImageData(int zoom) {
if (!zoomLevelToImageHandle.isEmpty()) {
return getScaledImageData(zoom);
}
ElementAtZoom<ImageData> loadedImageData = loadImageData(zoom);
return DPIUtil.scaleImageData(device, loadedImageData, zoom);
}

@Override
ImageHandle getImageMetadata(int zoom) {
if (zoomLevelToImageHandle.containsKey(zoom)) {
return zoomLevelToImageHandle.get(zoom);
} else {
ImageData scaledImageData = getImageData(zoom);
ImageHandle imageHandle = init(scaledImageData, zoom);
return imageHandle;
}
ImageData scaledImageData = DPIUtil.scaleImageData(device, loadedImageData, zoom);
return adaptImageDataIfDisabledOrGray(scaledImageData);
}
}

Expand Down Expand Up @@ -2133,29 +2078,26 @@ protected Rectangle getBounds(int zoom) {
}

@Override
ImageData getImageData(int zoom) {
if (zoomLevelToImageHandle.isEmpty() || zoomLevelToImageHandle.containsKey(zoom)) {
return getImageMetadata(zoom).getImageData();
ImageData newImageData(int zoom) {
if (zoomLevelToImageHandle.isEmpty()) {
return createBaseHandle(zoom).getImageData();
}

return getScaledImageData(zoom);
}

@Override
ImageHandle getImageMetadata(int zoom) {
protected ImageHandle newImageHandle(int zoom) {
if (zoomLevelToImageHandle.isEmpty()) {
long handle = initBaseHandle(zoom);
ImageHandle imageHandle = new ImageHandle(handle, zoom);
zoomLevelToImageHandle.put(zoom, imageHandle);
return imageHandle;
} else if(zoomLevelToImageHandle.containsKey(zoom)) {
return zoomLevelToImageHandle.get(zoom);
} else {
ImageData resizedData = getImageData(zoom);
ImageData newData = adaptImageDataIfDisabledOrGray(resizedData);
init(newData, zoom);
return zoomLevelToImageHandle.get(zoom);
return createBaseHandle(zoom);
}
return super.newImageHandle(zoom);
}

private ImageHandle createBaseHandle(int zoom) {
long handle = initBaseHandle(zoom);
ImageHandle imageHandle = new ImageHandle(handle, zoom);
zoomLevelToImageHandle.put(zoom, imageHandle);
return imageHandle;
}

private long initBaseHandle(int zoom) {
Expand Down Expand Up @@ -2233,7 +2175,7 @@ Object getProvider() {
}

@Override
final ImageData getImageData(int zoom) {
ImageData newImageData(int zoom) {
Function<Integer, ImageData> imageDataRetrival = zoomToRetrieve -> {
ImageHandle handle = initializeHandleFromSource(zoomToRetrieve);
ImageData data = handle.getImageData();
Expand All @@ -2243,8 +2185,9 @@ final ImageData getImageData(int zoom) {
return cachedImageData.computeIfAbsent(zoom, imageDataRetrival);
}


@Override
final ImageHandle getImageMetadata(int zoom) {
protected ImageHandle newImageHandle(int zoom) {
ImageData cachedData = cachedImageData.remove(zoom);
if (cachedData != null) {
return init(cachedData, zoom);
Expand Down Expand Up @@ -2280,7 +2223,7 @@ protected ElementAtZoom<ImageData> loadImageData(int zoom) {
// Load at appropriate zoom via loader
if (fileForZoom.zoom() != zoom && ImageDataLoader.canLoadAtZoom(fileForZoom.element(), fileForZoom.zoom(), zoom)) {
ElementAtZoom<ImageData> imageDataAtZoom = ImageDataLoader.load(fileForZoom.element(), fileForZoom.zoom(), zoom);
return new ElementAtZoom<>(adaptImageDataIfDisabledOrGray(imageDataAtZoom.element()), zoom);
return new ElementAtZoom<>(imageDataAtZoom.element(), zoom);
}

// Load at file zoom (native or via loader) and rescale
Expand Down Expand Up @@ -2539,12 +2482,12 @@ protected Rectangle getBounds(int zoom) {
}

@Override
ImageData getImageData(int zoom) {
ImageData newImageData(int zoom) {
return getImageMetadata(zoom).getImageData();
}

@Override
ImageHandle getImageMetadata(int zoom) {
protected ImageHandle newImageHandle(int zoom) {
initialNativeZoom = zoom;
Image image = new Image(device, width, height, zoom);
GC gc = new GC(image, drawer.getGcStyle());
Expand All @@ -2554,12 +2497,11 @@ ImageHandle getImageMetadata(int zoom) {
ImageData imageData = image.getImageMetadata(zoom).getImageData();
drawer.postProcess(imageData);
ImageData newData = adaptImageDataIfDisabledOrGray(imageData);
init(newData, zoom);
return init(newData, zoom);
} finally {
gc.dispose();
image.dispose();
}
return zoomLevelToImageHandle.get(zoom);
}

@Override
Expand Down
Loading