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

ShahzaibIbrahim · 2025-11-19T13:27:00Z

Enabling a consistent pattern to apply disablement or graying of an Image. They are applied whenever a handle is being created from init method. Ensuring streamline usage and avoid double calls to this method.

How to test

Run the Snippet382 with following VM Argument: swt.autoScale.updateOnRuntime=true
Move the shell between different zoom levels (100,125,150,175,200,250)
The snippet tests different kind of image providers to create an Image.
Expected result would be that we don't lose disabled or grayed image attributes.

This adaption is made on the basis of this comment: #2737 (comment)

Instead of calling the adaptImageDataIfDisabledOrGray method from init method (which could be executed even when temporary handle is created) we will now call it only when data is loaded for the first time. So instead of calling loadImageData, all providers must call loadImageDataWithGrayOrDisablement and also provide loadImageData implementation of its own. In this way we can achieve one common place to call adaptImageDataIfDisabledOrGray from.

Enabling a consistent pattern to apply disablement or graying of an Image. They are applied whenever a handle is being created from init method. Ensuring streamline usage and avoid double calls to this method.

ShahzaibIbrahim · 2025-11-19T13:40:54Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

+		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);
 			}


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

ShahzaibIbrahim · 2025-11-19T13:42:02Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

 			ImageData imageData = image.getImageData(targetZoom);
 			drawer.postProcess(imageData);
-			ImageData newData = adaptImageDataIfDisabledOrGray(imageData);
+			ImageData newData = adaptImageDataIfDisabledOrGray(imageData, styleFlag);


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.

ShahzaibIbrahim · 2025-11-19T13:45:55Z

@akoch-yatta @HeikoKlare Please review the PR and comments, It might not be in final shape but here I could use some suggestions.

github-actions · 2025-11-19T13:54:41Z

Test Results

111 files ±0 111 suites ±0 17m 48s ⏱️ +7s
4 597 tests ±0 4 582 ✅ +1 14 💤 - 1 1 ❌ ±0
282 runs ±0 281 ✅ ±0 1 💤 ±0 0 ❌ ±0

For more details on these failures, see this check.

Results for commit eb0405c. ± Comparison against base commit 63d5582.

♻️ This comment has been updated with latest results.

ShahzaibIbrahim · 2025-12-10T09:33:42Z

@akoch-yatta can you please have a look/review if you got time?

HeikoKlare

Can you please explain the underlying pattern where the application of gray/disablement is supposed to be done now? The PR description states:

In this way we can achieve one common place to call adaptImageDataIfDisabledOrGray from.

But I see calls of that method from at least:

initializeHandleFromSource
newImageData
getOrCreateImageHandleAtClosestSize

To assess if this is a good concept/systematics where to apply gray/disablement, it would be helpful to have a description of that concept.

In addition to that, the PR seems to contain unnecessary changes (e.g., addition of unused parameters, duplicate application of adaptImageDataIfDisabledOrGray) and also seems to remove crucial code (handle initialization in copy constructor).

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

HeikoKlare · 2025-12-10T10:33:05Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

 			if (imageData.isPresent()) {
-				ImageData adaptedData = adaptImageDataIfDisabledOrGray(imageData.get());
-				ImageHandle imageHandle = init(adaptedData, -1);
+				ImageHandle imageHandle = init(imageData.get(), -1);


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

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

HeikoKlare · 2025-12-10T10:34:23Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

-				ImageData data = srcImage.getImageData(imageHandle.zoom);
-				ImageData newData = applyGrayImageData(data, rect.height, rect.width);
-				init (newData, imageHandle.zoom);
+				srcImage.getImageData(imageHandle.zoom);


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?

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).

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.

HeikoKlare · 2025-12-10T10:36:47Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

+		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);


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.

ShahzaibIbrahim marked this pull request as draft November 19, 2025 13:27

ShahzaibIbrahim force-pushed the master-496-new-tech branch from 4c8ed71 to 2f08cde Compare November 19, 2025 13:34

Streamline calls to Image#adaptImageDataIfDisabledOrGray()

91c9747

Enabling a consistent pattern to apply disablement or graying of an Image. They are applied whenever a handle is being created from init method. Ensuring streamline usage and avoid double calls to this method.

ShahzaibIbrahim force-pushed the master-496-new-tech branch from 2f08cde to 91c9747 Compare November 19, 2025 13:36

ShahzaibIbrahim commented Nov 19, 2025

View reviewed changes

Implementing load logic for PlainImageDataProvider

eb0405c

ShahzaibIbrahim linked an issue Dec 10, 2025 that may be closed by this pull request

Streamline calls to Image#adaptImageDataIfDisabledOrGray() vi-eclipse/Eclipse-Platform#496

Open

HeikoKlare reviewed Dec 10, 2025

View reviewed changes

Remove unnecessary variable.

6bce6ef

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

Are you sure you want to change the base?

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

Conversation

ShahzaibIbrahim commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to test

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShahzaibIbrahim commented Nov 19, 2025

Uh oh!

github-actions bot commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

ShahzaibIbrahim commented Dec 10, 2025

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ShahzaibIbrahim commented Nov 19, 2025 •

edited

Loading

github-actions bot commented Nov 19, 2025 •

edited

Loading