Group-Handling with blockly

Good you found out yourself. I actually overlooked that part in the blockly reference document. I won’t be able to do it tonight but will do tomorrow.

Thanks @Christian_Kittel by the way for trying that out so much. I would have hoped we had had you earlier on board. I will check tomorrow why the direct use in the condition doesn’t work and get back to you asap.

cc @Christian_Kittel

First of all, I had to look what is happening and I will explain that (which is partly, I am sure, what you meant, Rich).

Buckle up, the explanation becomes a bit complicated (and bear in mind the time of writing :zzz:)

First let’s look at this

A) Why doesn’t “get xxx of item” connect?

This actually depicts an issue I have described in the blockly reference as a bug. Unfortunately a wrong blockly output type was used which does not allow the block to connect. There is a bug fix PR already waiting. Depending on the chosen attribute it now produces either a String or an oh_item, which works well.

The reason you made it possible to work with is via the Variable-Workaround which basically tricks blockly and let’s you assign anything which is a workaround for that bug. Hence, you can use it in any block and therefore in your if-statement.

B) Why does “get state of item” throw an exception?

First, the log works because it just asks any given block to produce its String-representation. That is why it works.

Now let’s look the following two examples in the for-loop:

with the related code that is generated

var member_list = Java.from(itemRegistry.getItem('Lights').members);
for (var member_index in member_list) {
  member = member_list[member_index];
  logger.info(itemRegistry.getItem(member).getState());
  logger.info(member.getName());
}

The second line in the loop uses (the bug fixed) “get xxx of item”, which is a more generic version of the up to now existing “get state of item” in fact expects a “oh_itemtype” as an input type and references the following block which produces “itemRegistry.getItem(‘MyItem’)”

oh_itemtype image

As a result the block
“get state of item” image
wants you to only allow the oh_itemtype to be attached like so

I just noticed that can even attach a thing-block which is hard to be prevented because neither the thing- nor the item-type is “typed”, so it connects everywhere. I am hesitant to add type-awareness to these at the moment because I fear the side effects that might break zillion other places

Anyways, as you can see above, if you use the right block it expects (a real item) it produces

logger.info(itemRegistry.getItem(member).getState());

because the “real item” produces

