Rework new Image constructors accepting a ImageGcDrawer

[HannesWell]

When implementing the ImageGCDrawer as lambda, the resulting code is better to read if the ImageGCDrawer is passed as last argument:

new Image(device, 16, 16, (gc, w, h) -> {
	gc.draw ...
})

For example at eclipse-platform/eclipse.platform.ui#2869 or in the adapted parts within SWT one can see that the current order encourages to code in way that is more verbose.

Furthermore this way the argument order also better reflects the order in which the arguments are applied:

1. an image with the given size is created.
2. the GC draws on that image

I know that changing an API after it was released is not ideal, but since it's very new I assume there are not yet many users and existing users are very likely able to adapt especially as the change is trivial. Consequently we have a real chance to remove the first shot eventually. On the other hand if we stick with the existing API we might be annoyed by its imperfection forever.
If we have this, I can take care of adopting the existing callers in the SDK.

Follow-up on

• Callback for dynamic drawing with a GC on images #1734

@HeikoKlare, @fedejeanne or anybody else, what do you think?

[github-actions]

Test Results

104 files   -    435  104 suites   - 435   5s ⏱️ - 31m 51s
 63 tests  -  4 267   63 ✅  -  4 257  0 💤  -   9  0 ❌  - 1 
210 runs   - 16 369  210 ✅  - 16 267  0 💤  - 101  0 ❌  - 1 

Results for commit 49d2fcc. ± Comparison against base commit 3670670.

This pull request removes 4267 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 comment has been updated with latest results.

[HeikoKlare]

I don't have a strong opinion on this. I agree that the proposed order would have been better from the beginning and I also agree on the "semantics" of the argument order (first specifying image creation and then image contents). I only disagree with the other argument of producing more verbose code because of the current API preventing easily inlining a multi-line lambda, as that conflicts with my understanding of clean coding practices: If there is a multi-line lambda, that should be assigned to a variable that condensely expresses its meaning. When I inline a multi-line lambda into the call of another method (or a constructor, like here), the code becomes incomprehensible anyway, no matter whether that lambda is the last argument of the call or not. That's why I would encourage to never do that anyway

So in summary, I am not sure whether the change is worth the API extension + deprecation, but at the same time I don't have a strong argument against it, in particular since the API has just been introduced and is not consumed that much yet.

[fedejeanne]

@HeikoKlare, @fedejeanne or anybody else, what do you think?

Same as Heiko: I have no strong opinion about this.

Your proposal is reasonable and I see the slight benefits. If no one objects, you may go for it @HannesWell :-) ✔️

[HannesWell]

I only disagree with the other argument of producing more verbose code because of the current API preventing easily inlining a multi-line lambda, as that conflicts with my understanding of clean coding practices: If there is a multi-line lambda, that should be assigned to a variable that condensely expresses its meaning. When I inline a multi-line lambda into the call of another method (or a constructor, like here), the code becomes incomprehensible anyway, no matter whether that lambda is the last argument of the call or not. That's why I would encourage to never do that anyway

I have to admit that I have the exact opposite opinion about it. If a lambda is only used once, most of the times I find it much cleaner to line it, because a lambda usually specifies the behavior of something and I find it more clear if it's then written in the context of the 'thing' whose behavior it defines then somewhere else. One prominent example for me is the map() operation of a Stream-pipeline. Usually I would always define the map-operation as inlined lambda, even if it has multiple lines. Then one can read and understand the code sequentially without the need to jump mentally between (code) locations.

But to stick with this case, with a inlined lambda I can define the drawer and therefore the content of the image, literally within the image-constructor and can say I create a new Image and draw this content XYZ :

Image rightCaretBitmap = new Image(display, caretWidth, lineHeight, (gc, width, height) -> {
	gc.setBackground(display.getSystemColor(SWT.COLOR_BLACK));
	gc.fillRectangle(0, 0, width, height);
	gc.setForeground(display.getSystemColor(SWT.COLOR_WHITE));
	gc.drawLine(width-1,0,width-1,height);
	gc.drawLine(0,0,width-1,0);
	gc.drawLine(width-1,1,1,1);
});

Previously it read like: I create an ImageGcDrawer that draws content XYZ and then I create a new Image and pass the just defined drawer:

ImageGcDrawer rightCaretDrawer = (gc, width, height) -> {
	gc.setBackground(display.getSystemColor(SWT.COLOR_BLACK));
	gc.fillRectangle(0, 0, width, height);
	gc.setForeground(display.getSystemColor(SWT.COLOR_WHITE));
	gc.drawLine(width-1,0,width-1,height);
	gc.drawLine(0,0,width-1,0);
	gc.drawLine(width-1,1,1,1);
};
Image rightCaretBitmap = new Image(display, rightCaretDrawer, caretWidth, lineHeight);

