Allow GC to draw images in arbitrary sizes

[Michael5601]

No description provided.

[github-actions]

Test Results

  219 files   -    327    219 suites   - 327   7m 21s ⏱️ - 22m 26s
4 370 tests  -     37  4 312 ✅  -     78  58 💤 +41  0 ❌ ±0 
4 517 runs   - 12 196  4 459 ✅  - 12 127  58 💤  - 69  0 ❌ ±0 

Results for commit 73d7ed4. ± Comparison against base commit 1a1f0c2.

This pull request removes 37 tests.

AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_dollarSign
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_emptyString
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letterA
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16LE_null
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_AsciiLetters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_Asciiletter
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_LotsOfLetters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letter
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letters
…

This pull request skips 43 tests.

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_StatusTextListener_hoverMouseOverLink[browser flags: 0]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color[browser flags: 0]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_CCombo ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_CLabel ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_CTabFolder ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_StyledText ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Button ‑ test_setForegroundAlphaCheckButton
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Button ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Button ‑ test_setForegroundAlphaRadiokButton
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Canvas ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
…

♻️ This comment has been updated with latest results.

[HeikoKlare]

I have left some initial comments from taking a first look at the code.

[HeikoKlare]

I do not understand why this method is needed. Every image will be capable of providing a (potentially temporary) handle with the data represented at a specific size (in the worst case by just scaling the image data from the best fitting zoomed image). So there should be no need to any case distinction based on whether an image is in some way size aware or not.

[Michael5601]

I need a distinction in two GC methods that are both called drawImage().

First one:
In case of a size aware image provider a temp handle will be generated to draw on the GC. Otherwise the method Image.win32_getHandle() is called. I inserted a case distinction here.

Second one:
In the method drawImage(Image image, int srcX, int srcY, int srcWidth, int srcHeight, int destX, int destY, int destWidth, int destHeight, int imageZoom, int scaledImageZoom) the bounds of the image are scaled with a scaledImageZoom. If I understand it correctly this can lead to rounding errors that need a workaround to work (In a comment this is stated as a hack). To avoid this behaviour, when we can directly use the targetSize to draw the image on the GC, I inserted a case distiction by using image.createdWithTargetSize() which is using imageProvider.isSizeAware().

[HeikoKlare]

The goal of adding the capability of an image to draw at a specific size will also be to get rid of this special handling inside GC. A GC should not care about resizing and finding the best fitting zoom handle but just tell the Image instance to provide a handle at a specific size, such that it can draw at that exact size.

[Michael5601]

I agree but such a change needs further discussion. If I understand i correctly we want to get rid of the zoom based parts.

For the first case this would mean that every image is drawn with a temp handle because to retrieve a handle with Image.win32_getHandle(srcImage, imageZoom) we need to provide a zoom.

For the second case it is more complicated:
When drawing on the GC an imageZoom is calculated and a gcZoom is retrieved. In my testing the gcZoom was always equivalent to my native monitor zoom (125). Before drawing, this zoom is used to calculate the size of the image that should be drawn. This size is different from the destWidth and destHeight from the caller. For example if gc.drawImage is called with destWidth == 5 and destHeight == 20, the drawing will be performed with the width 6 (5 * 125% zoom) and 25 (20 * 125% zoom). I don't understand why this is the case as I would expect this method to directly draw in the called size.

For my new functionality which can directly draw images at target size I just ignored the calculated zooms and all the extra logic needed. This is why I needed the conditional statement in the first place.

If we want to get rid of this special handling we can delete the zoom based parts but this also means that images will be drawn in a different size than before.

Please correct me if I am wrong somewhere. The current functionality is not easy to comprehend.

[HeikoKlare]

What's the difference of newImageData and loadImageData? Shouldn't that be combined to one method that returns the image data at the appropriate size?

[Michael5601]

well it is also implemented in seperate methods when using a zoom factor instead of a target size.
newImageData() calls loadImageData() to get the ImageData and afterwards performs a scaling and/or adapts the imageData to look disabled.

The method loadImageData() is overridden in the ImageDataProviderWrapper and ImageFileNameProviderWrapper to perform the actual loading.

As stated the existing implementation with zoom factor does the same but nested in methods like initializeHandleFromSource().

[HeikoKlare]

It makes sense to have a template method that calls a factory method (loadImageData()) that is implemented by subclasses and always does the required image data scaling on top of the loaded image data. What I maybe don't get then is why AbstractImageProviderWrapper overwrites the template method instead of the factory method. It seems like that wrapper will return faulty data from newImageData() then, because its fallback logic just returns the 100% zoomed data even if data for a different size is requested. To avoid such a mistake, I propose to make the newImageData() template method final and only allow specializations of the loadImageData() factory method.