imagej / imagej-legacy

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

Add converter from image titel to Dataset #272

Closed maarzt closed 2 years ago

maarzt commented 2 years ago

The newly added StringToDatasetConverter converts a String that matches an ImagePlus title or ID to Dataset. (The ImagePlus must be known to the WindowManager.)

This functionality is required to properly call a SciJava Commands with Dataset inputs from an IJ1 macro.

Fixes #246

maarzt commented 2 years ago

I'd suggest to use a DelegateConverter approach here, as suggested in #246 originially. What are the reasons you implemented it in a different way?

Hi @imagejan, that's an excellent question. Yes, there is some reasoning behind it. This String -> Dataset converter should only open image titles.

A String -> ImagePlus -> Dataset delegate converter, would instead also allow to open files, if there's a String -> ImagePlus converter for opening a file. It would thereby overwrite the String -> File -> Dataset converter in scifio, due to the high priority. The overall situation would become less predictable.

imagejan commented 2 years ago

Still I don't see why a DelegateConverter pattern wouldn't work here.

We have the following scenarios to get an image input (i.e. ImagePlus or Dataset) from a String:

In all cases, taking an open image should (arguably) have higher priority than loading a file from path.

So we need to fix SCIFIO's StringToDatasetConverter if it still picks up file paths that don't exist.


Relatedly, there might be issues in how an image input gets recorded. If I remember correctly (I didn't test right now), the image title is recorded, right? This might be something to reconsider when we actually implement the possibility to Load from file... in the input widget for image inputs, see https://github.com/scijava/scijava-common/issues/380.

maarzt commented 2 years ago

Still I don't see why a DelegateConverter pattern wouldn't work here.

There are two different ways to do the conversion String -> Dataset. It can be done by: 1. finding an window with this title 2. opening a file. I assume this two different ways should have different priorities: 1. normal priority (as StringToImagePlusConverter) and 2. low priority (as SCIFIO's StringToDatasetConverter).

A DelegateConverter for String -> ImagePlus -> Dataset would support both ways: opening image titles and files. But it would assign the same priority to both conversion. Which is (as I assume) not what we want. And there's something else that might be problematic: The delegate converter would create an ImagePlus when opening a file rather than using SCIFIO.

imagejan commented 2 years ago

We could still have two DelegateConverters, one via File and one via ImagePlus, and each one would only accept conversions it actually can handle (i.e. if the image title exists, or if the file exists and can be opened).

They can have different priorities, but this would matter only if there's an image title that at the same time is a valid file path.

hinerm commented 2 years ago

@imagejan @maarzt any objections to my change to an AbstractDelegateConverter that uses a private StringToImagePlusConverter to ensure only image titles/ids are grabbed?