Skip to content

[win32] Improve GC init for ImageGcDrawer #2110

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
May 6, 2025
Merged

[win32] Improve GC init for ImageGcDrawer #2110

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
30 changes: 27 additions & 3 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 @@ -1719,6 +1719,10 @@ private ImageHandle init(ImageData i, int zoom) {
*/
@Override
public long internal_new_GC (GCData data) {
return configureGC(data, 100);
Copy link
Contributor

@HeikoKlare HeikoKlare May 6, 2025

Choose a reason for hiding this comment

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

Just for clarification: why do we use 100 instead of DPIUtil.getNativeDeviceZoom() here? Wouldn't the latter be more consistent with existing behavior (i.e., in case monitor-specific scaling is disabled an images are drawn this way)?

Copy link
Contributor Author

@akoch-yatta akoch-yatta May 6, 2025

Choose a reason for hiding this comment

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

We know GC works better with 100% then with e.g. 125 or 175 that could be provided with DPIUtil.getNativeDeviceZoom() so I would like to prevent using DPIUtil.getNativeDeviceZoom() as default

Copy link
Contributor

@HeikoKlare HeikoKlare May 6, 2025

Choose a reason for hiding this comment

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

I was concerned that this changes behavior if now for the same calling code a GC at a different zoom is created. Isn't that the case?

Copy link
Contributor Author

@akoch-yatta akoch-yatta May 6, 2025

Choose a reason for hiding this comment

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

yes, it sure does. Before DPIUtil.getNativeZoom was used, but not for a specific reason, especially not for the GC, but just for creating the probably most suitable initial handle here So, for this default case I would argue, no value is really the correct value here. I chose 100 out of two reasons

  • drawing on the GC works best with 100% - e.g. having GEF in mind.
  • all "easy" use cases involving GC and Images should be covered with ImageGcDrawer, leaving the complex ones (like. GEF) which brings me back to the upper point

Copy link
Contributor

@HeikoKlare HeikoKlare May 6, 2025

Choose a reason for hiding this comment

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

Thank you for the clarification!
Just like in #2108 (comment), I was mistakenly thinking that the image data may be at a wrong size and that may lead to issues when drawing on them, but since the drawing happens in point coordinates it will always work.
The decision makes sense to me.

}

private long configureGC(GCData data, int zoom) {
if (isDisposed()) SWT.error(SWT.ERROR_GRAPHIC_DISPOSED);
/*
* Create a new GC that can draw into the image.
Expand All @@ -1743,9 +1747,9 @@ public long internal_new_GC (GCData data) {
data.style |= SWT.LEFT_TO_RIGHT;
}
data.device = device;
data.nativeZoom = initialNativeZoom;
data.nativeZoom = zoom;
data.image = this;
data.font = SWTFontProvider.getSystemFont(device, initialNativeZoom);
data.font = SWTFontProvider.getSystemFont(device, zoom);
}
return imageDC;
}
Expand Down Expand Up @@ -2530,7 +2534,7 @@ protected ImageHandle newImageHandle(int zoom) {
} else {
image = new Image(device, width, height, zoom);
}
GC gc = new GC(image, gcStyle);
GC gc = new GC(new DrawableWrapper(image, zoom), gcStyle);
try {
gc.data.nativeZoom = zoom;
drawer.drawOn(gc, width, height);
Expand All @@ -2544,6 +2548,26 @@ protected ImageHandle newImageHandle(int zoom) {
}
}

private class DrawableWrapper implements Drawable {
private final Image image;
private final int zoom;

public DrawableWrapper(Image image, int zoom) {
this.image = image;
this.zoom = zoom;
}

@Override
public long internal_new_GC(GCData data) {
return this.image.configureGC(data, zoom);
}

@Override
public void internal_dispose_GC(long handle, GCData data) {
this.image.internal_dispose_GC(handle, data);
}
}

@Override
Object getProvider() {
return drawer;
Expand Down
Loading