Make blockly Else if to an actual else if

Xztraz · Post by **Xztraz** » Thursday 22 February 2018 10:22

As people have figured out domoticz implementation of blocklys Else-If just works as a bunch of stacked if's. Not very logical.
This should be rewritten to work as a real else-if and ordinary if-blocks should be stackable.

Not so easy but blockly is one of the strongest part of domoticzs. it should be given higher priority.

heggink · Post by **heggink** » Thursday 22 February 2018 10:32

Blockly being the strongest part is an opinion IMHO. I personally think dzVents is the strongest part but that's just me

.

Back to Blockly, I may be wrong here but my understanding is that this is a blockly issue, not a domoticz issue. I think you would have to change the blockly behaviour. Not sure how many people in the domoticz community know how to do that to be honest.

Again, I may be completely mistaken.

H

Xztraz · Post by **Xztraz** » Thursday 22 February 2018 10:49

What i see from testing the logic blocks on blockly demo site they produce real else if and such.

: blockly_real.PNG (20.2 KiB) Viewed 8731 times

Produces this lua code:

Code: Select all

if not true then
  for count = 1, 10 do
    print('abc')
  end
elseif blah == 0 then
  print('abc')
else
  blah = 1 + blah
end
if true and not false then
  blah = blah + 1
end

so it's the implementation of blockly in domoticz that's weird.

Xztraz · Post by **Xztraz** » Thursday 22 February 2018 11:38

And yes it's an opinion. But i thought blockly was something unique in domoticz. but after some googling other projects are implementing this too.

It should still follow programming logic

leonmoonen · Post by **leonmoonen** » Thursday 22 February 2018 17:31

I’m a bit lost in your argument... what do you think Domoticz is making of the blockly example you posted?

Xztraz · Post by **Xztraz** » Thursday 22 February 2018 18:10

The example was just to show that blocky outputs real ifs and elses and such. the blockly code is just jibberish but shows it's translated to real code.

blauwebuis · Post by **blauwebuis** » Thursday 22 February 2018 18:23

Fixing this would rock.

I agree that Blockly is a valuable feature that most other open source home automation didn't/don't have. For me it was also the main reason to stick with Domoticz: it has potential to make home automation accessible to a less tech-savvy audience.

leonmoonen · Post by **leonmoonen** » Friday 23 February 2018 5:30

Xztraz wrote:The example was just to show that blocky outputs real ifs and elses and such. the blockly code is just jibberish but shows it's translated to real code.

I understood that part, but you seem to say that Domoticz either doesn’t allow you to create that control flow, or that it interprets it wrongly. Can you give the translation to code that you think Domoticz is using for that same piece of blockly?

nb: I have very similar else-if blockly logic in Domoticz and haven’t seen an issue , which may explain my confusion - I’m not saying there’s nothing wrong, just trying to pinpoint what exactly needs to be fixed

ash77 · Post by **ash77** » Friday 23 February 2018 5:43

It's a pretty simple change. There are four calls to parseBlocklyActions() in CEventSystem::EvaluateBlockly(). Change all four the same way:

Change:

Code: Select all

parseBlocklyActions(it->Actions, it->Name, it->ID);

to:

Code: Select all

parseBlocklyActions(it->Actions, it->Name, it->ID);
break;

Xztraz · Post by **Xztraz** » Friday 23 February 2018 8:55

Ah! maybe my initial explanation was a bit short

If setting up an -if-else- block. its intepreted as 2 -if- blocks after eachother. its not an else condition.
both -if- and - if-else- are ran independent ignoring if it matches the if condition.

This example should never reach the else if. if the first is tru the second should not run

: elseifbug.PNG (12.41 KiB) Viewed 8647 times

and when pushing the button. both rows are executed as expected with 2 separate ifs

Code: Select all

2018-02-23 08:53:10.091 EventSystem: Event triggered: elseiftest_1
2018-02-23 08:53:10.091 -IF-
2018-02-23 08:53:10.091 EventSystem: Event triggered: elseiftest_2
2018-02-23 08:53:10.091 -ELSE IF-

so its converted to something like:

Code: Select all

if nexach3 = on
 print('-IF-')
end
if nexach3 = on
 print('-ELSE IF-')
end

enabling -ELSE- (only else) in the if-else block would also be freaking super.

CLEMENT99 · Post by **CLEMENT99** » Friday 23 February 2018 9:17

Xztraz wrote: ↑Friday 23 February 2018 8:55 Ah! maybe my initial explanation was a bit short

If setting up an -if-else- block. its intepreted as 2 -if- blocks after eachother. its not an else condition.
both -if-
and - if-else- are ran independent if there is something true in the first row.

