Image: add getSize()

[tmssngr]

Image currently has just a method getBounds() returning a Rectangle with x and y always zero. IMHO it would make sense to add getSize() and deprecated getBounds().

[ptziegler]

-1 for deprecating getBounds() no opinion on adding a getSize()

I think the getBounds() method is primarily there for compatibility with the remaining API. Just looking at our own code base, I see several instances of e.g. gc.fillRectangle(image.getBounds()) and getClientArea().equals(image.getBounds()), which will cause a lot of bloat if this method were to be deprecated.

The Control class also defines both getBounds() and getSize(), so this isn't without precedence.

[HeikoKlare]

The proposal makes sense to me, but as usual, replacing such integral API is difficult. So we will likely have to maintain two methods for a long time then, but in this case there are at least rather simple.

The Control class also defines both getBounds() and getSize(), so this isn't without precedence.

The difference I see here is that getBounds() of control will usually yield x/y != 0. Statements like gc.fillRectangle(image.getBounds()) are pretty much convenience to avoid the necessity to wrap a size of an image into bounds with x=y=0 (which could be solved different than adding a convenience methods getBounds() to Image). But that's how it is and I always ask in such situation whether it's really worth the effort to request consumers to adapt to slightly improved API with no real other gain.

Since @HannesWell has been working on improving APIs (also the one of Image), maybe you have an opinion on this?

[ptziegler]

The difference I see here is that getBounds() of control will usually yield x/y != 0. Statements like gc.fillRectangle(image.getBounds()) are pretty much convenience to avoid the necessity to wrap a size of an image into bounds with x=y=0 (which could be solved different than adding a convenience methods getBounds() to Image).

I think GC is actually a good example of why I have mixed feelings about this. SWT doesn't have a Dimension class like Swing. So a getSize() method will likely have to return a Point.

In my example, the GC method then has to be refactored to:

Point p = image.getSize();
gc.fillRect(0, 0, p.x, p.y);

Or alternatively, a new gc.fillRectangle(Point) is added and the example can be simplified to gc.fillRectangle(image.getSize()).

But semantically, this just doesn't work very well, because you now use x and y ambigously; As the x and y coordinates of the rectangle and also as its width and height.

Or to rephrase it: If I, as a user, have the choice between image.getBounds().width and image.getSize().x, I will always pick the former, simply because it's much less ambiguous.

Edit: My point is: If a getSize() method is added, it should not return a Point as this class describes the location of a component, not its dimension. But if a new Dimension class is added, support also needs to be added to other classes like GC, which then makes me wonder whether this is really worth the effort...

[tmssngr]

@ptziegler Your example only is a shortcut for those (rare?) case where you draw the image at (0, 0). I agree with the meaning of x/y for a size, but that's the API. Instead of image.getBounds().width I always would prefer to add image.getWidth() (same for Control).

OK, let's forget about deprecating getBounds(). But introducing getSize() seems very log for me.

[HeikoKlare]

Edit: My point is: If a getSize() method is added, it should not return a Point as this class describes the location of a component, not its dimension. But if a new Dimension class is added, support also needs to be added to other classes like GC, which then makes me wonder whether this is really worth the effort...

You are right, the common usage of Point to define a size makes it semantically imprecise / incomprehensible again. I rather meant that with cleaner class design, one might have some Dimension (or, in our case unfortunatly, Point) class with some asBounds() or the like, i.e., something to explicitly transforms a size object into some zero-based bounds object. But with using Point for sizes this does hardly make sense.

So to summarize, we would probably need some even larger change introducing a proper size/dimension representation in SWT before it makes sense to provide some getSize() functionality for Image.

[tmssngr]

I rather meant that with cleaner class design, one might have some Dimension (or, in our case unfortunatly, Point) class with some asBounds() or the like, i.e., something to explicitly transforms a size object into some zero-based bounds object.

This brings me to the thought: Event could have a asPoint() method, too, which wraps event.x and event.y.

