search

nengo / nengo-gui

Nengo interactive visualizer
Other
99 stars 38 forks source link

Undo/Redo creation/deletion of plots breaks when code in the editor is changed #322

Open bjkomer opened 9 years ago

bjkomer commented 9 years ago

If you create a component (value plot, slider, etc) and then modify the model in the code editor (i.e. remove a connection or ensemble, doesn't even have to be directly related to where the component came from), and then undo and redo the component creation, things get messed up. The component will get created again, but with a uid something like <Ensemble (unlabeled) at 0x7f7235a98e50> and throw a key error.

s72sue commented 9 years ago

I am unable to repro this one. Which browser are you using?

bjkomer commented 9 years ago

This was in Firefox, but I just tried it in Chrome and it is happening there too.

Here's what I did

It looks like part of my comment got cut off (apparently < and > mess things up), the uid I'm seeing is something like: Ensemble (unlabeled) at 0x7f1ba7097e50

tcstewar commented 9 years ago

Ah... that uid is what happens when it can't find an item. So the undo system might be able to detect that this has happened....

bjkomer commented 9 years ago

There would have to be a lot of detecting going on. We also run into problems if someone moves an ensemble, then removes that ensemble from the code editor, and then tries to undo it. Is there some sort of resetting going on that changes uids when code in the editor is changed? I'm just not sure how it is not finding the item, it even draws the graph back in the correct location just before throwing the error. I need to look into it a bit more.

What should the correct approach be to undoing an action that cannot be performed? One option is to detect it when it comes up, and just skip over it (I think this should be straight forward in every case except for auto-layout). Another could be to figure out what changed in the code and strip out all items in the stack with matching uids (this sounds messy and prone to bugs). A third option would be to just delete the undo stack when you change the code (simplest thing to do, but not a very good option).

tcstewar commented 9 years ago

Is there some sort of resetting going on that changes uids when code in the editor is changed? I'm just not sure how it is not finding the item, it even draws the graph back in the correct location just before throwing the error.

The reload system in components/netgraph.py tries its hardest to figure out what the matching uids are when things change, but it's a tricky problem.

What should the correct approach be to undoing an action that cannot be performed? One option is to detect it when it comes up, and just skip over it (I think this should be straight forward in every case except for auto-layout). Another could be to figure out what changed in the code and strip out all items in the stack with matching uids (this sounds messy and prone to bugs). A third option would be to just delete the undo stack when you change the code (simplest thing to do, but not a very good option).

I'm curious what option 1 would be like -- that might feel kinda okay. Option 3 is also at least clear and straight-forward, so we may have to fall back on that.

The coolest option (4) would be to integrate with ace's undoManager. That might actually be doable... http://ace.c9.io/api/undomanager.html

tcstewar commented 9 years ago

Better docs for that, including a discussion of inserting your own actions into the undo stack:

http://cloud9-sdk.readme.io/v0.1/docs/the-undomanager

tcstewar commented 7 years ago

It's definitely a big task to merge the two undo stacks, but I think it'd be worthwhile to at least fix this so that it doesn't break, and do the merging in a separate PR. Even something as simple as clearing the netgraph's undo stack whenever a change happens in the code editor would be a good temporary measure.