Add getGcStyle() method to accommodate styling

[ShahzaibIbrahim]

GC can be initialize with GC(drawable) or GC(drawable, style). Currently Image(Device, ImageGcDrawer, height, width) doesn't consider style attribute while initializing GC. We need to create a new constructor for Image e.g. Image(Device, ImageGcDrawer, height, width, style) for which later GC inside ImageGcDrawer is initialized considering the style provided to it.

Now from implementation side, you can override getGcStyle() to provide styling in GC e.g. LEFT_TO_RIGHT, etc

Consumer usage

Before this change, there was no way for consumer to provide styling using ImageGcDrawer.

final ImageGcDrawer imageGcDrawer = (gc, width, height) -> {
  gc.fillRectangle(0, 0, 16, 16);
};
 Image = new Image(Display.getDefault(), imageGcDrawer, 1, 1);
}

Now with this change. Consumer can define the styling with which GC will be initialized later in ImageGCDrawerWrapper.

final ImageGcDrawer imageGcDrawer = new ImageGcDrawer() {
  @Override
  public void drawOn(GC gc, int iWidth, int iHeight) {
    gc.fillRectangle(0, 0, iWidth, iHeight);
 }

  @Override
  public int getGcStyle() {
    return SWT.LEFT_TO_RIGHT;
  }
};
 Image = new Image(Display.getDefault(), imageGcDrawer, 1, 1);
}

Required by

• Replace usages of new Image(device, width, height) eclipse-jdt/eclipse.jdt.ui#2116

[github-actions]

Test Results

   539 files  + 6     539 suites  +6   23m 29s ⏱️ +5s
 4 330 tests +37   4 320 ✅ +35  10 💤 +3  0 ❌  - 1 
16 407 runs  +37  16 309 ✅ +35  98 💤 +3  0 ❌  - 1 

Results for commit 9a14355. ± Comparison against base commit 74c39bf.

♻️ This comment has been updated with latest results.

[HeikoKlare]

The change looks basically fine. Can you please:

• correct the formatting and
• add a consumer that demonstrates necessity for the extension?

[akoch-yatta]

@ShahzaibIbrahim Is there a PR with an implementation that is overriding this method to utilize it?

[fedejeanne]

@ShahzaibIbrahim Is there a PR with an implementation that is overriding this method to utilize it?

eclipse-jdt/eclipse.jdt.ui#2116

[HannesWell]

Sorry for commenting after the fact, but I think we could avoid the extra method, if we introduce a static factory that besides and ImageDrawer accepts the style and returns a special implementation. In the Image class we could then check for that special calls and get the style from it, for example:

public interface ImageGcDrawer {
...
	public static ImageGcDrawer withGCStyle(int style, ImageGcDrawer drawer) {
		return new ExtendedImageDrawer(drawer, style);
	}
}

This way one can still use a lambda if a style is to be set and we avoid an extra method on the interface itself (of course we get the factory for it).

If you think this would be better, I can create a follow-up PR for it.

[fedejeanne]

@HannesWell thank you for the suggestion!
While I usually like factory methods and builders, I don't think that coding style matches the current style in SWT and I'm worried that it would result counterintuitive for consumers. I did a quick search for these methods names:

• with*
• using*
• builder*
  ... and came up (almost) empty

Is there any downside with the current approach or is it a matter of using newer coding practices?

(Disclaimer: I would have even gone with a builder pattern combined with lambdas so one could extend the returned instance by chaining calls together and providing new functionalities (in form of lambdas). But as I was writing this, the word "overkill" kept popping up in my head 😅)

[HeikoKlare]

If you think this would be better, I can create a follow-up PR for it.

I generally like the idea. What I currently don't see is how we could realize the idea without type switches. The ImageGcDrawer is consumed by Image. There, a GC is created, for which the GC style is required. So Image either has to retrieve the style from the ImageGcDrawer (making it necessary to check for a specific subtype) or it has to defer the responsibility for creating the GC to the ImageGcDrawer, which can also only be provided in the specific extended subclass (making it necessary to check for a specific subtype as well). Do you see a solution for this, @HannesWell?

[HannesWell]

Is there any downside with the current approach or is it a matter of using newer coding practices?

It's just that this pattern remembers me about the existing typed Event interfaces, their adapter classes with empty implementation for easier sub-classing and the static XYAdpter factory methods added the the interfaces as last.

And with the current approach we somehow have the Adapter directly in the interface, while (at least from my understanding and experience) the XYAdpter factory methods are usually more convenient to use (and more modern). Therefore I was thinking about using the latter style here again.
But yes, a builder also came into my mind more than once. 😅

As a picture (or code) speaks more than a thousands words I have now added these ideas in #1985 (comment).

If you think this would be better, I can create a follow-up PR for it.

I generally like the idea. What I currently don't see is how we could realize the idea without type switches.

If I understand you right, that's exactly how this is solved. Within the image class it's checked if the special (SWT internal) type is passed. But lets discuss that in #1985 :)