Skip to content

Conversation

flatsiedatsie
Copy link
Contributor

@flatsiedatsie flatsiedatsie commented Feb 23, 2018

This is a pull request based on the comment by Ash77 here:
https://www.domoticz.com/forum/viewtopic.php?f=31&t=22320&p=172288#p172288

He claims that in order for blockly's "If Else" functionality to work as most users would expect it to, adding these 4 break statements would suffice as a fix.

Now, pay in mind:

  • I have no idea what I'm doing here. I am not a C coder, so I don't know if this really is the fix. I'm just the messenger, don't shoot!
  • More importantly: adding this change might mean that existing blockly's, where users have worked around this issue, might behave differently.

Still, my opinion is that it's worth it to fix this.

  1. Change is good:
  • Beta users accept that they can expect small changes in how Domoticz works.
  • Following standards and conventions is a good practice.
  1. Impact might be limited:
  • Since most users don't use the "else" because it is broken, it may be fair to assume that they haven't used it that much in their scripts.
  • The scripts of users that created 'else' functionality in other ways (for example by using variables) would still work fine.
  1. There is a big demand:
  1. Blockly is worth the effort:
  • For many users Blockly is a core reason they like Domoticz. Me included. Even if this patch is not the fix, having a blockly system that works as most people would expect it to would be a worthy cause.

This is a pull request based on the comment by Ash77 here:
https://www.domoticz.com/forum/viewtopic.php?f=31&t=22320&p=172288#p172288

He claims that in order for blockly's "If Else" functionality to work as most users would expect it to, adding these 4 break statements would suffice as a fix.

Now, pay in mind:
- I have no idea what I'm doing here. I am not a C coder, so I don't know if this really is the fix. I'm just the messenger, don't shoot!
- More importantly: adding this change might mean that existing blockly's, where users have worked around this issue, might behave differently.

Still, my opinion is that it's worth it to fix this:

Change is good
- Beta users accept that they can expect small changes in how Domoticz works.
- Following standards and conventions is a good practice.

Impact might be limited
- Since most users don't use the "else" because it is broken, it may be fair to assume that they haven't used it that much in their scripts.
- The scripts of users that created 'else' functionality in other ways (for example by using variables) would still work fine.

There is a big demand:
- Many users, me included, run into this problem when they start using Domoticz. Here are just a few thread on the forum:
https://www.domoticz.com/forum/viewtopic.php?f=62&t=16535&p=123167
https://www.domoticz.com/forum/viewtopic.php?f=28&t=15908&p=119285
https://www.domoticz.com/forum/viewtopic.php?f=62&t=14802&p=108423
https://www.domoticz.com/forum/viewtopic.php?f=23&t=10911&p=78158
https://www.domoticz.com/forum/viewtopic.php?f=23&t=10706&p=76394
etc..

Blockly is worth the effort:
- For many users Blockly is a core reason they like Domoticz. Me included. Even if this patch is not the fix, having a blockly system that works as most people would expect it to would be a worthy cause.
@gizmocuz
Copy link
Contributor

Was just going to look at the if/else issue. Could you make the patch against the development branch ?

@flatsiedatsie
Copy link
Contributor Author

I tried, but the dev branch looks different. I don't know how, as I'm not a C coder.

@szczukot
Copy link
Contributor

This pull request is for master - not for development.
In development branch, the code looks completely different

@gizmocuz
Copy link
Contributor

Patches needs to be created against the development branch...
But as this patch does not solve the problem that is being discussed on the forum, it is safe to close this PR

@gizmocuz gizmocuz closed this Feb 26, 2018
@gizmocuz gizmocuz deleted the flatsiedatsie-patch-7 branch April 1, 2018 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants