imagej / imagej-legacy

ImageJ+ImageJ2 compatibility layer
https://imagej.net/libs/imagej-legacy
BSD 2-Clause "Simplified" License
16 stars 25 forks source link

Fix duplicate entries in image selections #171

Closed stelfrich closed 5 days ago

stelfrich commented 6 years ago

Previously, image selections (e.g. in a Swing UI) would show three entries in a dropdown for each image opened as a Dataset. The issue was that multiple Converters would add convertible inputs to the set of compatible inputs without checking if another converter has added a different representation of the same data (e.g. ImageTitle, ImageDisplay, Dataset).

This commit changes the implementation in ImageTitleToImagePlusConverter to check if a Dataset is available, i.e. it prefers Datasets. Similar changes check for an available Dataset in the ImageDisplayToImagePlusConverter to prefer that.

ctrueden commented 6 years ago

@stelfrich Thank you very much for your initiative in investigating this and proposing a fix!

But oof, this is hairy stuff. I want to think a bit about whether this is really the best solution. It honestly may be—I just want to think through the implications a bit before merging it.

We could dig a bit during our weekly video chat, if you are amenable to that.

stelfrich commented 6 years ago

But oof, this is hairy stuff.

Trust me, I am fully aware of that. My biggest problem was that the duplicates or triplicates that are shown in the UI are actually three different objects... we could also think about a way to do the checks from this PR in the UI layer?

We could dig a bit during our weekly video chat, if you are amenable to that.

Let's do that!

hinerm commented 4 weeks ago

This change is sadly insufficient.

I was unaware of this PR and a year ago made a change that is very similar but more fragile, by using String comparison.

However, let's say I have two image script parameters (using ImgPlus, while working on this issue) and two images open. If I run the script:

  1. When parsing input 1, all the converters miss until net.imagej.legacy.convert.ImagePlusToImgPlusConverter is hit and tells the ConvertService "hey we have two ImagePlus instances as options"
  2. The "first" image is arbitrarily chosen as an initial value for input 1, which triggers legacy image registration, so there is now a Dataset (and an ImageDisplay)
  3. When parsing input 2, net.imagej.convert.ImageConverters$DatasetToImgPlusConverter and net.imagej.convert.ImageConverters$ImageDisplayToImgPlusConverter now hit, because of the registered ImagePlus for input 1.
  4. The user is presented with three options: the unconverted second ImagePlus, an ImageDisplay, and a Dataset for the fconverted first input (the latter two appear identical in the list)

If the user were to select the unconverted ImagePlus as a parameter, that would result in its registration with the legacy service and future script runs would have 4 options: the repeated ImageDisplays and Datasets for both original ImagePlus instances. The ImagePlus is being hidden solely because of the aforementioned logic I added.

So the point is: we also would need this logic in DatasetToImgPlusConverter and ImageDisplayToImgPlusConverter . But that also is not sufficient.

If I change my inputs to Dataset instead of ImgPlus, I still get multiple options, this time because of net.imagej.convert.ImageConverters$ImageDisplayToDatasetConverter and net.imagej.convert.ImageConverters$ImgToDatasetConverter.

A simple enough fix that would work now is to change all the to[Image] converters to avoid listing a particular item if we have reason to believe some other converter already is handling that item, either by:

  1. explicitly hard-coding a hierarchy (as was done in this PR - Dataset wins)
  2. taking a guess (as was done by me, excluding equivalently-named objects)

This does concern me for extensibility when creating other converters between new data types (e.g. Mat) and ImgPlus/Dataset/etc...

The reality is that these converters all need to know about each other, so maybe it would make more sense to formalize that in an extensible way. That could be disturbingly complex, though.

hinerm commented 5 days ago

Closing in favor of fixing this upstream in https://github.com/scijava/scijava-common/pull/482