Skip to content

Add WidgetStyle utility class to centralize style logic #384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 29, 2020

Conversation

imagejan
Copy link
Member

@imagejan imagejan commented Apr 30, 2020

This pull request addresses #333 and is a first step toward fixing #197 and #382.

The methods in the new WidgetStyle utility class should replace isStyle() implementations and other case logic around (possibly multiple) style attributes of parameters in the following places:


Remaining questions:

  • I wasn't sure about the naming of getStyleModifiers. The intended use case is for styles like "extensions:tiff/tif" to get the list of allowed extensions, but there might be more general use cases.

  • Should getStyleModifiers return an array of strings directly from split("/")? Or rather a Set<String> with trimmed, lower-case values?

@imagejan imagejan force-pushed the widget-style branch 2 times, most recently from 9f48d17 to 655d08c Compare June 19, 2020 20:03
@imagejan imagejan requested a review from ctrueden August 28, 2020 19:23
This should replace isStyle() implementations and other case logic around (possibly multiple) style attributes of parameters.

Also add a test exercising this utility class.
@imagejan
Copy link
Member Author

Thanks @ctrueden for merging this.
In the light of scijava/scijava-ui-swing#52, I now think we should maybe replace/modify getStyleModifiers to do something a bit more general, i.e. return everything after the : (String instead of String[]) and leave it to the caller to split the returned string (e.g. in the case of multiple extensions, such as extensions:tiff/tif) or to use the returned string entirely (e.g. with format:#0.000).

Now that 2.85.0 is released, we probably have to keep the API for backwards compatibility, right? : )

@ctrueden
Copy link
Member

Now that 2.85.0 is released, we probably have to keep the API for backwards compatibility, right? : )

If we change it immediately, I'd be OK with breaking it. It's not part of pom-scijava et al. yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants