Improves IJ1 Macro Recording by using objectService.getName method instead of toString

[NicoKiaru]

Hi all, and in particular @imagejan and @ctrueden,

This PR adds 2 tests, which, if passing, would improve IJ1 scripting recordability and execution. The first one passes with the modification done in the commits, but not the second one.

• First one: checks whether, when one records a command, the name of the object specified in the objectService is used instead of the call in the toString method.

In practice, this gives:

run("myCommand", "animal=felix");

In the Macro Recorder, instead of:

run("myCommand", "animal=org.orga.unit.stuff.Pet@57a3e26a");

This works if one has put the object in the objectService using objectService.add(new Pet(),"felix");

• the second test is somehow the mirror of this. The idea, is that if one copy and paste run("myCommand", "animal=felix"); in the script editor and launch it, that would work directly.

However, I'm not sure at this point if that's possible in a 'generic' way. I guess the idea would be that if a String input cannot be resolved, even after looking through all the converters, one would have to look in the ObjectService whether there's a named object which fits the expected class. Such a mechanism should not be in legacy, and it looks like a strong change.

One thing which works currently is to write explicit converters from String to a specific class (here a Converter<String, Pet>. And we can probably live with that.

Last thing : even if an input would be resolved thanks to its objectService.getName String representation, the given input to the script should be different depending on the scripting language : the Object should be given for all languages, except in IJ1 Macro where you need to send a String. And ideally, this String should be the one from objectService.getName instead of the one from toString.

I hope it's clear, that's of lot of things and I'm ready to spend some time on it, but I'd be pleased to have your inputs.

[imagejan]

the Object should be given for all languages, except in IJ1 Macro where you need to send a String

I think returning the named object would be sufficient. How do you imagine handling arbitrary objects in the macro language anyhow? Parameters such as #@ TrackMate or #@ BDV just don't work in the limited macro language. You'd have to implement special-casing for each object you want to support anyways.

[NicoKiaru]

the Object should be given for all languages, except in IJ1 Macro where you need to send a String

I think returning the named object would be sufficient. How do you imagine handling arbitrary objects in the macro language anyhow? Parameters such as #@ TrackMate or #@ BDV just don't work in the limited macro language. You'd have to implement special-casing for each object you want to support anyways.

Indeed, I just want to have a String returned, but not the one from toString(), rather the name in the object service. Right now there's a test which returns:

org.junit.ComparisonFailure: 
Expected :Oh, Felix is so cute!
Actual   :Oh, net.imagej.legacy.plugin.IJ1MacroLanguageNamedObjectTest$Pet@57a3e26a is so cute!

net.imagej.legacy.plugin.IJ1MacroLanguageNamedObjectTest$Pet@57a3e26a may be cute, but his life is probably difficult with a name like that.

[NicoKiaru]

the Object should be given for all languages, except in IJ1 Macro where you need to send a String

I think returning the named object would be sufficient. How do you imagine handling arbitrary objects in the macro language anyhow? Parameters such as #@ TrackMate or #@ BDV just don't work in the limited macro language. You'd have to implement special-casing for each object you want to support anyways.

Indeed, I just want to have a String returned, but not the one from toString(), rather the name in the object service. Right now there's a test which returns:

org.junit.ComparisonFailure: 
Expected :Oh, Felix is so cute!
Actual   :Oh, net.imagej.legacy.plugin.IJ1MacroLanguageNamedObjectTest$Pet@57a3e26a is so cute!

net.imagej.legacy.plugin.IJ1MacroLanguageNamedObjectTest$Pet@57a3e26a may be cute, but his life is probably difficult with a name like that.

Found the culprit : https://github.com/imagej/imagej-legacy/blob/5c46549e765b3148d767a8c90d15b6be03b4c154/src/main/java/net/imagej/legacy/plugin/IJ1MacroEngine.java#L251

[NicoKiaru]

Ok, so the commit https://github.com/imagej/imagej-legacy/pull/243/commits/56d3b21ac0ff46b0be27e39db3528a2decf2bb46 now allows IJ1 macro variables to be initialized with the name of objects present in the ObjectService, instead of the toString() function, in line with the PR https://github.com/scijava/scijava-common/pull/374

I removed the extra test in https://github.com/imagej/imagej-legacy/pull/243/commits/fb55fc22afc8d1fc1a95e3655ba68905103535ee, which is probably a behaviour which should not be allowed. Indeed, it would risk to create unexpected behaviour as it would convert a String to an object of potentially unbounded class. Instead, if one writes a custom converter from String to a specific class, the expected behaviour will be reached in a safer manner I believe.

[imagejan]

The problem for the lookup Name => Object is that names don't have to be unique (see https://github.com/scijava/scijava-common/issues/371#issuecomment-566504575).

The ParseService supports this already when running with strict=false, as illustrated by this script:

#@ ObjectService os
#@ ParseService ps

import org.scijava.parse.eval.Unresolved

foo = "fooString"

os.addObject(foo, "barName")

item = ps.parse("someObjectParam=barName", false).asMap().get("someObjectParam")

if (item instanceof Unresolved) {
    // tryGettingObjectFromName(item.getToken())
    println "Here we'd try to retrieve the object with name ${item.getToken()}"
}

... but we would have to:

1. determine the type of the parameter being parsed
2. get all objects of this type using ObjectService
3. get the NamedObjectIndex using objectService.getIndex()
4. cycle through all objects of suitable type and keep the first (?) with a matching name.

[NicoKiaru]

Hey @imagejan,

Do you think we could try to extend this mechanism to Array of Objects? Current a File[] shows up in the recorder as:

run("command_with_files_as_input", "f=[Ljava.io.File;@15711767");

Maybe it's a bit tricky (we need to escape separators, should we deal with arrays of arrays ?), but that could be worth it. I think it's not hard to get something at least a bit better.

Combined with a String to YourObject[] converter, this could really make life easier for image analysts who are not programmers.

[imagejan]

We have StringToFileArrayConverter since the the File[] parameters were implemented:

https://github.com/scijava/scijava-common/blob/1731a7812aa4f7c0fd78a590c0f5ea2e1118c771/src/main/java/org/scijava/convert/FileListConverters.java#L78-L92

and I think we discussed exactly the case of providing an option string and converting to a list of files, so I'm a bit surprised that it doesn't record correctly (any more?).

But this use case here is different, as it would imply converters from String to any object, as you don't know what type of objects it might be used for. We should be able to improve the ModuleService/CommandService that it knows which type of object it requires to resolve a given input, and to try to convert a String into that type of object by asking the ObjectService.

All this is unrelated to imagej-legacy and to the IJ1 macro language.

In IJ1 macros, as you say, the only valid use case would be to get a String in the arguments, and to keep it as a String because the only thing you do with it is passing it in yet another option string to a run(...) call. No need for any converters on the level of the macro language.

[ctrueden]

Current a File[] shows up in the recorder as:

run("command_with_files_as_input", "f=[Ljava.io.File;@15711767");

We could add case logic to use Arrays.toString(<primitive-array-type>) or Arrays.deepToString(Object[]) as appropriate. Just off the top of my head—it's been awhile since I've looked at this code.

[NicoKiaru]

But this use case here is different, as it would imply converters from String to any object, as you don't know what type of objects it might be used for.

In an IJ1 macro case where run("myCommand", "arg1=val1 arg2=val2"); is called, and where myCommand refers to an IJ2 Command, we know what class of object is needed, because the class of arg1 is know. Then scijava looks for a converter from String to the class of arg1. This already works.

And that's the case which is the most interesting for IJ1 Macro Language / IJ2 Command interoperability.

We should be able to improve the ModuleService/CommandService that it knows which type of object it requires to resolve a given input, and to try to convert a String into that type of object by asking the ObjectService.

I think this is already in place. For instance, with the bdv playground repo, I have a converter from a String to a BdvHandle. It's all it takes to call IJ2 command from IJ1 Macro Language. All the commands with one or several BdvHandle inputs are resolved using this converter, if called from an IJ1 macro.

In practice : this Screenshot command perfectly works right now, in IJ1.

Simple example : the IJ1 macro code

run("Screenshot", "bdvh=bdvwindowtitle targetPixelUnit=micron showRawData=true")

executes nicely thanks to the converter from String to BdvHandle, the BdvHandle object is correctly given as an input to the IJ2 screenshot command.

In short: Calling an IJ2 command from an IJ1 macro command is already working - thanks to scijava converters - no modification needed

We have StringToFileArrayConverter since the the File[] parameters were implemented: https://github.com/scijava/scijava-common/blob/1731a7812aa4f7c0fd78a590c0f5ea2e1118c771/src/main/java/org/scijava/convert/FileListConverters.java#L78-L92 and I think we discussed exactly the case of providing an option string and converting to a list of files, so I'm a bit surprised that it doesn't record correctly (any more?).

For the recorder, you need the opposite converter : a converter from File[] to String. I don't know if it exists, but even if it would exist 1 - it wouldn't be called, because the MacroRecorder do not try at all currently to look for converters from Object to String. And that is what needs to be fixed, because IJ1 macro can only deal with String. At the moment, this line is used to convert an object to a String, and that's a toString call.

But! Thanks to the mechanism that @imagejan introduced in scijava-common, there is a way to associate a name to an Object in the ÒbjectService. And what I did in commit https://github.com/imagej/imagej-legacy/pull/243/commits/56d3b21ac0ff46b0be27e39db3528a2decf2bb46 is to at least use this name instead of the toString method. For the previous commit, this works, as this test is passing.

In short: With this PR, currently, the macro recorder uses the getName function of the ObjectService instead of toString, this makes macro recording better when they reference a single object of the ObjectService

BUT there are two limitations : 1 - Synchronization : what if I set the name of the BdvHandle object by its window title, and then the user is changing the name of the window ? That can lead to some horrible difficult to comprehend bugs for the user. To solve this, I need to keep track of these window title modifications (controlling all command, or via an event...). 2 - Multiple Object Selection : I have many command which require an array of input objects. For instance in Bdv I need to reference tens of sources. I deal with this by having SourceAndConverter[] inputs (see here for instance). While each Source object is in the ObjectService, an array of them is not in the ObjectService... So the macro recorder will fall back to the toString method, which gives a useless and ugly result.

Here's my proposition to solve this: we need to involve scijava converters.

And a first remark: we should not use converters to String like a BdvHandle to Stringconverter or a SourceAndConverter[] to String converter : this is too problematic as it interferes with all String inputs of all Command. Horrible behaviour example : if both a SingleInputPreprocessor<MyClass> and a Converter<MyClass,String> exists, as soon as a MyClass object is present in the object service, all String inputs parameter from ALL commands are automatically prefilled with the converter applied on the preprocessor output. This also messes up String inputs that are supposed to be of multiple choices (an error is thrown).

Instead I suggest to use the already existing org.scijava.Named interface. So in practice, if I want to have correctly recorded SourceAndConverter and SourceAndConverter[] objects. First I need to write two converters : from SourceAndConverter to Named and another one from SourceAndConverter[] to Named. Second we need to introduce a mechanism which looks for converters from Object to Named when the macro recorder is involved. And here there's a choice:

• either we restrict this conversion mechanism to imagej-legacy only. This means we consider that this mechanism is useful for macro recording only. The function is modify is [this one] (https://github.com/imagej/imagej-legacy/pull/243/commits/56d3b21ac0ff46b0be27e39db3528a2decf2bb46#diff-b20f27bed3ef652ee830ee0c7a299cb6R660). It should look for converter from Object to Named, and if the conversion is not possible or returning null, then fall back on ObjectService::getName.

• or we consider this is useful not only in IJ1 macro - and I think that would be better in theory, but maybe there would be unexpected issue, so it's more dangerous. One example I have in mind is for Swing Inputs. Maybe that could solve some synchronization issues in IJ1 images ? Anyway, in this case, it's a modification that has to done in scijava-commons. More specifically , ObjectService::getName has to be changed by including the potential conversion of Object to Named before giving it up to other alternatives.

The big advantage of using this converters for Arrays of object, if that even if a SourceAndConverter[] object is not in the object service ( and thus has no name ) it can still have an associated converter which can then output a nice name.

Are you ok to involve the converter framework into naming object that are into the ObjectService?

Should it be done in imagej-legacy (macro language only) or scijava-common?

Final remark : I tested all of these possiblities by modifying locally scijava-common and inagej-legacy, and it can work nicely

ping @ctrueden @imagejan

[imagejan]

Thanks a lot for your extensive write-up, @NicoKiaru!

Just a few thoughts:

I'm not sure if converting to Named is a good idea. Objects could just as well implement Named if they need that functionality. And if this is not desired from the BDV side (e.g. for BdvHandle), you could still make a new NamedBdvHandle extends BdvHandle implements Named, no?

But alright, if we conclude we want this, I'd rather implement some improvement in scijava-common than creating yet another IJ1 macro special casing. (In general, the calling of commands is not macro-specific, it's the same from the command line; the recording is currently specific to the IJ1 macro recorder, just because there still is no other command history mechanism available in SciJava.)

One general issue I see with the approach is: how to handle duplicate names, i.e. two or more objects that have the same name (be it via Named or via objectService.getName()). Should we just return the first matching object, or throw an exception.

Lastly, converting arrays of objects: we'd need something like the Arrays.deepToString(Object[]) suggested by @ctrueden, but instead of toString calling objectService.getName() for each object, right?

[NicoKiaru]

I'm not sure if converting to Named is a good idea.

Maybe, but I don't see an alternative that would give as much additional functionality and flexibility.

Objects could just as well implement Named if they need that functionality.

Yes! And I just asked this ;-) (https://gitter.im/bigdataviewer/bigdataviewer-core?at=5ea6c828afa1f51d4e22d37c)

you could still make a new NamedBdvHandle extends BdvHandle implements Named

Yes!, but honestly that's a pain, and this means I need to control all created instances of these. If somebody creates a BdvHandle somewhere else, then I need to wrap it.

how to handle duplicate names, i.e. two or more objects that have the same name (be it via Named or via objectService.getName())

This doesn't change anything I believe. And we have nothing to do because this will be dealt with in the converter from String to YourObject. So the developper who wishes to have his specific object correctly dealt with has to do the work of handling duplicates - or not if he wishes. For instance this converter (which already works and which functionality will not be affected) takes the first BdvHandle with the correct name, if it exists. But I could have done differently - and no modification in scijava-common or imagej-legacy will be needed.

Lastly, converting arrays of objects: we'd need something like the Arrays.deepToString(Object[]) suggested by @ctrueden, but instead of toString calling objectService.getName() for each object, right?

Actually, nothing is required except modifying the ObjectService::getName like this:

/**
     * Gets the name belonging to a given object.
     * <p>
     * If no explicit name was provided at registration time, the name will be
     * derived from {@link Named#getName()} if the object implements
     * {@link Named}, or from an existing converter
     * {@link org.scijava.convert.Converter<Object, Named>}
     * or from the {@link Object#toString()} otherwise. It is
     * guaranteed that this method will not return {@code null}.
     * </p>
     **/
    default String getName(final Object obj) {
        if (obj == null) throw new NullPointerException();
        final String name = getIndex().getName(obj);
        if (name != null) return name;
        if (obj instanceof Named) {
            final String n = ((Named) obj).getName();
            if (n != null) return n;
        }
        ConvertService cs = context().getService(ConvertService.class);
        if (cs != null) {
            if (cs.supports(obj, Named.class)) {
                final Named named = cs.convert(obj, Named.class);
                if (named != null) {
                    if (named.getName() != null) return named.getName();
                }
            }
        }
        final String s = obj.toString();
        if (s != null) return s;
        return obj.getClass().getName() + "@" + Integer.toHexString(obj.hashCode());
    }

Again, the job of writing correctly the array of object to Named will be delegated to the YourObject[] to Named converter. I already wrote what would be the required converters in a branch of bdv-playground. See for instance the pair of converters:

• from BdvHandle[] to Named
• from String to BdvHandle[].

I used gson and it's maybe overkill, but anyway the point is that this work is delegated to converters.

[NicoKiaru]

Hmm for completeness, here's what could go wrong with modifying scijava-common: imagine there's an object obj which has a nice toString() method, but then, for some reasons, there's a weird converter floating around which could convert obj into an irrelevant class that extends Named. Then getName could return a nonsense result.

Even if this case didn't show up in the various testing I've made, that's a possibility and maybe it's a bit safer to keep this mechanism in imagej-legacy while waiting for a use case outside of macro recording ? Otherwise, in scijava-common, one could try to restrict the converter to one which would output an object implementing nothing else than the Named interface, but I don't know if that's possible. Last alternative is to make a class exclusively dedicated to this use case, like IJ1MacroName, but that's "yet another IJ1 macro special casing".

[NicoKiaru]

Ok, I'll take your heart for an approval @imagejan 😁

Here's what I wanted to do :

• implement the improved conversion within imagej-legacy and using an IJ1MacroName class, to be on the safe side
• add a test for this behaviour
• add somewhere (where?) an example and document how to use this improved macro scripting mechanism
• ask for review(s) (@imagejan or @ctrueden ?)
• merge ?

Sounds like a plan ?

[imagejan]

@NicoKiaru was closing this intended?

[NicoKiaru]

Oups maybe I was too harsh in my cleanup... To be fair I moved away from the idea of adding more complexity to IJ1 macro language. So maybe it's good anyway ?

Your PR is more recent. Let's restart from here if needed.