Introduce a new Image(Display,Point) constructor

[ptziegler]

The deprecation of the Image(Display,Rectangle) constructor in 1d59663 introduces a lot of verbosity.

Where one could previously create an image via:

new Image(display, widget.getBounds())

One now has to store the bounds in a local variable and then call:

new Image(display, r.width, r.height)

This approach is also error prone, as one might easily mix up the order of parameters. With this new constructor, this problem can be avoided as one can simply call:

new Image(display, widget.getSize())

[github-actions]

Test Results

  173 files   -  3    173 suites   - 3   25m 13s ⏱️ - 1m 50s
4 670 tests ± 0  4 647 ✅  -  1  23 💤 +1  0 ❌ ±0 
  463 runs   - 18    458 ✅  - 17   5 💤  - 1  0 ❌ ±0 

Results for commit bbdf959. ± Comparison against base commit 9412c36.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.

org.eclipse.swt.tests.gtk.snippets.Bug336238_ShellSetBoundFailTest ‑ testSetBounds

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_graphics_Image ‑ test_ConstructorLorg_eclipse_swt_graphics_DeviceLorg_eclipse_swt_graphics_Point

This pull request skips 1 test.

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Shell ‑ test_activateEventSend

[HeikoKlare]

I understand the wish for creating an image from a given size without the need to decompose an according size object. Still I am hesitant to add such a constructor for at least the following reasons:

• The existing amount of constructors is a burden already. Nowadays, one would rather create factories or builders for types of a library/framework. In particular, convenience constructors (like the proposed one and the existing one taking a Rectangle) are better placed in factories / factory methods.
• Constructing an empty image is something that should be avoided as far as possible anyway. Such images cannot be scaled "properly"/sharply, so it should only be used for, e.g., screenshot functionalities. In all other cases, the "dynamic" image types based on ImageDataProvider, ImageFileNameProvider, ImageGcDrawer etc. should be used.

Do you have references where the pattern ...

new Image(display, widget.getBounds())

is necessary and cannot/should not be replaced with, e.g., an ImageGcDrawer?

In case we agree that such a construction pattern is still needed, I propose to rather do one of the following:

• Undeprecate the Rectangle constructor: in case that one is sufficient, we probably better preserve an existing constructor instead of introducing an additional, very similar one, as we would then also avoid the potential complications when finally removing that constructor
• Provide the convenience construction functionality as a factory / factory method: we have already discussed the idea of constructing resource with factories (that are OS-independent common code) instead of constructors at some other place as well. It's probably nothing we can enforce full migration to, but at least for new code (or for new ways of construction), we could use it. So we could either have an Image#create(Device, Point) factory method or a new common ImageFactory class or the like which only needs to implement that method once instead of multiple times for every OS.

[ptziegler]

Do you have references where the pattern ...

new Image(display, widget.getBounds())

is necessary and cannot/should not be replaced with, e.g., an ImageGcDrawer?

There are probably several reasons, but quite often, it is simply not needed. I'm also not sure whether using an ImageGcDrawer (which is even more verbose) is the correct solution, when the goal is to reduce the amount of bloat.

At least for my use case, it generally revolves around the idea of taking a screenshot of a widget. For example:

Control c = ...
Image img = new Image(null, c.getBounds());
GC gc = new GC(img);
c.print(gc);
gc.dispose();

This generally doesn't make sense with an ImageGcDrawable because I can't guarantee that the widget is scaled at the same time as the image (e.g. when the image is moved to a different monitor). So even if the image becomes larger, the source does not.

More broadly speaking, I would never use an ImageGcDrawable when I don't have a way to provide the image data at different zoom levels (PNGs as another example). Or also if the widget is short-lived and might not exist after the initial screenshot was taken.

Undeprecate the Rectangle constructor: in case that one is sufficient, we probably better preserve an existing constructor instead of introducing an additional, very similar one, as we would then also avoid the potential complications when finally removing that constructor

The constructor is not only deprecated but also marked as "for removal". Which I assume was done for a very good reason? At least my understanding is that it was done because it doesn't make much sense to use a Rectangle, when the x and y coordinates are never used.

I don't have a strong opinion on how this instance is created, as long as it's something that can be easily written down. Be it via a new constructor, a factory method or whatever else is fine by me, as long as it can be written down without any additional overhead.

[ptziegler]

So we could either have an Image#create(Device, Point) factory method or a new common ImageFactory class or the like which only needs to implement that method once instead of multiple times for every OS.

We already have those static factory methods in the Rectangle. For the sake of consistency, that sounds like the better solution. But then of course the Image class needs to be restructured so that it can actually be moved to the common project...

[HeikoKlare]

Thank you for the detailed information. The use case obviously makes sense and I agree that in such a case of just taking a screenshot, a "dynamic" image construction (e.g., via ImageGcDrawer) does not make sense.

Actually, when taking a look into the PR introducing the deprecation (for removal), we had a short discussion about that exact use case: #2088 (comment)
And it was mentioned that there should probably be a specific API for taking such screenshots, for which a draft already exists:

• Introduce simple API to take a screenshot #2104

The constructor is not only deprecated but also marked as "for removal". Which I assume was done for a very good reason? At least my understanding is that it was done because it doesn't make much sense to use a Rectangle, when the x and y coordinates are never used.

That's correct, the Rectangle constructor does not make so much sense because x/y are ignored anyway. It seems to have been more a convenience for the exact case you describe, that you can simply pass a bounds rectangle to create an image of that size. If I am not mistaken, the decision to directly mark it for removal was to have the chance to actually remove it after 2 years of deprecation. In case of raised complains or concerns (such as here), we could remove that deprecation (even the one for removal) again. But I see that this has the drawback that consumer might migrate away already (because they think they are forced to), but potentially unnecessary if the deprecation is removed again.

So yes, the Rectangle constructor is questionable, but still I proposed to remove the deprecation as an alternative as it would be the easiest thing to do:

• preserve compatibility of existing consumers that cannot / do not want to migrate and might break otherwise
• no need to think about different ways of object construction now (such as factories in OS-common code)
• no need to introduce an even new constructor to maintain (and then removing a one could have stayed instead which potentially breaks remaining consumers)

@HannesWell since you were involved in the discussion on deprecation back then and since you have already drafted the screenshot API functionality (https://github.com/eclipse-platform/eclipse.platform.swt/pull/2104/files), do you have an opinion on this?