Community
Participate
Working Groups
Created attachment 262541 [details] Minimal example I'm trying to modify an existing Image by manipulating individual pixels. The Image was created using an ImageFileNameProvider, therefore has access to multiple .png files (.png, @2x.png). To make sure the manipulated Image is also High-DPI aware, I am using an Image created using an ImageDataProvider. However, there is no API to access the data of the source Image in a specific zoom level. I attached a minimal example to explain things. I think there are currently two things missing to satisfy use cases such as this: 1. Provide a public API to access ImageData in arbitrary zoom factors: Image.getImageData(zoom) 2. Provide a public API to access the providers of an Image, if they exist: Image.getImageFileNameProvider() Image.getImageDataProvider(). Or am I missing something obvious here, that could get my attached example to work without the hack using reflection?
I think ImageData belongs to SWT, not UI
(In reply to Sebastian Ratz from comment #0) > Created attachment 262541 [details] > Minimal example > > I'm trying to modify an existing Image by manipulating individual pixels. > > The Image was created using an ImageFileNameProvider, therefore has access > to multiple .png files (.png, @2x.png). > > To make sure the manipulated Image is also High-DPI aware, I am using an > Image created using an ImageDataProvider. > > However, there is no API to access the data of the source Image in a > specific zoom level. > > I attached a minimal example to explain things. > > I think there are currently two things missing to satisfy use cases such as > this: > > 1. Provide a public API to access ImageData in arbitrary zoom factors: > Image.getImageData(zoom) > > 2. Provide a public API to access the providers of an Image, if they exist: > Image.getImageFileNameProvider() > Image.getImageDataProvider(). > > Or am I missing something obvious here, that could get my attached example > to work without the hack using reflection? Hi Sebastian, Below 2 APIs should be sufficient, if you plan to do Pixel manipulation: Image#getImageDataAtCurrentZoom() Image#getBoundsInPixels() If you agree, we can close this bug.
These APIs are already part of 4.6 release: https://www.eclipse.org/eclipse/news/4.6/M6/#high-dpi-api
I think it is not sufficient: Like I demonstrated in my example code: 1) In order to create a DPI aware Image, it has to be based on an ImageDataProvider. 2) In order to implement an ImageDataProvider, the method get "public ImageData getImageData(int zoom)" has to be implemented. Now there is no way to satisfy this for all circumstances: a) CURRENT zoom is 100, getImageData(100) is called: This works via getImageDataAtCurrentZoom() b) CURRENT zoom is 200, getImageData(200) is called: This works via getImageDataAtCurrentZoom() c) CURRENT zoom is 100, getImageData(200) is called: No way to get pixel data at 200. d) CURRENT zoom is 200, getImageData(100) is called: This could technically work via the old API, however the result is then an Image with 32x32 at 100% zoom, which is wrong, too, since it will be shown too large. Cases c) and d) actually do happen. Therefore, Image.getImageData(int zoom) should be made public. Also: getImageData(int zoom) does a lot of scaling via the DPIUtil. If I know that the source image was implemented via a ImageFileNameProvider or a ImageDataProver, too, getting direct access to those underlying providers would simplify things a lot, since these providers already have methods to get the ImageData at specific zoom levels (see attachment in first message). I hope this makes things a bit clearer.
The status of this bug is 'assigned' but it's still assigned to platform-swt-inbox. Doesn't seem like anyone's working on it atm? Status -> New
(In reply to Niraj Modi from comment #2) > Hi Sebastian, > Below 2 APIs should be sufficient, if you plan to do Pixel manipulation: > Image#getImageDataAtCurrentZoom() > Image#getBoundsInPixels() > > If you agree, we can close this bug. How should these API be sufficient? Can you please show this by adapted the example provided by Sebastian by only using these API methods.
Yes, this is a real problem. I just wanted to file a duplicate bug with title "Replace Image#getImageDataAtCurrentZoom() with #getImageData(int zoom)". The problem is that the notion of "current" zoom doesn't make sense on the Mac, since a single Image instance can be used in two different shells that are placed on separate monitors with different zoom levels. On GTK and Windows, SWT currently lacks support for setups with multiple monitors running at different resolutions, but this will eventually have to be implemented as well. Image#getImageData(int zoom) should implement ImageDataProvider#getImageData(int zoom), and getImageData(100) should be equivalent to getImageData().
We need to implement this for 4.7M7 and will need PMC approval for the API addition. See bug 513637 for the reasons. Unfortunately, the underlying problems were only fully understood after M6.
New Gerrit change created: https://git.eclipse.org/r/93242
New Gerrit change created: https://git.eclipse.org/r/93243
New Gerrit change created: https://git.eclipse.org/r/93245
> https://git.eclipse.org/r/93242 I've implemented the new API and verified it on Cocoa. Didn't find any issues. Needs polish, and implementation on other platforms. Fixing the ugly implementation of CTabFolder#createButtonImage(Display, int) needs some more head-scratching.
I've started to look at the implementation on GTK and thereby realized that clients like CompositeImageDescriptor#getZoomedImageData(ImageDataProvider) will still have to implement auto-scaling on their end unless Image#getImageData(int) already does that (i.e. promises to always return a non-null ImageData). Since SWT already implements auto-scaling when one of the old Image constructors is used, it would be good to offer clients a way to use the same scaling algorithm (which is even configurable on some platforms, and which may be improved over time). New proposed spec: /** * Returns an {@link ImageData} for the given zoom level based on the * receiver. * <p> * Note that this method is mainly intended to be used by custom * implementations of {@link ImageDataProvider} that draw a composite image * at the requested zoom level based on other images. For custom zoom * levels, the image data may be an auto-scaled version of the native image * and may look more blurred or mangled than expected. * </p> * <p> * Modifications made to the returned {@code ImageData} will not affect this * {@code Image}. * </p> * * @param zoom * The zoom level in % of the standard resolution (which is 1 * physical monitor pixel == 1 SWT logical pixel). Typically 100, * 150, or 200. * @return an <code>ImageData</code> containing the image's data and * attributes at the given zoom level * * @exception SWTException <ul> * <li>ERROR_GRAPHIC_DISPOSED - if the receiver has been disposed</li> * <li>ERROR_INVALID_IMAGE - if the image is not a bitmap or an icon</li> * </ul> * * @since 3.106 */ public ImageData getImageData(int zoom) {
(In reply to Markus Keller from comment #13) > Since SWT already implements auto-scaling when one of the old Image > constructors is used, it would be good to offer clients a way to use the > same scaling algorithm (which is even configurable on some platforms, and > which may be improved over time). DPIUtil#AutoScaleImageDataProvider class offers a clean way to consume SWT scaling algorithm(currently used internally), we didn't had a known use-case for this Class during Neon, so I had raised bug 494264 then. Additional possibilities with this class: - w.r.t. to API requirement for bug 513844(which is scoped only for new HiDPI Image constructors) we can return AutoScaleImageDataProvider instance for old Image constructors.
New Gerrit change created: https://git.eclipse.org/r/93727
(In reply to Eclipse Genie from comment #15) > New Gerrit change created: https://git.eclipse.org/r/93727 Hi Markus, Please try windows implementation of Image#getImageData(int zoom). Also replace getImageDataAtCurrentZoom() method usage in SWT code base(only outside of Image class). Now using new API we need to pass the currentZoom explicitly using DPIUtil. We can make this easy(without using DPIUtil), if we introduce below convention: Image#getImageData (-1) // -1 will internally map to current device zoom.
(In reply to Niraj Modi from comment #16) > We can make this easy(without using DPIUtil), if we introduce below > convention: > Image#getImageData (-1) // -1 will internally map to current device zoom. I don't think we need this additional convention. Certainly not as API, since "current" is not well-defined in multi-monitor setups. Keeping explicit internal references to DPIUtil.getDeviceZoom() will help us identify the places to fix. The only place where client code needs to know a "current" zoom level is when loading/saving images from/to the clipboard or an external file using ImageTransfer or ImageLoader. For those cases, bug 480639 proposes a new API Monitor#getZoom() that would tell clients the right zoom level. I'm not pushing for adding that API in 4.7, since we don't have any use case for it in the SDK that would allow to validate it. The current implementation of the ClipboardExample doesn't work well at 200%: When you e.g. take a screenshot (using the operating system) and then load the screenshot file, then the image is rendered twice as big as it should be. I don't think the ClipboardExample should try to scale images. Applications that deal with user-supplied bitmap images should typically either - use the resolution that was stored in the image (that's what e.g. macOS 10.12's Preview.app does when you open a screenshot file), or - render bitmap images so that 1 image pixel corresponds to 1 native monitor pixel.
https://git.eclipse.org/r/#/c/93242/3 is good to go on Cocoa and GTK. I've added a test case for the new API. The test is green at 100% and at 200% zoom on both platforms. It fails on Gerrit due to a setup issue (known bug). Dani, please approve the API change in that Gerrit. Niraj, please adjust the Windows implementation to the API from my Gerrit. I saw that your implementation should also support dynamic adjustment of the native zoom level. I can't test that. If you see problems with that scenario on GTK, then please fix my patch. To test the new API in Eclipse (e.g. decorated JDT icons), pull the Gerrits for platform.ui and jdt.ui. Before pushing my Gerrit to master, please fix the commit message.
(In reply to Markus Keller from comment #18) > Dani, please approve the API change in that Gerrit. Looks good to me but the official process is to request the change on the eclipse-pmc mailing list, so that other PMC members can also vote.
(In reply to Markus Keller from comment #18) > https://git.eclipse.org/r/#/c/93242/3 is good to go on Cocoa and GTK. I've > added a test case for the new API. The test is green at 100% and at 200% > zoom on both platforms. It fails on Gerrit due to a setup issue (known bug). > > Dani, please approve the API change in that Gerrit. > > Niraj, please adjust the Windows implementation to the API from my Gerrit. I > saw that your implementation should also support dynamic adjustment of the > native zoom level. I can't test that. If you see problems with that scenario > on GTK, then please fix my patch. > > To test the new API in Eclipse (e.g. decorated JDT icons), pull the Gerrits > for platform.ui and jdt.ui. > > Before pushing my Gerrit to master, please fix the commit message. IMHO, we should leverage the image providers(if present) in GTK implementation as well, do you see any down-side of this ? On Windows7 we need to log off to apply/update the native zoom. Win10 has some(but limited) support for dynamic zoom(as tested last during Neon time-frame) Lets finalize on this by next week.
(In reply to Niraj Modi from comment #20) > IMHO, we should leverage the image providers(if present) in GTK > implementation as well, do you see any down-side of this ? As I already said, I would support such a change, but I can't write nor test it. On my Ubuntu 16.04 install, changing the scale in "System Settings > Displays" only produces screen cheese (only text in Labels gets enlarged, but layouts are not adjusted, see bug 490321).
(In reply to Markus Keller from comment #21) > (In reply to Niraj Modi from comment #20) > > IMHO, we should leverage the image providers(if present) in GTK > > implementation as well, do you see any down-side of this ? > > As I already said, I would support such a change, but I can't write nor test > it. On my Ubuntu 16.04 install, changing the scale in "System Settings > > Displays" only produces screen cheese (only text in Labels gets enlarged, > but layouts are not adjusted, see bug 490321). Done, I have updated the GTK code in the your patch, which now uses provider(if present) and also revised Windows changes(minor tweaks). Now both GTK and Windows API changes look same and JUnits are also passing on both, we should be good now.
Gerrit change https://git.eclipse.org/r/93727 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=a200595cf5e1478d953549e08dbe4d3495f493e4
Gerrit change https://git.eclipse.org/r/93242 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=eb6b62d1e1b5f529233b396ff15c7f51c308bcc2
The recent patches caused build failures on Hudson: https://hudson.eclipse.org/platform/job/eclipse.platform.swt-Gerrit/4380/console Due to this, Hudson fails for other patches also. [INFO] BUILD FAILURE [DEBUG] Closing connection to remote [ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:1.1.0-SNAPSHOT:compile (default-compile) on project org.eclipse.swt.tests: Compilation failure: Compilation failure: [ERROR] /jobs/genie.platform/eclipse.platform.swt-Gerrit/workspace/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java:[815] [ERROR] image.getImageData(zoom); [ERROR] ^^^^^^^^^^^^ ... [ERROR] The method getImageData(int) from the type Image is not visible [ERROR] /jobs/genie.platform/eclipse.platform.swt-Gerrit/workspace/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java:[823] [ERROR] ImageData imageDataAtZoom = image.getImageData(zoom); [ERROR] ^^^^^^^^^^^^ .... [ERROR] The method getImageData(int) from the type Image is not visible [ERROR] 5 problems (5 errors) [ERROR] -> [Help 1] The errors do not occur locally, nor are there errors with local jUnits. Please kindly resolve the build issue or revert the patches until hudson is running again, so that other patches can be tested by hudson. Thank you
(In reply to Leo Ufimtsev from comment #25) > The errors do not occur locally, nor are there errors with local jUnits. > > Please kindly resolve the build issue or revert the patches until hudson is > running again, so that other patches can be tested by hudson. Please see bug 507602.
(In reply to Dani Megert from comment #26) > (In reply to Leo Ufimtsev from comment #25) > > The errors do not occur locally, nor are there errors with local jUnits. > > > > Please kindly resolve the build issue or revert the patches until hudson is > > running again, so that other patches can be tested by hudson. > > Please see bug 507602. The tests for gerrit builds failed as they used this new API which was not yet part of the I-build. I've verified that the gerrit build is running fine now.
Thanks Lakshmi and Niraj, I think we're done here. Clients have already successfully adopted this API.
Verified on Mac. Build id: I20170509-2000
Verified on Win7 using Build id: I20170512-0500 Created below bug 516639 for fixing the JavaDocs of deprecated APIs Image#getBoundsInPixels() and Image#getImageDataAtCurrentZoom() Also create bug 516640 for evaluating need for Image#getBounds(int zoom) API