In the end it's of course not a game-changer, but I personally found the former more clear than the latter.

[HannesWell]

In a second commit I have now also added the suggestion from #1948 (comment), about how a Image(GC)Drawer could be extended in an alternative way. Please consider this just as a starting point for a discussion. This doesn't have to be the final result.
The idea is that simple ImageDrawers are just created as lambdas and if one wants to add post-processing and defines the style of the GC one can create special static (factory) methods that wrap the given drawer together with the post-processing/style info and as the Image class then can know this (internal) class it can check for the type and extract the additionally specified information if possible.

E.g. to create a drawer with post-processing one could then use:

Image image = new Image(display, width, height, ImageDrawer.create((gc, w, h) -> {
	gc.fillRectangle(0, 0, w, h);
	...
}, imageData -> {
	// do some post-processing
});

to create an Image with a drawer with a certain style one could use:

Image image = new Image(display, width, height, ImageDrawer.withGCStyle(SWT.LEFT_TO_RIGHT, (gc, w, h) -> {
	gc.fillRectangle(0, 0, w, h);
	...
});

or to create a drawer with a style and post-processing one could use:

Image image = new Image(display, width, height, ImageDrawer.create(withGCStyle(SWT.LEFT_TO_RIGHT, (gc, w, h) -> {
	gc.fillRectangle(0, 0, w, h);
	...
}), imageData -> {
	// do some post-processing
});

And since multiple things are then already change in this area I also wanted to suggest to replace ImageGcDrawer by an new interface ImageDrawer because I assumed the GC part is implied because I don't know another way drawing on an image than with a GC. And without GC the name is a bit simpler. But we can also realize the factory methods within the existing ImageGcDrawer interface. So that's also just an idea. :)

There are more changes to discuss

[HeikoKlare]

I generally like the proposed way of creating the drawer instances. My only two concerns/comments are:

• I am not a fan of explicit type checks, in particular when they are already used while introducing completely new concepts. At least I would expect everything regarding these type checks to be encapsulated at a single place (namely the ImageDrawer). I would also expect something like getGCStyle() to be provided on ImageDrawer rather than ExtendedImageDrawer to avoid that anyone needs to have knowledge about that one.
• I am not a fan of these overly complex inline construction concepts, but we already found that we disagree here :-) Having some complex lambda inlined into a call makes it impossible for someone not knowing the signature of the called method to understand what's going on. In the last example, in which a post-processor is passed to an ImageDrawer, you need to know about the meaning of all the different lambdas to understand this construction flow. For people not aware of all the interfaces, this means pretty high effort to understand what's going on. But to be fair, this is not an argument against the proposed APIs/factories but only about how to use them.

So in short: the changes are basically fine for me. Just be aware that this requires adaptations in at least Platform UI as well, as the deprecated APIs are already used there.

[laeubi]

+1 for having lamda as last argument and
+1 for ImageDrawer name (while I more think it is an ImageDecorator or ImageProducer)

Beside that I personally more like BuilderPatterns over constructors with many optional and/or varying variants.

Something like

Image image = Image.from(device)
  .withSize(width, height)
  .drawing((gc, w, h)-> {
        //draw on me
   }).process(imageData -> {
        // do some post-processing
   })
 .create();

that way its much easier to adjust/change without the hassle to find the "right" position for parameter X and its easier to evolve and better to read.

[fedejeanne]

• -1 on having to explicitly know about ExtendedImageDrawer (just as Heiko pointed out)
• +1 on the usage of builders if you want to hide such details
• +0 (neutral) on renaming ImageGcDrawer to ImageDrawer but please adapt the names of the parameters and variables accordingly
• +0 on changing the order of the parameters in the constructor

All in all: I like factories/builders but I don't like having to know the classes explicitly in order for them to work. Having checks saying if (drawer instanceof ExtendedImageDrawer extendedDrawer) is fine inside a factory method (or a builder) but only there and only if that factory method (or builder) is the only one that needs to know about (and instantiate) ExtendedImageDrawer.

HTH

[fedejeanne]

I'd rather not have to use static methods in here since one needs to know about the existence of ExtendedImageDrawer (just like Heiko pointed out). A more natural approach to me would be:

GC gc = new GC(image, imageGcDrawer.getGCStyle());
	try {
		imageGcDrawer.drawOn(gc, width, height);
		ImageData imageData = image.getImageData(zoom);
		imageGcDrawer.postProcess(imageData);
		return imageData;

Which means that you need to move getGcStyle() back to ImageDrawer.