jmoenig / Snap

a visual programming language inspired by Scratch
http://snap.berkeley.edu
GNU Affero General Public License v3.0
1.51k stars 744 forks source link

BJC students losing projects by loading Embroidery library #3409

Closed brianharvey closed 1 week ago

brianharvey commented 1 week ago

https://piazza.com/class/h8yx482idav52y/post/m2dcc8qfs035rc

This isn't a bug, exactly; it's behaving as designed. But it's causing problems in classrooms, and neither the students nor the teachers discover the "Restore primitives" option. They then panic, delete their projects, and start over.

I guess what's happening is that the students are curious about the libraries and just try loading one to see what happens. Such curiosity is just what we want to encourage!

I'm not sure exactly how to make this more discoverable/fixable/preventable. Here are some ideas.

  1. When you load a library that redefines primitives, you get an "are you sure" warning dialog.

  2. The Embroidery library should catch the specific error of _embroidery undefined and should suggest "Restore primitives" to fix the problem.

  3. More generally, any error in a redefined primitive should suggest "Restore primitives." (This could be done at the level of Snap! code with SAFELY TRY in the relevant redefinitions.)

  4. Make "Restore primitives" a button in the scripting area instead of a menu option.

jmoenig commented 1 week ago

I'd like to discover how exactly the Embroidery library ends up in these students' projects. I'm not convinced that they explore libraries on their own and decide to load this particular library. Maybe they mis-click when in reality they wish to load either the 3D Beetle or the World Map modules. Or - much more likely - those blocks originate from some starter project which they get from a teacher.

Making restore promitives more prominent is not a solution I want to pursue. It's - once again - interesting, how this problem only surfaces in very particular schools and classes, and never in others, that's why I presume that there might be a different reason altogether. In those environments where teachers or students are actually aware that they installed the Embroidery library, they don't need to restore the primitives, because they want the library. The problem here seems to be that neither the teacher nor the students know that they loaded it, and even at some time tried to unload it again. Trying to unload seems to perhaps be the problem. Frankly, I've personally seen the unload unused blocks glorified in BJC PDs way too often for my liking, touted as a general solution for cleaning up projects before submitting them.

brianharvey commented 1 week ago

They aren't getting the Embroidery library in a starter project in a BJC class. All those starter project vastly predate the Embroidery library!

The OP was doing Brick Wall. Here's its starter project; see for yourself. https://snap.berkeley.edu/snap/snap.html#open:https://bjc.edc.org/bjc-r/prog/3-lists/U3L1-brickwall.xml

As for 3D Beetle or World Map, if they're loading either of those into their class projects it's again just out of curiosity; we don't use those.

But, Jens, it's more than one teacher and more than one curriculum page. Whatever the reason, it's making trouble for kids, and just saying "the student or the teacher is doing something wrong" when there are things we could do to ameliorate the problem seems suboptimal.

What's wrong with the "Unused blocks" feature to clean up a project? That's what it's for! It's a great feature! Anyway, I tried loading Embroidery into that starter project and then doing Unused blocks, and although it does leave the GO TO primitive changed, it doesn't seem to break the project.

(Btw, Unused blocks doesn't remove the _embroidery variable, which is hidden, so it doesn't even appear in the dialog, but I guess anyway it's not unused if a primitive has been modified to use it. So loading and unloading Embroidery doesn't show the behavior they're reporting. You have to unhide the variable and then remove unused blocks again to make it disappear.)

(Also, maybe removing unused blocks should remove categories which become empty after the block removal.)

I kinda wanna suggest that Unused blocks should offer to Restore primitives... I can see that sometimes that's not what you want, so it shouldn't always silently do it, but maybe it should offer.

jmoenig commented 1 week ago

What I'm saying is that I want us to understand where this problem is coming from, and how exactly it comes to pass, before we jump to engineer some workaround.

If a student simply imports the Emboidery library nothing should ever break. I'm not understanding how they end up in this situation. What I expect teachers to do, is to interview such students what exactly they did. Then we can think about what to do about it, and how to help them. Right now we're in the situation that we don't know if they even loaded that library into the starter project, whether they did, in fact, try to revert by unloading unused blocks, or what the sequence of actions that lead to this situation was at all. All I'm reading is "I didn't do anything, it's a quirk in the language", and that clearly cannot be the case.

It's very important to me to point out that loading the Embroidery library does not break a project. At least that's what I think. Can we reproduce this situation at all?

jmoenig commented 1 week ago

One thing we could do is to save a log of the last, say couple of hundred, user interactions in the project. We already have an api for this. Then we could look at a project and it would tell us how it got into the state it's in....

jmoenig commented 1 week ago

Okay, I think I'm onto something...

jmoenig commented 1 week ago

There might be a bug...

jmoenig commented 1 week ago

I'm puzzled. If someone unloads "unused blocks" on a project with the embroidery library it might remove too many blocks. But that doesn't explain the particular issue students / teachers report...

jmoenig commented 1 week ago

Okay, I've found an actual bug: When removing allegedly unused (custom) blocks Snap currently does not scan customized primitives. Therefore it will unload too many custom blocks. The next time the project is deserialized those removed blocks will cause customized primitives that rely on them to fail, and there will be "undefined" blocks in those definitions. This is clearly a bug, and I'm fixing it right now for a quick patch.

I hope that this addresses the underlying issue, as it will not let users accidentally "break" their projects. But I'm not sure if it is, in fact, at the bottom of the reported issues. We'll have to see about this. It still would be very useful to know exactly what the users did that led up to the reported issues.

Anyway, I hope this patch I'm about to commit will, in fact, fix it.

jmoenig commented 1 week ago

So, I've just released and deployed a patch with this fix. I now can no longer manage to break a project by loading the embroidery library and then by unloading "unused blocks" again. Let's see if that solves those problems.

One thing to remember: Libraries that customize primitives can not be removed entirely easily. Instead, to really remove all their blocks, you will at some point have to also restore the primitives. After that you can then remove the remaining blocks.

brianharvey commented 1 week ago

Right, that's why I'm saying Unused blocks should say something like "This project has modified primitive blocks; should I also restore them to the standard version?"

brianharvey commented 1 week ago

But thanks for finding the bug! That's great.