OK, thanks for this explanation, I understand now my problems with blocky.

Xztraz · Post by **Xztraz** » Friday 23 February 2018 9:32

people end up with super huge and comparator blocks instead of neat nestling.

This needs to be fixed too in the same time if you now would need multiple if rows in a blockly script

: elsifif.PNG (18.42 KiB) Viewed 8635 times

-Second If- never runs

Xztraz · Post by **Xztraz** » Friday 23 February 2018 9:55

The Blockly code generator should just make code in lua and run under drventz or something.
Then the fullish blockly set would be possible to get working. i'm not sure how its interpreted now but its quite limited.

This should be something to look at in the long run. Then the step to lua for even more advanced stuff is easy. just klick edit raw lua and continue from there..

i know lots of work.. but would be a big leap forward.

blauwebuis · Post by **blauwebuis** » Friday 23 February 2018 10:24

@ash77 - wow, is that really the solution? If so, let's create a pull request!

I've created a pull request here:
https://github.com/domoticz/domoticz/pull/2164

gizmocuz · Post by **gizmocuz** » Friday 23 February 2018 15:04

Best to test this solution first locally...

But i don't think this is the fix unfortunately, and it's the way it was designed/implemented years ago.

The OP from this topic also created a github issue where i try to explain how to overcome (his/some) this issue:

https://github.com/domoticz/domoticz/issues/2154

you will see what i mean if you create both blockly's and have a look in the database what is created in the EventRules table
if this is correct, then all will work, but if an advanced system dzVents/Lua/Python is not desired, indeed someone have spent some time on this

leonmoonen · Post by **leonmoonen** » Friday 23 February 2018 15:40

Ok, it’s also clear now why I don’t see the issue, as all my conditions turn out to be mutually exclusive.

One more question for @gizmocuz: Although technically ‘more correct’, fixing this behavior would break the logic of existing scripts. Would that be a concern for possibly ever accepting such a patch? If not, I’d be happy to look into this and try to give something back to my favorite home automation system...

gizmocuz · Post by **gizmocuz** » Friday 23 February 2018 15:43

You mean the patch from 'ash77' ? i dont think that this is 'the' patch and causes only the first 'then' to be executed and then it stops

If the 'EventRules' is correctly populated by a patch, this will not cause previous scripts to break....
(in this case there should be more rules created)

leonmoonen · Post by **leonmoonen** » Friday 23 February 2018 17:16

I wasn't specifically referring to asf77's patch but let's say we have somehow come up with a magical patch that addresses the issue. I think that this does mean that the behavior would change and my question was aimed at finding out if that would be a reason to never accept the patch (there can be good reasons not to want to change the current behavior, but IMO it also would mean it's not worth exploring a potential patch, since it inherently changes behavior).

Let me try to explain why I think it changes behavior / may break existing scripts: after applying the patch, the blockly else-if branch will be interpreted as a real else-if branch, that means its condition will only be evaluated when the condition of the corresponding if branch is false. Then existing scripts that rely on the current interpretation of the blockly else-if logic would "break" to the extend that their else-if branch is no longer evaluated when the if condition is true.

In terms of Xztraz' example,

before the patch EventRules would be populated in such a way that the log would show

Code: Select all

2018-02-23 08:53:10.091 EventSystem: Event triggered: elseiftest_1
2018-02-23 08:53:10.091 -IF-
2018-02-23 08:53:10.091 EventSystem: Event triggered: elseiftest_2
2018-02-23 08:53:10.091 -ELSE IF-

and after the patch EventRules would be populated in a way the log would show

Code: Select all

2018-02-23 08:53:10.091 EventSystem: Event triggered: elseiftest_1
2018-02-23 08:53:10.091 -IF-

If a user expected their script to behave as before the patch, that script is now "broken".

Hope this makes sense, it is surprisingly hard to write down unambiguously

gizmocuz · Post by **gizmocuz** » Friday 23 February 2018 17:43

Thanks, this makes sense!

This patch could be accepted, as all will happen in the first IF, and it should not even go to the ELSE because the above is true.

But, there can be more conditions then one

if (A=1 and B=2) then
...
else if (A=2 and B=1) then
...
else if (A=2 and B=2) then
...
else
..

will this also work? i hope it does, and then of course the patch is more then welcome!

Edit: made a small type above, and corrected it

leonmoonen · Post by **leonmoonen** » Friday 23 February 2018 20:07

cool, sounds like I’ve just got a project to work on

and yes, that multi-condition situation should of course be supported too