Skip to content

[Refactoring] Streamline calls to Image#adaptImageDataIfDisabledOrGray() #2795

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

Draft
ShahzaibIbrahim wants to merge 3 commits into
base: master
Choose a base branch
from
+25 −36
Draft

[Refactoring] Streamline calls to Image#adaptImageDataIfDisabledOrGray() #2795

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91c9747
Streamline calls to Image#adaptImageDataIfDisabledOrGray()
ShahzaibIbrahim Nov 5, 2025
eb0405c
Implementing load logic for PlainImageDataProvider
ShahzaibIbrahim Nov 19, 2025
6bce6ef
Remove unnecessary variable.
ShahzaibIbrahim Dec 11, 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
61 changes: 25 additions & 36 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 @@ -172,8 +172,7 @@ private boolean isReusable(int width, int height) {
private Optional<ImageHandle> createHandleAtExactSize(int width, int height) {
Optional<ImageData> imageData = imageProvider.loadImageDataAtExactSize(width, height);
if (imageData.isPresent()) {
ImageData adaptedData = adaptImageDataIfDisabledOrGray(imageData.get());
ImageHandle imageHandle = init(adaptedData, -1);
ImageHandle imageHandle = init(imageData.get(), -1);
Copy link
Contributor

@HeikoKlare HeikoKlare Dec 10, 2025

Choose a reason for hiding this comment

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

Why don't we apply gray/disablement here anymore?

Copy link
Contributor Author

@ShahzaibIbrahim ShahzaibIbrahim Dec 11, 2025

Choose a reason for hiding this comment

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

Because data will already be loaded with disablement/gray properties.

return Optional.of(imageHandle);
}
return Optional.empty();
Expand All @@ -186,9 +185,8 @@ private ImageHandle getOrCreateImageHandleAtClosestSize(int widthHint, int heigh
int imageZoom = DPIUtil.getZoomForAutoscaleProperty(Math.max(imageZoomForWidth, imageZoomForHeight));
ImageHandle bestFittingHandle = zoomLevelToImageHandle.get(imageZoom);
if (bestFittingHandle == null) {
ImageData bestFittingImageData = imageProvider.loadImageData(imageZoom).element();
ImageData adaptedData = adaptImageDataIfDisabledOrGray(bestFittingImageData);
bestFittingHandle = init(adaptedData, -1);
ImageData bestFittingImageData = imageProvider.loadImageDataWithGrayOrDisablement(imageZoom).element();
bestFittingHandle = init(bestFittingImageData, -1);
}
return bestFittingHandle;
}
Expand Down Expand Up @@ -333,21 +331,10 @@ public Image(Device device, Image srcImage, int flag) {
}
break;
}
case SWT.IMAGE_DISABLE: {
for (ImageHandle imageHandle : srcImage.zoomLevelToImageHandle.values()) {
Rectangle rect = imageHandle.getBounds();
ImageData data = srcImage.getImageData(imageHandle.zoom);
ImageData newData = applyDisableImageData(data, rect.height, rect.width);
init (newData, imageHandle.zoom);
}
break;
}
case SWT.IMAGE_DISABLE:
case SWT.IMAGE_GRAY: {
for (ImageHandle imageHandle : srcImage.zoomLevelToImageHandle.values()) {
Rectangle rect = imageHandle.getBounds();
ImageData data = srcImage.getImageData(imageHandle.zoom);
ImageData newData = applyGrayImageData(data, rect.height, rect.width);
init (newData, imageHandle.zoom);
srcImage.getImageData(imageHandle.zoom);
Copy link
Contributor

@HeikoKlare HeikoKlare Dec 10, 2025

Choose a reason for hiding this comment

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

Wny do we remove the gray/disabled adaptation and, especially, the init call here? Won't this lead to the new image not being initalized with appropriate handles anymore?

Copy link
Contributor Author

@ShahzaibIbrahim ShahzaibIbrahim Dec 11, 2025

Choose a reason for hiding this comment

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

Since the logic to apply gray/disablement is moved to whenever image data is loaded, we don't need to apply adaptions here (also the init method).

Copy link
Contributor

@HeikoKlare HeikoKlare Dec 11, 2025

Choose a reason for hiding this comment

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

This is an image constructor, which copied/adapts the handles of the original image. Not doing that will leave the new image empty. How should that work, e.g., in case you have an image based on an existing handle? The new Image instance with GRAY/DISABLED flag would simply be empty.

}
Comment on lines +334 to 338
Copy link
Contributor Author

@ShahzaibIbrahim ShahzaibIbrahim Nov 19, 2025

Choose a reason for hiding this comment

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

Now we only need to reload the data for other handle if exist in order to apply gray or disable image data.

break;
}
Expand Down Expand Up @@ -698,9 +685,9 @@ public Image(Device device, ImageGcDrawer imageGcDrawer, int width, int height)
init();
}

