Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Event handlers being dockable blocks is confusing (IE ability to nest) #849

Closed
jaustin opened this issue Dec 5, 2016 · 14 comments
Closed

Comments

@jaustin
Copy link

jaustin commented Dec 5, 2016

While it's useful for advanced users to be able to dynamically allocate event handlers, this is an advanced user behavior.

We commonly see people incorrectly putting event handlers inside the 'forever' loop, or inside other event handlers. This leads to unexpected behaviour and frustration:

Two examples of this:
Radio received inside a 'forever' block
image

This doesn't do what you'd expect! When I saw this behaviour the user was expecting the second handlers to fire only after button A had been pressed, but only the first handler ever fires.
image

(Even when the handler is top level, the behaviour is still not as expected - the user expects the code always to enter via the top level handler, but actually after button A is pressed once, the second two handlers fire and never the first?
image
Joe/James Jonny (me!) each thought that this would do something different:
Joe: Only the first handler ever will fire
Jonny: Both will fire for button A, only button B will fire after the first press of A
James: Only the second one will fire afterwards <-- James was correct, it seems from the simulator
)

This is a long way of saying "should we have hats in PXT for event handlers?"
image

  1. Teachers are already used to it from Blocks and Scratch
  2. It makes the kind of odd racey undefined conditions much less possible

Supporting advanced uses seems possible through Typescript

@pelikhan
Copy link
Member

pelikhan commented Dec 5, 2016

Hats were brought in to support rountripping to/from JS.

  • bringing hats would be a major breaking change
  • hats would prevent roundtripping of program with nested handlers or forever loops

@pelikhan
Copy link
Member

pelikhan commented Dec 5, 2016

Note that hats do not solve the problem of registering the same event multiple times (#850).

@jaustin
Copy link
Author

jaustin commented Dec 5, 2016

@pelikhan can you please explain "hats were brought in to support roundtripping"? We can't see hats but maybe I'm missing your point.

Agree it'd be a major breaking change, but that means we should get the right behaviour sooner rather than later, if we're going to make that change.

@jaustin
Copy link
Author

jaustin commented Dec 5, 2016

In reference to #850 - if using hats we could more easily warn/disallow more than one event handler per event - whereas the current behaviour it's quite valid to later re-set a handler, with hats we'd rule that out in blocks, and take the 'hit' of being unable to round-trip if/when someone adds event handlers dynamically/not in a hat.

For simple round-tripping, what about the rule "event handers at the top level get converted to have hats, event handlers anywhere else get converted as normal blocks or don't get converted at all"

@pelikhan
Copy link
Member

pelikhan commented Dec 5, 2016

As you noticed, if someone uses a nested event-like interface (forevoer, on...) in JavaScript, it would not be possible to reconstruct it back in blocks.

@whaleygeek
Copy link

There are two main use-cases here.

  1. user is just starting out and they want a single event handler (in which case they don't understand event handlers and they always put them in a forever loop, which affects performance and causes all sorts of glitching and bad experience).

  2. User knows a little bit about what they are doing, and they effectively implement a state specific event handler. This is a beautifully simple way of implementing some complex logic.

It feels to me that we need to support round-tripping for both use cases. So, I'm thinking that we need to fix this in the docs really, with some simple examples.

It might additionally be possible to catch the common case, which is only an event handler in a forever loop, and pop up a warning box saying 'did you really mean to do this' with a link to the documentation explaining how the event handlers work.

That 'misses' the case of where people have a forever loop with an event handler in it and other stuff (again thinking of it like a poller rather than an event handler). So, you would catch this case too if you noticed any event handler that was anywhere inside a forever loop (if it appears inside a deeper level like inside an if statement inside a forever loop, that is probably intentional).

I sort of think it would be good to have a way to warn people they might have made a mistake, rather than preventing them using this alternative design pattern in the visual language. A bit like C++ compilers say 'possibly incorrect assignment in if statement' - sometimes you might want to do that, but it's a common bug case.

@whaleygeek
Copy link

@jaustin I'm not sure your conditional 'hat wearing' solves the problem? People will often put down a forever, then put an event handler inside it, so that will 'miss' this error/warning, because you are assuming people will put in the event handler first (which gets a hat) and then wrap a forever around it. I see people doing it the other way (they put in forever first, then the event handler inside it).

@EdNutting
Copy link

EdNutting commented Dec 8, 2016

I'm not convinced by this idea of forever loops or event handlers being nestable. In real world software, it's universally indicative of "clever" but bad design. From a teaching point of view, it's impossibly complex to explain.

The block editor, IMHO, shouldn't be trying to reproduce everything that's possible in JS. It should be providing the clearest, simplest route for teaching the fundamentals of programming. Being able to nest forever loops and event handlers is neither clear nor a fundamental of programming. It makes it very difficult to explain to bright kids the intended operation of code and even more difficult for less bright kids to get their heads around what the blocks mean.

For example, chained forever blocks execute in parallel. That makes literally no sense...sequential blocks executing in parallel is nonsense. So outermost forever blocks should not be chainable. Likewise event handlers.

It'd be better to teach the design pattern of one event handler then set variables based on conditions / state that is checked within the handler. This also produces cleaner, safer code that avoids many pitfalls. And again, sticks to industry recommendations. I have never seen a good system that dynamically adds events as a way of conditionally enabling actions. Nested forever doesn't make sense unless a break statement is possible (which I don't recall seeing) and a while-true loop is more appropriate if children are trying to use that kind of complex design construct.

As a teacher, as a software engineer and as a computer science student, I'd like to see nested forever loops and event handlers removed.

(Edits: my mobile autocorrect is having a very bad day :) )

@WilliamMayor
Copy link

I saw a lot of students using the forever block as simply "the place that all the code goes". It's the first example you see and I think they just assumed that you had to put everything there. Some of them even had to go hunting around for the "while true" block because they wanted to repeat something forever but didn't know how. When I explained how the forever block worked they were surprised.

They were also caught out by the event handlers overwriting each other. In every similar examples to @jaustin.

How about having some static blocks in the editor for the common events. You can't move them around or delete them, you can only add blocks into them. That way there's an obvious place to put all of the handler logic. There could also be a static init block?

If the first example uses the static blocks instead of the forever one then I think fewer people would confuse its purpose.

@pelikhan
Copy link
Member

After discussion, we have decided to bring in hats. Work in progress in "hats" branch.

@pelikhan
Copy link
Member

PR #925

@pelikhan
Copy link
Member

The "unpluggable" events are now live in beta.

We are doing a best effort to load existing scripts without messing them. Please tell us if the conversion went bad. The strategy is simply: try loading blocks. if fail, try loading javascript. Scripts that have nested events are still valid in javascript but not in blocks anymore.

@pelikhan
Copy link
Member

Duplicate events are now detected and live in beta. I think this covers this bug.

#940

@pelikhan
Copy link
Member

"on start" button now required for initialization code.
#945

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants