-
Notifications
You must be signed in to change notification settings - Fork 25
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 duplicate entries in image selections #171
Conversation
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 preferes Datasets. Similar changes check for an available Dataset in the ImageDisplayToImagePlusConverter to prefer that.
eb54a50
to
49255b6
Compare
@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. |
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?
Let's do that! |
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 However, let's say I have two image script parameters (using
If the user were to select the unconverted 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 A simple enough fix that would work now is to change all the
This does concern me for extensibility when creating other converters between new data types (e.g. 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. |
Closing in favor of fixing this upstream in scijava/scijava-common#482 |
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 multipleConverter
s would add convertible inputs to the set of compatible inputs without checking if another converter has added adifferent representation of the same data (e.g.
ImageTitle
,ImageDisplay
,Dataset
).This commit changes the implementation in
ImageTitleToImagePlusConverter
to check if aDataset
is available, i.e. it prefersDataset
s. Similar changes check for an availableDataset
in theImageDisplayToImagePlusConverter
to prefer that.