Improves IJ1 Macro Recording by using objectService.getName method instead of toString #243
Conversation
I think returning the named object would be sufficient. How do you imagine handling arbitrary objects in the macro language anyhow? Parameters such as |
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:
|
Found the culprit : |
… in the objectservice instead of the toString() method
…Convert from String to unbounded class type should be avoided. Use explicit custom converter instead.
|
Ok, so the commit 56d3b21 now allows IJ1 macro variables to be initialized with the name of objects present in the ObjectService, instead of the I removed the extra test in fb55fc2, 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. |
|
The problem for the lookup Name => Object is that names don't have to be unique (see scijava/scijava-common#371 (comment)). The #@ 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:
|
| final String script = "" + // | ||
| "#@ Pet animal\n" + // | ||
| "#@output String greeting\n" + // | ||
| "greeting = 'Oh, '+animal+' is so cute!'\n" + // |
There was a problem hiding this comment.
What is the benefit of having the object name available in the macro language? Apart from printing log messages, I don't see much value in this, as there's no way to actually do anything with the input object. IMHO it would only make sense if you use the API of the object, i.e. for other scripting languages than IJ1 macros.
There was a problem hiding this comment.
Imagine you have an IJ1 script that manipulate a BdvHandle, and a library with a String to BdvHandle converter. The user makes a screenshot and close the window.
The recorder then outputs:
run("BdvScreenShot", "bdvh=BDV_01");
run("BdvClose", "bdvh=BDV_01");
Now the user wants to make a small script that does both actions. And he likes the macro language.
He will just have to do a script like that:
#@BdvHandle bdvh;
run("BdvScreenShot", "bdvh="+bdvh);
run("BdvClose", "bdvh="+bdvh);
Does that make sense ? I wanted to try it but I cannot use the new legacy I built because of the error I mentioned on gitter, but I have no time to investigate it now. Will later later or next week.
There was a problem hiding this comment.
I tried and it works.
|
Hey @imagejan, Do you think we could try to extend this mechanism to Array of Objects? Current a 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. |
|
We have 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 All this is unrelated to In IJ1 macros, as you say, the only valid use case would be to get a |
We could add case logic to use |
In an IJ1 macro case where And that's the case which is the most interesting for IJ1 Macro Language / IJ2 Command interoperability.
I think this is already in place. For instance, with the bdv playground repo, I have a converter from a In practice : this Screenshot command perfectly works right now, in IJ1. Simple example : the IJ1 macro code executes nicely thanks to the converter from In short: Calling an IJ2 command from an IJ1 macro command is already working - thanks to scijava converters - no modification needed
For the recorder, you need the opposite converter : a converter from But! Thanks to the mechanism that @imagejan introduced in In short: With this PR, currently, the macro recorder uses the BUT there are two limitations : Here's my proposition to solve this: we need to involve scijava converters. And a first remark: we should not use converters to Instead I suggest to use the already existing
The big advantage of using this converters for Arrays of object, if that even if a Are you ok to involve the converter framework into naming object that are into the Should it be done in Final remark : I tested all of these possiblities by modifying locally scijava-common and inagej-legacy, and it can work nicely |
|
Thanks a lot for your extensive write-up, @NicoKiaru! Just a few thoughts: I'm not sure if converting to But alright, if we conclude we want this, I'd rather implement some improvement in 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 Lastly, converting arrays of objects: we'd need something like the |
Maybe, but I don't see an alternative that would give as much additional functionality and flexibility.
Yes! And I just asked this ;-) (https://gitter.im/bigdataviewer/bigdataviewer-core?at=5ea6c828afa1f51d4e22d37c)
Yes!, but honestly that's a pain, and this means I need to control all created instances of these. If somebody creates a
This doesn't change anything I believe. And we have nothing to do because this will be dealt with in the converter from
Actually, nothing is required except modifying the /**
* 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 I used gson and it's maybe overkill, but anyway the point is that this work is delegated to converters. |
|
Hmm for completeness, here's what could go wrong with modifying 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 |
|
Ok, I'll take your heart for an approval @imagejan 😁 Here's what I wanted to do :
Sounds like a plan ? |
|
@NicoKiaru was closing this intended? |
|
Oups maybe I was too harsh in my cleanup... Your PR is more recent. Let's restart from here if needed. |
3 participants
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.
In practice, this gives:
In the Macro Recorder, instead of:
This works if one has put the object in the objectService using
objectService.add(new Pet(),"felix");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
Stringto a specific class (here aConverter<String, Pet>. And we can probably live with that.Last thing : even if an input would be resolved thanks to its
objectService.getNameString 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 fromobjectService.getNameinstead of the one fromtoString.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.