Skip to content
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

Fix --fill-value to use setFillColor(...) API #257

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

melissalinkert
Copy link
Member

...so now this option can be used with all readers.

Noticed while working on #256 that --fill-value still only applied to MiraxReader. Given that setFillColor and getFillColor were added to IFormatReader in Bio-Formats 6.13.0 (see ome/bioformats#3963), this updates --fill-value to make use of this new API. Several of the readers didn't make use of getFillColor() in openBytes, so that's been fixed too.

...so now this option can be used with all readers.
@melissalinkert melissalinkert added this to the 0.10.0 milestone Sep 23, 2024
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on a MRXS file with --fill-value set but the application entered an infinite recursion while calling getFillColor

@@ -1069,10 +1069,17 @@ public void setFillValue(Byte fill) {
* for brightfield.
*
* @return fill value for missing tiles
* @deprecated use getFillColor()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is planned for 0.10.0, is there a value in going through a deprecation rather than dropping these in favor of the new API defined upstream?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't totally sure what the best approach would be here, so went with deprecating to be on the safer side. If we're all OK with just removing getFillValue() and setFillValue(), I'm fine with that - it would only impact anyone using MiraxReader directly, either by extending the reader or by calling those methods directly.


@Override
public Byte getFillColor() {
Byte color = getFillColor();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing this caused a java.lang.StackOverflowError: null due to the infinite recursion

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed with e3dd7ca.

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