private ImageData adaptImageDataIfDisabledOrGray(ImageData data) {
private static ImageData adaptImageDataIfDisabledOrGray(ImageData data, int styleFlag) {
ImageData returnImageData = null;
switch (this.styleFlag) {
switch (styleFlag) {
case SWT.IMAGE_DISABLE: {
ImageData newData = applyDisableImageData(data, data.height, data.width);
returnImageData = newData;
Expand All @@ -726,7 +713,7 @@ void init() {
this.isInitialized = true;
}

private ImageData applyDisableImageData(ImageData data, int height, int width) {
private static ImageData applyDisableImageData(ImageData data, int height, int width) {
PaletteData palette = data.palette;
ImageData newData = new ImageData(width, height, 32, new PaletteData(0xFF, 0xFF00, 0xFF0000));
newData.alpha = data.alpha;
Expand Down Expand Up @@ -775,7 +762,7 @@ private ImageData applyDisableImageData(ImageData data, int height, int width) {
return newData;
}

private ImageData applyGrayImageData(ImageData data, int pHeight, int pWidth) {
private static ImageData applyGrayImageData(ImageData data, int pHeight, int pWidth) {
PaletteData palette = data.palette;
ImageData newData = data;
if (!palette.isDirect) {
Expand Down Expand Up @@ -2007,6 +1994,11 @@ public Collection<Integer> getPreservedZoomLevels() {

protected abstract ElementAtZoom<ImageData> loadImageData(int zoom);

protected ElementAtZoom<ImageData> loadImageDataWithGrayOrDisablement(int zoom) {
ElementAtZoom<ImageData> imageDataAtZoom = loadImageData(zoom);
return new ElementAtZoom<>(adaptImageDataIfDisabledOrGray(imageDataAtZoom.element(), styleFlag), imageDataAtZoom.zoom());
}

abstract ImageData newImageData(ZoomContext zoomContext);

abstract AbstractImageProviderWrapper createCopy(Image image);
Expand Down Expand Up @@ -2119,9 +2111,8 @@ protected ImageHandle newImageHandle(ZoomContext zoomContext) {
}

private ImageHandle initializeHandleFromSource(ZoomContext zoomContext) {
ElementAtZoom<ImageData> imageDataAtZoom = loadImageData(zoomContext.targetZoom());
ElementAtZoom<ImageData> imageDataAtZoom = loadImageDataWithGrayOrDisablement(zoomContext.targetZoom());
ImageData imageData = DPIUtil.scaleImageData(device, imageDataAtZoom.element(), zoomContext.targetZoom(), imageDataAtZoom.zoom());
imageData = adaptImageDataIfDisabledOrGray(imageData);
return newImageHandle(imageData, zoomContext);
}
}
Expand Down Expand Up @@ -2271,21 +2262,20 @@ protected Rectangle getBounds(int zoom) {

@Override
ImageData newImageData(ZoomContext zoomContext) {
int targetZoom = zoomContext.targetZoom();
return loadImageDataWithGrayOrDisablement(zoomContext.targetZoom()).element();
}

@Override
protected ElementAtZoom<ImageData> loadImageData(int targetZoom) {
if (zoomLevelToImageHandle.isEmpty()) {
return createBaseHandle(targetZoom).getImageData();
return new ElementAtZoom<> (createBaseHandle(targetZoom).getImageData(), targetZoom);
}
// 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, a new handle is created for the requested zoom
if (memGC != null) {
return newImageHandle(zoomContext).getImageData();
return new ElementAtZoom<> (newImageHandle(new ZoomContext(targetZoom)).getImageData(), targetZoom);
}
return getScaledImageData(targetZoom);
}

@Override
protected ElementAtZoom<ImageData> loadImageData(int zoom) {
return getClosestAvailableImageData(zoom);
return new ElementAtZoom<> (getScaledImageData(targetZoom), targetZoom);
Comment on lines +2265 to +2278
Copy link
Contributor

@HeikoKlare HeikoKlare Dec 10, 2025

Choose a reason for hiding this comment

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

Why is this semantics change of the methods necessary? In case of a PlainImageProviderWrapper there is no need to apply any gray/disabled if we just create an empty handle. And if we rescale an existing handle, that one should already have gray/disablement applied.

}

@Override
Expand Down Expand Up @@ -2413,9 +2403,8 @@ protected ImageHandle newImageHandle(ZoomContext zoomContext) {
}

private ImageHandle initializeHandleFromSource(int zoom) {
ElementAtZoom<ImageData> imageDataAtZoom = loadImageData(zoom);
ElementAtZoom<ImageData> imageDataAtZoom = loadImageDataWithGrayOrDisablement(zoom);
ImageData imageData = DPIUtil.scaleImageData (device, imageDataAtZoom.element(), zoom, imageDataAtZoom.zoom());
imageData = adaptImageDataIfDisabledOrGray(imageData);
return init(imageData, zoom);
}

Expand Down Expand Up @@ -2765,7 +2754,7 @@ protected ImageHandle newImageHandle(ZoomContext zoomContext) {
drawer.drawOn(gc, width, height);
ImageData imageData = image.getImageData(targetZoom);
drawer.postProcess(imageData);
ImageData newData = adaptImageDataIfDisabledOrGray(imageData);
ImageData newData = adaptImageDataIfDisabledOrGray(imageData, styleFlag);
Copy link
Contributor Author

@ShahzaibIbrahim ShahzaibIbrahim Nov 19, 2025

Choose a reason for hiding this comment

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

I still have to keep it because a new image is created here with plainImageDataProvider which loads the data directly from handle. I am open to suggestions here.

return init(newData, targetZoom);
} finally {
gc.dispose();
Expand Down