So to summarize, we would probably need some even larger change introducing a proper size/dimension representation in SWT before it makes sense to provide some getSize() functionality for Image.

I don't think, the introduction of image.getSize() needs to delayed.

[ptziegler]

I rather meant that with cleaner class design, one might have some Dimension (or, in our case unfortunatly, Point) class with some asBounds() or the like, i.e., something to explicitly transforms a size object into some zero-based bounds object.

That sounds like a good idea. It definitely makes the migration a lot easier.

Just out of curiosity, what's the motivation behind touching the getBounds() method?

[tmssngr]

Just out of curiosity, what's the motivation behind touching the getBounds() method?

I just noticed that getBounds() had always zero coordinates while a getSize() was missing. For me, the getSize() method is more important than touching the legacy getBounds().

[HeikoKlare]

So is it only about boilerplate code required due to abscence of getSize(), right? Then the issue remains that we have no proper return type to be used for getSize() yet.

[ptziegler]

Then the issue remains that we have no proper return type to be used for getSize() yet.

I can try to draft a PR over the next few days, just to see how much change a new datatype would require. The main challenge I see is that this new datatype should also be returned by the already existing getSize() method. Which is problematic, given that some have been API for a very long time...

[tmssngr]

So is it only about boilerplate code required due to abscence of getSize(), right? Then the issue remains that we have no proper return type to be used for getSize() yet.

That's the case for Control, too, but this IMHO can't be changed in a backward compatible way.

[HeikoKlare]

just to see how much change a new datatype would require. The main challenge I see is that this new datatype should also be returned by the already existing getSize() method. Which is problematic, given that some have been API for a very long time...

Honestly, I don't expect that this is something we can do. You cannot make some getSize() methods return a different type than others (e.g., Point vs. Size) and since the existing getSize() methods have been there forever, you cannot easily remove them. Of course you can deprecate them and define new ones, but consider how much effort that is for consumers. Those methods are very central API used at many places in consumers, so we will never get rid of them anyway. You will end up having to implementations of the same thing represented by different data types and methods. That will introduce more confusion than improve comprehensibility (unfortunately, as it would of course be nice to have that improved).

Edit: note that it would not only be about getSize() but would need to be consistent everywhere a Point is representing a "size" (at leasting including according setter and probably other API as well).

[merks]

I really think we're going off the deep end on the Size() data type thing:

"It's water under the bridge." "That ship has sailed."

[ptziegler]

You cannot make some getSize() methods return a different type than others (e.g., Point vs. Size) and since the existing getSize() methods have been there forever, you cannot easily remove them.

You can't on a language level, but it's certainly possible to define bridge methods on a byte-code level to ensure compatibility... But I agree, this ship has sailed.

So what's the plan instead? As mentioned, I'm not a huge fan of using Point, as you then always have to do the mental conversion from (x,y) to (width,height).

Looking at Swing again, the BufferedImage class doesn't have a getSize() method either but instead defines both a getWidth() and getHeight() method. Would this already help with reducing the boilerplate code?

[tmssngr]

OK, let's do getSize(), getWidth() and getHeight() (and also introduce getX(), getY(), getWidth() and getHeight() for Control). Or should we rather avoid the get prefix as Java does with records?

[HannesWell]

Since @HannesWell has been working on improving APIs (also the one of Image), maybe you have an opinion on this?

I have not yet have a full picture, but try to get it soon.

OK, let's do getSize(), getWidth() and getHeight() (and also introduce getX(), getY(), getWidth() and getHeight() for Control). Or should we rather avoid the get prefix as Java does with records?

Without knowing all the details, I'm really not sure if adding that many new methods is a good idea.

[merks]

Consider that for such library that's being available for decades, adding bunch of "helpful" methods is really not likely to get used by many consumers who are very unlikely to spend their time changing their already-working code to use methods that make their library compatible only with the latest version of SWT. Mixing the existing style with a "records" style also seems questionable to me.