Allow primitives to have "cancel" behavior; cancel "play sound until done" on thread retire #2002
Conversation
|
@kchadha @ericrosenbaum This may be a good candidate for "co-review". :-) |
|
This also closes #706, since Fwiw, this doesn't implement stopping the motor as discussed in #706 and #40; it only adds the support for that feature. It's probably worth implementing stopping the motor in this PR, but I don't have a lego motor or any external devices to do that. Implementing motor-stopping into this would mean adding support for this to extensions (since the motor blocks are all part of extensions), which is a good idea anyway since that looks to be the direction Scratch 3.0 is going now. |
|
@ericrosenbaum @kchadha Bump |
|
@towerofnix sorry it took us a long time to look at this! @kchadha and I were just looking into it, and we realized that Scratch 1 and 2 also did not stop the sound for "play sound until done" when you click the stack to stop it, so that specific thing feels less important than I originally thought when I filed that issue (scratchfoundation/scratch-audio#7). This would be a nice feature though, especially, as you point out, for the extensions that make sound or control motors. We'll investigate the implications of the change a bit further. |
|
After some more discussion, we decided to close this PR, and revisit in the future if needed for internal design and implementation. |
|
@ericrosenbaum alright. Since I spent a while on this, even if it's not help-wanted and I didn't particularly expect it to get merged, I'd appreciate just a summary of thoughts on this PR shared here? |
|
Ping @ericrosenbaum? |
|
Sorry I didn't provide a more detailed response. We appreciate your work on this. Our main concern is the risk of introducing bugs, due to this being a fairly complex feature. We realized that because previous versions of scratch do not have this feature, it's not itself a bug fix but a a new feature. So we need to decide as a group whether we want to add it (which we have decided to hold off on for now). If we do decide to add it in the future, due to the complexity, we'll want to do our own design and implementation of the feature within the team. Your implementation in that case would serve as a useful reference. Hope that helps! |
|
Alright, I understand! Thanks for the explanation. |
Resolves
Resolves scratchfoundation/scratch-audio#7. Closes #40 (as this PR provides an implementation).
Proposed Changes
At a high level, this PR allows classes such as
Scratch3SoundBlocksto specify behavior that "cancels" an actively executing block. These functions are called when you click a script that is already running, and its active block is one of that opcode. The "play sound until done" block makes use of this; if you retire a thread where it is the active block, the sound it started will be stopped.Internally, instances of
BlockCachednow keep track of their "cancel block" function alongside the main block function. This is referred to when a thread is retired; if present, it's called just like the normal block function.Reason for Changes
When a script is executing, it is highlighted with a yellow "glow" bordering its outline. If you click a script while it's executing, it will lose this glow and stop running further blocks in the script. The implication, to the user, is that the script is stopped. Internally, this is what is happening. But, before this PR, "stopping" a thread did not also mean stopping active side-effects. Intuitively, side-effects should be stopped when a script is stopped; otherwise it does not seem so much like you are stopping the the script.
This PR doesn't (currently) implement "cancel" blocks into extensions, but it provides the basic code. Once it's implemented, blocks like "play note for beats" and "turn motor on for (N) seconds" can be changed so that their side-effects are cancelled as soon as the thread is retired.
Test Coverage
I tested this manually. I'm not certain if unit tests are wanted here? I saw that there don't seem to be any for
execute.jsso far, only the higher-level classes (e.g. Thread, Sequencer); this PR doesn't add much behavior to those classes.