Skip to content

Conversation

@ShahzaibIbrahim
Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim commented Mar 11, 2025

Currently, when an image is instantiated using ImageDataProvider or ImageFileNameProvider, a handle is created immediately. We aim to optimize this by deferring handle creation until it is explicitly needed. To achieve this, getBounds() will return the bounds of a 100% zoomed image when called after instantiation, without requiring a handle to be created upfront. This change improves efficiency by reducing unnecessary resource allocation during image initialization.

Attaching the snippet to test the use cases after this change:

package org.eclipse.swt.snippets;

import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.widgets.*;

public class SnippetTestImage {

	final static String IMAGES_ROOT = "bin/org/eclipse/swt/snippets/";

	public static void main(String[] args) {
		imageFileNameProviderExample();
		imageDataProviderExample();
	}

	private static void imageDataProviderExample() {
		Image image;
		final Display display = new Display();
		ImageDataProvider imageDataProvider = zoom -> {
			String fileName;
			switch (zoom) {
			case 100:
				fileName = "eclipse16.png";
				break;
			case 150:
				fileName = "eclipse24.png";
				break;
			case 200:
			case 250:
				fileName = "eclipse32.png";
				break;
			default:
				return null;
			}
			return new ImageData(IMAGES_ROOT + fileName);
		};
		try {
			image = new Image(display, imageDataProvider);
			System.out.println("Image Bounds initially = " + image.getBounds());
			ImageData imgData = image.getImageData(250);
			System.out.println("Image Bounds on Demand (250) = " + imgData.height + " width = " + imgData.width);
			image.dispose();
		} catch (SWTException e) {
			System.out.println(e);
		}
	}

	private static void imageFileNameProviderExample() {
		Image image;
		final Display display = new Display();
		ImageFileNameProvider provider = zoom -> {
			String fileName;
			switch (zoom) {
			case 100:
				fileName = "eclipse16.png";
				break;
			case 150:
				fileName = "eclipse24.png";
				break;
			case 200:
			case 250:
				fileName = "eclipse32.png";
				break;
			default:
				return null;
			}
			return IMAGES_ROOT + fileName;
		};
		try {
			image = new Image(display, provider);
			System.out.println("Image Bounds initially = " + image.getBounds());
			ImageData imgData = image.getImageData(250);
			System.out.println("Image Bounds on Demand (250) = " + imgData.height + " width = " + imgData.width);
			image.dispose();
		} catch (SWTException e) {
			System.out.println(e);
		}
	}
}

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2025

Test Results

   510 files  ±0     510 suites  ±0   8m 10s ⏱️ -11s
 4 345 tests ±0   4 331 ✅ ±0   14 💤 ±0  0 ❌ ±0 
16 613 runs  ±0  16 502 ✅ ±0  111 💤 ±0  0 ❌ ±0 

Results for commit a46c317. ± Comparison against base commit f69194a.

♻️ This comment has been updated with latest results.

Copy link
Member

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

Just a NIT: width and height could be final (also in ImageGcDrawerWrapper)

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-200 branch 4 times, most recently from 2e3b140 to 7634852 Compare March 13, 2025 13:37
Copy link
Contributor

@akoch-yatta akoch-yatta left a comment

Choose a reason for hiding this comment

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

Looks good to me now

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-200 branch 3 times, most recently from f768700 to 969155c Compare March 14, 2025 16:03
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

Looks good to me as well. Just minor comments from my side.

@fedejeanne
Copy link
Member

Unintentional, I assume. Undo please:

grafik

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-200 branch 5 times, most recently from e2753b3 to 15f9c8c Compare March 17, 2025 12:33
fedejeanne
fedejeanne previously approved these changes Mar 17, 2025
Copy link
Member

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

LGTM

@fedejeanne
Copy link
Member

The code looks fine but some tests fail because of leaked resources. I am retriggering tests. @ShahzaibIbrahim stay tuned

@fedejeanne fedejeanne dismissed their stale review March 17, 2025 14:32

Retriggering tests because of leaked resources

@fedejeanne
Copy link
Member

The leaks are still there so it's definitely not a fluke @ShahzaibIbrahim . Here's the stack trace:

java.lang.Error: SWT Resource was not properly disposed
	at org.eclipse.swt.graphics.Resource.initNonDisposeTracking(Resource.java:184)
	at org.eclipse.swt.graphics.Resource.<init>(Resource.java:121)
	at org.eclipse.swt.graphics.Image.<init>(Image.java:593)
	at 

See https://github.com/eclipse-platform/eclipse.platform.swt/actions/runs/13899543146/job/38895976230?pr=1898#step:7:3980

@ShahzaibIbrahim
Copy link
Contributor Author

@fedejeanne resource leak is fixed.

@fedejeanne fedejeanne merged commit 4184369 into eclipse-platform:master Mar 18, 2025
14 checks passed
@fedejeanne fedejeanne deleted the master-200 branch March 18, 2025 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create handles for ImageDataProvider and ImageFilenameProvider on demand

4 participants