scijava / scijava-ui-swing

SciJava UI components for Java Swing.
BSD 2-Clause "Simplified" License
7 stars 11 forks source link

Add SwingColorAlphaWidget #48

Closed frauzufall closed 4 years ago

frauzufall commented 4 years ago

This PR makes it possible to use parameters of typeColorRGBA. Before, when using ColorRGBA, the SwingColorWidget would return null when changing the color because ColorRGB cannot be converted to ColorRGBA here.

I just quickly copied the SwingColorWidget and replaced ColorRGB with ColorRGBA which makes it work (and also possible to assign colors with alpha value). Let me know if there is a cleaner solution and maybe a way to write a test.

frauzufall commented 4 years ago

Thanks for looking at it @imagejan! Here's my use case, it's the LabelEditor where I already copied the widget into in order to use it for an interactive command to change the default colors of any open labeling model: Screencast-2020-05-17-13_50_38

frauzufall commented 4 years ago

The button icon was displaying the RGBA value as RGB (see screencast above) - I fixed that now.

frauzufall commented 4 years ago

I removed duplicate static methods which were copied into SwingColorAlphaWidget and referenced the ones from SwingColorWidget.

I noticed that the tab reordering of the color chooser is not working - the code comment says it would switch from Swatches, HSB, RGB to HSB, RGB, Swatches, but for me it doesn't, see screencast above. Is it working for anyone else? I don't have enough incentive to dig deeper, I'd be fine with the default ordering and just remove this part of the code.. @ctrueden you are listed as maintainer, do you have an opinion? Otherwise I'd just merge this if you don't object.

ctrueden commented 4 years ago

@ctrueden you are listed as maintainer, do you have an opinion?

Would it be possible to have a single SwingColorWidget that supports both RGB and RGBA cases? This would reduce code duplication, no?

frauzufall commented 4 years ago

@ctrueden I pushed a version where SwingColorWidget supports both. It does check the type via ColorRGBA.class.isAssignableFrom(get().getValue().getClass())) and I thought that's not ideal.. But this of course avoids more code duplication. It works for me.

imagejan commented 4 years ago

@frauzufall wrote:

It does check the type via ColorRGBA.class.isAssignableFrom(get().getValue().getClass())) and I thought that's not ideal..

You could also use SciJava's Types utility class:

Types.isAssignable(get().getValue().getClass(), ColorRGBA.class)

or

Types.isInstance(get().getValue(), ColorRGBA.class)

... but I don't know if that makes any difference.