itemRegistry.getItem('myItem")


So why does the first line then throw an exception?

image

Well, it expects not a “real item” but only the item reference (the green block) which simply produces ‘myItem’ but member is not a String but an Object, so you get an exception (you cannot ask the registry to retrieve an item by providing an item).

But why does it then allow you to put into that block if it throws an exception?

The reason is that blockly assigns that to a variable which then loses its type in the code. So during the code generation I (the programmer of the blocks) have no clue what code should be generated (with our without itemRegistry). So the only smart idea (and now I am back at Rich) would be to generate a code that is capable of working with both by checking the type during runtime (by the way, Rich uses the new Javascript engine here with item which is not yet supported by Blockly… I know Rich :wink: )

In summary, yes, Rich, it can be done because the whole situation above is too hard to explain anyone, so it must become more fool proof internally. Hence, I started a Proof of concept (even more :zzz:) and I was able to achieve that:

So both ways work now because it produces the following code (which doesn’t look nice but at least does work behind the scenes):

var logger = Java.type('org.slf4j.LoggerFactory').getLogger('org.openhab.rule.' + ctx.ruleUID);
var member_list = Java.from(itemRegistry.getItem('Lights').members);
for (var member_index in member_list) {
  member = member_list[member_index];
  logger.info((member instanceof org.openhab.core.items.GenericItem) ? member.state : itemRegistry.getItem(member).getState());
}
logger.info(('Lights' instanceof org.openhab.core.items.GenericItem) ? 'Lights'.state : itemRegistry.getItem('Lights').getState());

I also changed this for the “get xxx of item”-block.

The fix is now part of [blockly] Fix item.getAttributes by stefan-hoehn · Pull Request #1254 · openhab/openhab-webui · GitHub


(i have added this thread to Blockly Reference as a known issue)

1 Like

Actually my example was Nashorn. In Nashorn items['MyItem"] gives you the current state of that Item. So it’s the same as itemRegistry.getItem("MyItem").state.

In JSScripting, items is your one stop shop for all things related to Items, not a dict of the Item name/state pairs. To get the state of an Item using JSScripting’s items you’d need items.getItem("MyItem").state.

One of the nice things about this though is that it doesn’t really matter because all the user really sees is the block. So even if the code backing it has lots of complexity like this it’s not so bad.

Hi @stefan.hoehn,

if this pull request is part of the next MileStone-Build, i will test it.

Tanks for your help

I am sure it will be. Just watch it to be merged and it will be then part of the snapshot and eventuelle on M1

1 Like

I would respectfully disagree here. To me the endgame for Blockly scripts is not only to abstract away the actual code, but provide some “good” reference code, including for educational purposes. Cluttering it with safeguards to avoid misuse is somewhat defeating the purpose, IMO. See my remark here: [blockly] Fix item.getAttributes by stefan-hoehn · Pull Request #1254 · openhab/openhab-webui · GitHub

Educational purposes? Hm, this sounds a bit like always having to find difficult compromises. Naturally, code that is very robust needs many safe-guards, exception handlings, etc. and thus becomes less readable and imho usually not a good candidate to show to newbies that want to learn programming - the “essentials” of the code are cluttered by all the other details…
I’d personally rather consider blocks to be black (or rather colorful) boxes - as a user, I don’t really care what’s in them as long as they do what I expect them to do. Just my two cents.

5 Likes

This is probably a place where we will disagree.

Based on what I’ve seen so far, Blockly isn’t a bridge to “real code”. It is going to be the first and only language most of the non-technical users will use. That will be even more the case as the Blockly library builds up with more choices on in the marketplace. I would not be surprised if it becomes the most used language over all. Forcing these users to have to understand and avoid type errors, the number one problem these users face, because it’s good for them, is, IMO, user hostile to these types of users (and a big pain in my side as forum support since, as I said, type errors are the most common problem these users face).

Even before 3.2, with all the limitations and missing features, the uptake in Blockly use was breathtaking. It’s already very popular.

Like I argued in the GraalVM issue, we need to try to avoid forcing work and expectations on those users least able to carry the burden. If the leap between Blockly and text code for rules is just a little bit more, I think I’m OK with that.

tl;dr, treating Blockly as a stepping stone towards “real” code is a mistake; Blockly needs to be a first class rules language because that’s where a ton of users will stop.

3 Likes

Ok, I see your points.

My counterargument is my non-OH example over at GitHub:

image

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

the code for the standard “make text from list” block isn’t something like
(Array.isArray(n)) ? ... : ...
which in 99% of the cases when you look at the code trying to figure out what your blocks are doing (you can hit Ctrl-B in the Blockly script editor to see the actual JS) is only added complexity if you just assume n to be an array.

The fact is, there’s some confusion on what accepts an identifier (item name or thing UID) as an input and what accepts the whole object. I would have hoped the color-coded hints (greenish blocks return a string) helped but in this case - Item instance vs. item name - apparently it’s not enough.

Maybe we should introduce a distinct color scheme for GenericItem instances (you’ll notice in the “openHAB>Persistence” the first 3 blocks have a distinct color because their output is a ZonedDateTime.

Another thing that could make it more clear would perhaps to rename the “get state of item” block to “get state from item named” or something even more clear than color. Color alone often is not sufficient for many users including those with color blindness. But don’t get me wrong, I like color too, just not only color.

That change in text alone makes the blocks a little more clear what is going wrong and why it failed when trying to implement it in the obvious way by linking the Item block to “get state of item”. Color would provide more help and some docs to wrap it all up would be nice to.

On-the-other-hand, it seems pretty clear and intuitive from the end user perspective to make at least that one “get state of item” block be able to handle both names and Item Objects.

NOTE: There was a brief discussion on adding some of this sort of type conversion stuff to the JS Scripting library so, when the time comes to migrate Blockly to JS Scripting perhaps some of what currently looks ugly will just become a function call.

As I said, type issues are the number one issue with rules development so I personally want to push as much of that as is feasible away from the end users.

For one example, I’ve a “todatetime” function in my libraries which will convert just about anything you pass to it into a ZonedDateTime. For example:

  • “3h2m” → now.plusHours(3).plusMinutes(2)
  • DateTimeType → getZonedDateTime()
  • Number, DecimalType → now.plusNanos( number * 10000)
  • JavaScript Date → equivalent ZonedDateTime

and so on. So most of the time one can call todatetime and pass it almost anything that makes sense as a date time or a duration and you’ll get a ZonedDateTime suitable for use in a timer. The end user never needs to care about types here. I’d like to see something like that added to the library as well as something similar done for calculations and comparisons that deal with DecimalTypes, Numbers, primitives and quantity types.

My point is any ugliness added for this stuff now in Blockly might be temporary.

That’s exactly the kind of synergy I hope we can achieve in the end, with the openhab-js and Blockly parallel breakthroughs in JS scripting ultimately profiting from each other.
As another example, openhab-js now includes js-joda which has the same API as Java to handle ZonedDateTime and the like, so ultimately the Blockly blocks wouldn’t have to instantiate Java’s ZonedDateTime but work with the JS classes provided by js-joda.

I agree renaming labels to try to avoid confusion could do wonders too.

1 Like

Sorry for that dump question (perhaps it is the late hour,) but where I can find the “create text” block from your rule?

Thank you!

@johannesbonn Enjoy the awesome screenshot:

Ah ok, I saw this block but I think there is a problem configure it with my iPad and Safari. I will give it a try on my PC.

Thank you!

So, regarding my Bugfix-Pullrequest, which I think we should get out of the door soon, can you guys tell me what (or if) I should change because I am really not sure what is expected now from me. I would really like to have that fixed in a fail safe way so our users do not run into issues (by sacrificing a bit of a code generation elegance). I am not hesitating to change something for a better paradigm in the future but I think we shouldn’t do that for a bug fix.

The other topic I like to be decided in general if we favor blockly-user-fail-safeness over code-generation complexity or do we see blockly’s code generation as a “perfect example” for people how to write openHAB Javascript code? My opinion is that we should of course write very good blockly code but code that is being generated will always be worse than can code written by humans nor is it intended to be (and that IMHO is a general statement for code generation without becoming too philosophical about it :wink: ) .

The main reason I favour fail-safeness of usage to me is what Rich already said: it will reduce a lot of our time in guiding the people what and what not do to to avoid errors as 90% of at least my target user group are not developers (besides the effort I currently put into documentation).

The other reason why I like this to be merged soon is that I have a couple of other changes waiting and due to my git-incompetence I do not like to work at two things at the same time…

@Andrew_Rowe opened another thread for a generic discussion on this topic. [Discussion] Blockly implementation in OH

From my perspective, is less about making the code foolproof and more about not violating expectations. More details on that thread.

To be you’ve several options:

  1. Make the “get state of item” work with both item names and item Objects

  2. Make the block that iterates over members if a Group map the list so it’s just a list of item names instead of Item Objects

  3. Change the name of both blocks to make it intuitively clear that get item state only works on Item names and when iterating over a Groups members you are getting the whole Item Object.

I personally like 1 or 2 better as it gives a better end user experience. I’m OK with 3. I’m not OK with doing nothing (not that it matters much what I’m OK with in the first place).

No matter what approach is taken, when migrating off of Nashorn it should be revisited as a lot of code complexity might be hidden inside openhab-js which could give us the best of both worlds.

re 1) I vote for (1) as it is being implemented as it is the most intuitive from the user perspective
re 2) If I get you right that would make the behaviour incompatible with what is currently happening and would also require quite some rework.
re 3) I would maybe make the block clearer but makes our blocks less versatile like (1) would do.

Don’t miss what I said last though. No matter what we do now, when we switch to JSScripting we can have 1 without the extra complicated code.

So wait and see might be better and go with clearer names for the blocks.

If we wait and see and do nothing for now, then we have the problem why the whole thread was started, haven’t we? Honestly, I was trying to help and now the discussion has cost me much more time than everything else which frustrates me. Maybe I should rollback the PR and someone else takes over. :cry:

I didn’t say do nothing. That’s not really acceptable in my mind because it doesn’t solve this problem.

I’m proposing we solve it but changing the names of the blocks to be more clear what’s going on.

When we move to JSScripting the whole problem goes away I think because if how the helper library already implements items. So even if we implement 1 or 2 more, it will have to change in the not to distant future anyway. Since it’s controversial, maybe it’s better to wait a little bit and revisit when we do move to JSScripting.

To state again, I’m not suggesting doing nothing. But I am suggesting we not spend too much time and stress over something we know will change soon anyway.

1 Like