[blockly] Fix item.getAttributes

[stefan-hoehn]

Signed-off-by: Stefan Höhn stefan@andreaundstefanhoehn.de

Fix for what has been mentioned in https://community.openhab.org/t/blockly-reference/128785 (Documentation has already been updated related to this fix)

• Get Item attribute has the wrong block type. As a result it cannot be connected to any other block and cannot be used. Fix: Return "String" instead of "any"
• Get Item attribute returning categories returns an Array of Strings that cannot be used with further blocks. Fix: Return a Collection instead of String in case it is Set of attributes

Work in Progress: Fix is coming

• Get context attribute has the wrong block type. As a result it cannot be connected to any other block and cannot be used. Fix: Return "String" instead of "any"

[relativeci]

Job #315: Bundle Size — 10.67MB (0%).

896f8a5 vs 6ea1e67

Changed metrics (1/8)

Metric	Current	Baseline
Cache Invalidation	0%	3.76%

Changed assets by type (0/7)

No changes

---

View Job #315 report on app.relative-ci.com

[ghys]

Thanks, the dynamic type change looks nice. Some comments below:

[ghys]

I would prefer something simpler like:

Suggested change

	let code = `(${itemName} instanceof org.openhab.core.items.GenericItem) ? ${itemName}.state : itemRegistry.getItem(${itemName}).getState()`
	let code = `itemRegistry.getItem(${itemName}.getName ? ${itemName}.getName() : ${itemName}).getState()`

[stefan-hoehn]

@ghys @rkoshak I would propose I rollback / take out the group handling fix here and rather wait for a final idea how to handle that, so we do not hold the other two fixes back.

[ghys]

I'm really not sure we have to make the code more complex to handle this.

In the case of the oh_getitem_state block, if we mistakenly supply an Item instance instead of a string, we get an exception that's actually confusing (the item exists):

var i;


i = itemRegistry.getItem('Scene_General');
print(itemRegistry.getItem(i).getState());

13:53:26.751 [WARN ] [re.automation.internal.RuleEngineImpl] - Fail to execute action: script
java.lang.RuntimeException: org.openhab.core.items.ItemNotFoundException: Item 'Scene_General (Type=NumberItem, State=1, Label=Scene General, Category=house, Tags=[Status])' could not be found in the item registry
at jdk.nashorn.internal.runtime.ScriptRuntime.apply(ScriptRuntime.java:531) ~[jdk.scripting.nashorn:?]
at jdk.nashorn.api.scripting.NashornScriptEngine.evalImpl(NashornScriptEngine.java:456) ~[jdk.scripting.nashorn:?]
...

however if we supply something of a wrong type to the oh_getitem_attribute block we get an error that's much easier to understand:

var n;


n = 'Scene_General';
print(n.getLabel());

13:57:31.201 [ERROR] [.internal.handler.ScriptActionHandler] - Script execution of rule with UID 'blockly_new' failed: TypeError: n.getLabel is not a function in at line number 5

That's more of a general problem the user have to be aware of (some blocks accept inputs of certain types, as suggested by the shadow blocks), and if you trick your way into supplying wrong types using variables you accept the risk of running into errors. It's not linked to OH blocks, for example:

var n, i;


n = 123;
print((n.join(',')));

14:05:07.345 [ERROR] [.internal.handler.ScriptActionHandler] - Script execution of rule with UID 'blockly_new' failed: TypeError: n.join is not a function in at line number 5

So I would leave it as-is in this case.

[rkoshak]

I'm really not sure we have to make the code more complex to handle this.

The case that came up that prompted this (at my suggestion) was when looping though the list of a Group's members one gets the Item Object. However, "get item state" expects an Item's name.

I think it's somewhat unreasonable for the user to have to know and understand the difference in this one use case, especially when the same "get item state" block can be made to support both the Item's name and an actual Item Object transparently.

It's the difference between:

which intuitively seems like it should work but will fail and forcing the users to use

which clearly works but is anything but intuitive. Why do I need to save the Item's state to a variable first? Why can't I just use "get state of item" directly?

That is the specific problem being addressed here. I don't think, in this case at least, the users should have to know and deal with this type error so I'd say that merely changing the error so it's more meaningful is suboptimal. It would be more user friendly to just handle both cases (i.e. item name or item instance) transparently.

As an aside, the number one problem I need to help new users with on the forum, regardless of the rules language chosen, are type problems. Anywhere we can avoid or sidestep a type errors for the users will be a big win. This is pretty firmly in the type error category.

I know we can't get away from all type errors, but this appears to be case where we can.

[stefan-hoehn]

@ghys @kaikreuzer Can someone help me to undo the last commit without a trace so we can go forward with this PR as these fixes are more important

I fear if I do that I will do something wrong with the PR (even though it has been described here)

I then need to add another fix to this because I just found out during writing the blockly reference that the following block has the wrong return type "any" and therefore cannot be added to any block and not used at all:

I only want to push this fix after we have removed the one for the group handling.

Alternatively I could offer to close/reject the whole PR and do a new one.

[kaikreuzer]

@stefan-hoehn I have removed the second commit on your branch, now there's only the first one left. I hope this is what you asked for!

[stefan-hoehn]

Thanks @kaikreuzer, yes I did. The commit is gone here which is perfect.

I tried to sync my fork:

What I did on my side was to fetch upstream (on github) and now I am 2 commits ahead of main. Then I pulled it to the local repo and it asked to merge which is now definitely not what I aimed for because it shows

which is even adding more complexity to the tree which I don't want to bring back to the main repo.
So, if I added another commit this would probably make the whole git tree even worse.

My proposal is therefore rather

(1) just either merge this one now as it is (without the additional fix) and provide another really small one later

(2) or should someone from you just edit the one line here on github and then merge this PR? The one line would be to change in blocks-scripts.js

to

(basically change "any" to "String")

PS: You or others may tell me that I should finally "learn how to git" (I heard that once). This is really what I am trying and I am reading a lot about it but I have to admit the keeping all repos in sync all the time is something really complex. And yes, I did tutorials on git and have been in courses but it seems I always end up in situations like that. I can only apologize and if anyone is willing to direct me to the right source to learn how to deal with situations like that I open to take the time.

[ghys]

Thanks!

[stefan-hoehn]

Thanks!

Thanks to you, Yannick