parameter type for setValue() is Object should probably be Value

[urpeter]

I tried to use the setValue() function in my plugin, but it did not set the Value of my Variable to the intended Value. I wrote a short program where it occurs: The Box in the Test Node has to be set to "Wert" for it to work. The should be value is printed to the console.

Testprogram.zip

[timobaumann]

setValue requires you to set an instance of the Value type hierarchy. You're setting a String so it doesn't work. What you want to do is setValue(new StringValue(result));

However, @alexanderkoller , @akoehn : I wonder if setValue() should really fail silently if a non-Value is passed as parameter. Any thoughts about that?

Anyone interested in checking all calls to setValue() on whether we could simply change the signature to require Values rather than Objects?

[alexanderkoller]

Wow, that is stupid code in DialogOS. Yes, I can look into this.

[timobaumann]

thinking about this again, the intention may have been to eventually do the cast within setValue, which would be a very convenient decision. However, I don't know if we can make Java autobox ints to Integer, etc. If so, we could keep the signature and significantly improve the implementation with instanceof-checks.

[alexanderkoller]

Slot#setValue(Object) does do the cast of Object to Value. But given that DialogOS variables can only hold Values anyway, I don't really see a use for this. The only exception I can think of is in the context of Groovy variables. But in that case, the instanceof in line 116 would fail anyway, and nothing would happen.

@akoehn Do you know off-hand if Groovy values are stored in Slots or in objects of some other class?

[timobaumann]

I meant something along the lines of:

public void setValue(Object o) {
    switch(instanceof(o)) {
        case String: StringValue(o);
        case Integer: IntValue(o);
....
}

of course, the cast alone is not helpful.

[alexanderkoller]

Ah, I see.

[timobaumann]

I guess the "right" solution is to have both the plain setValue(Value) and a comfySetValue(Object). The advantage of the former is that @urpeter's issue would have become apparent in the former version and does not need to handle an illegalargumentexception at runtime, whereas the latter is just very convenient. We should implement magic for String, int, bool, ... and also maps, lists, and possibly arrays. The alternative to illegalargumentexception is casting to String via toString() which, again, is convenient (no exception handling) but probably error-prone.

[akoehn]

Groovy variables are can store anything, they are not wrapped in any way.

This is one of the reasons I dislike the dialogos scripting language; As already stated I would like to label it as legacy and push users towards groovy, which has sane semantics and good documentation.

I'm willing to add groovy functionality to all places where currently only the dialogos script is available. Then there would be no need to modernize the dialogos scripting facility.

[alexanderkoller]

@akoehn I'd be happy to talk about improving the Groovy integration sometime. This particular issue, I think, is not so much about "modernizing" the DialogOS script language, more about fixing a sloppy piece of code.

@timobaumann Yes, these are options. But as far as I can tell, there is no place in the entire codebase which uses Slot#setValue(Object). I'd be happy to just get rid of this method and force people to call Slot#setValue(Value). There is a method Value#of which converts from whatever to Value, so it's not like this would inconvenience people that much.

[alexanderkoller]

Upon closer analysis, I think Slot#setValue(Object) was introduced when Till Kolenda refactored it to introduce GroovyVariable as a sister to Slot, both derived from AbstractVariable.

I modified AbstractVariable with the correct type parameters, which makes Slot#setValue(Object) unnecessary to implement the abstract method from the base class. GroovyVariable remains unchanged.

[akoehn]

@alexanderkoller: My point was mainy re: @timobaumann's suggestion to implement additional magic.