Skip to content
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

Jump to bottom

Callback for dynamic drawing with a GC on images #1734

Merged
merged 1 commit into from
Jan 19, 2025
Merged

Callback for dynamic drawing with a GC on images #1734

merged 1 commit into from
Jan 19, 2025

Conversation

akoch-yatta
Copy link
Contributor

@akoch-yatta akoch-yatta commented Jan 16, 2025 •
edited by fedejeanne
Loading

This PR adds a new interface to be used to provide dynamic drawing on GCs created with Images. It is mostly targeted at solving the following use cases:

Image image = new Image (display, width, height);
GC gc = new GC (image);
// draw on the gc
otherGc.drawImage(image, ...);

or in combination with a ImageDataProvider

return new Image(display, (ImageDataProvider) zoom -> {
     Image image = new Image (display, width, height);
     GC gc = new GC (image);
     // draw on the gc
     gc.dispose ();

     final ImageData imageData = image.getImageData (zoom);
     image.dispose();
     return imageData;
});

Both use cases have the problem, that they do not work properly in a multi-zoom setting. Not properly working means that there will be a destructive up- or downscaling on the created image data in the background. This PR moves the initialization of the GC into the implementation of each image and can therefor be handled properly for all platforms.

I saw those use cases with two different settings: with and without transparency necessary. To address the use case with transparency we added the TransparancyColorImageGcDrawer as internal class to be used for CTabFolder::createButtonImage.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 16, 2025 •
edited
Loading

Test Results

   494 files  ±0     494 suites  ±0   9m 8s ⏱️ +22s
 4 326 tests +1   4 308 ✅ +1   18 💤 ±0  0 ❌ ±0 
16 536 runs  +4  16 388 ✅ +4  148 💤 ±0  0 ❌ ±0 

Results for commit 0fb919b. ± Comparison against base commit 6e219e1.

♻️ This comment has been updated with latest results.

@akoch-yatta akoch-yatta force-pushed the add-gc-image-provider branch 8 times, most recently from 3cd4ddf to 6607439 Compare January 17, 2025 10:54
@akoch-yatta akoch-yatta mentioned this pull request Jan 17, 2025
Merged
This was linked to issues Jan 17, 2025
Min/Max buttons are not scaled correctly vi-eclipse/Eclipse-Platform#176
Closed
Add ImageDrawingProvider interface for Images vi-eclipse/Eclipse-Platform#143
Closed
@akoch-yatta akoch-yatta force-pushed the add-gc-image-provider branch 2 times, most recently from 593e80a to d939382 Compare January 17, 2025 13:44
@akoch-yatta akoch-yatta marked this pull request as ready for review January 17, 2025 13:45
HeikoKlare
HeikoKlare reviewed Jan 17, 2025
View reviewed changes
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.

The proposed API and the according changes look reasonable to me.
I have so far taken a look at the implementation excluding the Image adaptations for MacOS and Linux.
I will also have a look at the other implementations afterwards and of course need to test the change.

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageGcDrawer.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageGcDrawer.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageGcDrawer.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageGcDrawer.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageGcDrawer.java Outdated Show resolved Hide resolved
...e.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java Outdated Show resolved Hide resolved
...e.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved
@akoch-yatta akoch-yatta force-pushed the add-gc-image-provider branch 2 times, most recently from ad2d315 to a8f9f5b Compare January 17, 2025 14:51
HeikoKlare
HeikoKlare reviewed Jan 17, 2025
View reviewed changes
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.

I've now tested the change on Windows and MacOS, also together with the consuming Platform UI PR eclipse-platform/eclipse.platform.ui#2722.
Both platform implementations work fine. Initializing the UI with existing (non-monitor-specific) scaling capabilities at different zoom levels (100%, 200% and on Windows in between) shows behavior that is consistent to before the change.
Enabling monitor-specific scaling on Windows shows improved behavior. I particularly tested with higher resolution primary monitor and lower resolution secondary monitor, moving between these monitors and changing the primary monitor zoom at runtime.

I have reviewed again after recent changes and only have nitpicky comments. I still need to have a look at and test the Linux implementation.

....eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/TransparancyColorImageGcDrawer.java Outdated Show resolved Hide resolved
....eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/TransparancyColorImageGcDrawer.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved
examples/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet382.java Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved
@akoch-yatta akoch-yatta force-pushed the add-gc-image-provider branch from a8f9f5b to a45010a Compare January 17, 2025 17:30
@HeikoKlare HeikoKlare force-pushed the add-gc-image-provider branch from a45010a to 428cd54 Compare January 18, 2025 13:33
HeikoKlare
HeikoKlare reviewed Jan 18, 2025
View reviewed changes
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.

I've now also taken a look at the GTK code and only have minor comments. I have amended the PR commit with the proposed changes. Please see if they are find and otherwise feel free to revert. Except for that (and potentially hashCode/equals), the PR is now fine for me.

I have tested the behavior on Linux/GTK (Ubuntu 24.04.1) and found no regression with respect to existing behavior. I tested with two monitors at different zoom values, both 100% and 200%, as well as different combinations of fractional scaling and changes between them. I found that together with eclipse-platform/eclipse.platform.ui#2722 we now get the LineNumberRuler scaling updated on DPI changes on GTK.
Changing from 100% to 150% yields the following results:

Before the changes

image

After the changes

image

@HeikoKlare HeikoKlare force-pushed the add-gc-image-provider branch 2 times, most recently from 339fcd3 to fde70a8 Compare January 18, 2025 17:10
@akoch-yatta akoch-yatta force-pushed the add-gc-image-provider branch 3 times, most recently from cada089 to ed4046e Compare January 18, 2025 19:28
@HeikoKlare HeikoKlare force-pushed the add-gc-image-provider branch from ed4046e to 0fb919b Compare January 19, 2025 14:20
@akoch-yatta @HeikoKlare
Callback for dynamic drawing with a GC on images
02ee164 
This commit contributes a new interface that can to used to initialize
images with. The ImageGcDrawer interface should be used to replace the
common use case of images to be used as the pane for a GC to draw on.
This usecase leads to issues with the multi-zoom-support added to the
win32 implementation, but can lead to scaling artifacts on other
platforms as well, if the usages leads to scaling ofImageData.
@HeikoKlare HeikoKlare force-pushed the add-gc-image-provider branch from 0fb919b to 02ee164 Compare January 19, 2025 15:11
@HeikoKlare HeikoKlare merged commit c493ae8 into eclipse-platform:master Jan 19, 2025
13 checks passed
@HeikoKlare HeikoKlare deleted the add-gc-image-provider branch January 19, 2025 15:21
fedejeanne
fedejeanne reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

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

On a post-merge (rather superficial) analysis I only found 1 possible issue: the default method in the newly introduced public interface ImageGcDrawer.

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageGcDrawer.java Show resolved Hide resolved
@HeikoKlare HeikoKlare mentioned this pull request Jan 27, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedejeanne fedejeanne fedejeanne left review comments

@HeikoKlare HeikoKlare HeikoKlare approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Min/Max buttons are not scaled correctly Add ImageDrawingProvider interface for Images
3 participants
@akoch-yatta @HeikoKlare @fedejeanne