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

assigning a parent to a sprite does not update the palette with inherited blocks #2250

Closed
s5bug opened this issue Nov 8, 2018 · 11 comments
Closed
Assignees
Labels

Comments

@s5bug
Copy link

s5bug commented Nov 8, 2018

Video

Currently inheritance is not working as it is shown in other videos. I am unable to Inspect to see JS errors due to my school district blocking it on these computers.

@jmoenig
Copy link
Owner

jmoenig commented Nov 9, 2018

I'm having issues following what you do very quickly in the video. Could you please share the project and some notes as to what you're expecting and what actually happens? Then I will be happy to investigate. Thanks!

@jguille2
Copy link
Contributor

jguille2 commented Nov 9, 2018

Hi Jens,

I've also reproduced the issues reported. I share my notes (appearing in any project):

  • When I make a sprite child of other sprite:
    • If the parent have local variables, they appear immediately.

    • If the parent have local blocks, they are not appearing. Then, two options:

      • If I check the "inherited" anywhere (in any element), suddenly the local blocks appear, and they appear as "inherited".
      • If I remake the action to make the sprite child of the other sprite (the same action it has already been made), the local blocks also appear, but here it appear as "not inherited".

I think this is the current behavior and the issue.

@jmoenig
Copy link
Owner

jmoenig commented Nov 9, 2018

Thanks, Joan! That indeed explains it, and yes, I can reproduce this, also. Luckily I know how to fix this. Will do!

@jmoenig jmoenig changed the title Inheritance currently broken assigning a parent to a sprite does not update the palette with inherited blocks Nov 9, 2018
@jmoenig jmoenig self-assigned this Nov 9, 2018
@jmoenig jmoenig added the bug label Nov 9, 2018
jmoenig added a commit that referenced this issue Nov 12, 2018
@jmoenig
Copy link
Owner

jmoenig commented Nov 12, 2018

thanks! I've patched the current dev version with a fix for this. Note that as you change a sprite's exemplar (parent) it will inherit the new parent's methods, but before that it will also shadow its currently inherited ones, if any. "Shadowing" methods currently incomplete, i.e. it will only retain a version of the method's header, not a copy of the definition's body. This ensures that existing scripts using inherited blocks won't be ripped apart, but you'll have to edit the shadowed definition in order to specify a desired behavior, otherwise shadowed inherited blocks will just be no-ops.

@jguille2
Copy link
Contributor

Thanks Jens,

I've tested and it's fine.

Only a small issue is still there...

When I make a Sprite2 child of Sprite1, Sprite1 local methods are now appearing, and they are inherited. Perfect!
But if I remake the action (to make Sprite2 child of Sp1), the inherited property changes and all the blocks change to be "not inherited".

Yes, I know that it's not important (it has no sense to remake its child association), but the result (changing the inherited prop) is wrong.

Thanks!

@jmoenig
Copy link
Owner

jmoenig commented Nov 12, 2018

Hi Joan, what you're noticing is intentional, it's what I've described one post above yours, or am I wrong about that? When you change a sprite's parent, all heretofore inherited methods become shadowed, i.e. they become "own" blocks of the emancipated sprite. But I'm only creating the headers, not copying the actual methods. That way you can either manually delete the shadowed method stubs or edit them to adjust their behavior the new situation. Does that make sense?

@brianharvey
Copy link
Collaborator

I would be happier with either not shadowing (so that the user manually fixes any problems that arise) or shadowing the entire procedure body (taking a guess as to what the user meant) than with this in-between idea of shadowing only the header. That presents the user with a bunch of procedures that no longer do what's expected, even though they don't complain no such procedure. It won't be obvious to the user how to fix the resulting bugs. (Yes, after some debugging effort the user will see that a procedure's body has disappeared, and then they'll file a Snap! bug report!

Do you have a situation in mind in which a user will want this behavior? Bear in mind that it won't be obvious to the user which blocks have had their bodies disappear.

@jmoenig
Copy link
Owner

jmoenig commented Nov 12, 2018

The reason I'm shadowing the inherited method headers is that existing scripts don't get broken up. Setting a parent attribute is a somewhat "black arts" methods of turning a sprite into a clone, or of refactoring a clone for another inheritance hierarchy. The normal way is to clone a prototype, in which case this whole issue does not arise. The use case I have in mind is someone plays with setting a parent, and then wants to revert that. If all the scripts get broken this will be unduly hard. So, the scripts get preserved while switching parents back and forth, and once the user has decided on the "final" parent, they can simply mark the relevant custom methods as "inherited" again.

Yes, I agree that shadowing the original methods would be nice, and it's probably what I'll end up doing, but it's tricky in some ways. So, while it's not strictly necessary just yet I would still love to give users the feature to set a sprite's parent, even while it's not completely done yet :-)

@jguille2
Copy link
Contributor

Hi, for me, there are two different things...

  1. When user remakes the action to be child of the same sprite
  • This is what I wanted to report.
  • Here we are no changing the sprite's parent. Yes, it is a silly action... but it is possible (in fact, it was in the video, the origin of this issue). I guess it occurs more often than I thought, when users are testing... and they want to make the relationship and they do not realize that the relationship already existed before.
  • What I expect is that Snap! does nothing when I choose a parent that is already the sprite's parent.
  1. When a sprite change its parent (for other or for none)
  • Current behavior is not bad to me
  • I understand that copying the entire procedure (not only the header) could be delicate (I have to think better about aside effects)... but it could be good. In fact, in the current behavior, local variables are copied with its value (not only the header). So all the scope is copied... and it seems we can copy the entire procedure without problems.
  • And a good thing to me was that, if some of these local blocks or variables inherited has not been used anywhere, they can be deleted. We are talking about sprites made child (not copies nor clones). Then, if we break the relationship, all the inherited that has not been used could be erased.

@brianharvey
Copy link
Collaborator

I agree that changing to the same parent should be a no-op.

Otherwise, I would prefer the simplest possible behavior. If a child is orphaned (including by reparenting), the formerly inherited methods are no longer in the palette. If such a method is used in a script in the child, that block remains in the script, and when the script is run, we give a runtime error message about a nonexistent block. Then the user gets to choose how to fix the problem.

Sometimes I'm in favor of doing favors for the user, but not in this case; it just doesn't feel intuitive to me.

By the way, someday we should talk about multiple inheritance, as in Object Logo.

@jmoenig
Copy link
Owner

jmoenig commented Nov 13, 2018

One day I wish I could just fix a bug and that admitting that something is still under construction wouldn't trigger endless debates :-/

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

No branches or pull requests

4 participants