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

Add undo feature #13

Closed
prmr opened this issue Jan 10, 2015 · 10 comments
Closed

Add undo feature #13

prmr opened this issue Jan 10, 2015 · 10 comments
Labels
feature A feature request
Milestone

Comments

@prmr
Copy link
Owner

prmr commented Jan 10, 2015

No description provided.

@EJBQ
Copy link
Contributor

EJBQ commented Feb 6, 2015

I'm still working on this. The foundation seems solid, based on the enhanced Violet source with commands and an UndoManager. Right now movement seems like the hardest part, since it's done in many pieces in a different section, harder to do under Demeter's Rule.

@EJBQ
Copy link
Contributor

EJBQ commented Feb 6, 2015

There are issues with the method split between GraphPanel, GraphFrame, and Graph. I need to use Graph for adding nodes, which requires a reference there, and removing stays inside the Graph too, so I can't track every deleted Edge by staying only in GraphPanel.

@EJBQ
Copy link
Contributor

EJBQ commented Feb 6, 2015

Also, to update the graph after undoing you need to move the window a bit. It doesn't seem to check whether it should refresh on every frame, but rather when it is called to repaint. I'm working on this still.

@EJBQ
Copy link
Contributor

EJBQ commented Feb 10, 2015

So I'm hitting the issue that adding nodes and edges happens in the "Graph" class, whereas moving nodes and changing properties happens in the "GraphPanel" class. This means it's hard for me to create the Commands so that I can pass just one or the other. The issue is that calling the method in the Graph (where adding nodes and edges is) has no way to refer to its parent GraphPanel at the moment. It seems less clean to just add a reference to the parent though. And if I don't pass a reference to GraphPanel, then I can't repaint the scene after my action and it remains still until you change something else. @prmr and @JoelChev , do you think I should just add a reference to GraphPanel for now or refactor in some better way?

@prmr
Copy link
Owner Author

prmr commented Feb 10, 2015

Since there's only one Graph per GraphPanel, can't you just pass in the GraphPanel to your commands and write delegates in GraphPanel that forward necessary requests (add, remove) to the graph therein?

@prmr prmr closed this as completed Feb 10, 2015
@prmr prmr reopened this Feb 10, 2015
@EJBQ
Copy link
Contributor

EJBQ commented Feb 10, 2015

Yeah, that's probably easier than what I'm doing. I'm going to have to override the MouseAdapter constructor to pass the GraphPanel, but that's much cleaner still.

EJBQ added a commit that referenced this issue Feb 10, 2015
EJBQ added a commit that referenced this issue Feb 11, 2015
EJBQ added a commit that referenced this issue Feb 11, 2015
@EJBQ
Copy link
Contributor

EJBQ commented Feb 11, 2015

Most things work properly and cleanly now. There's a null pointer issue with properties I will fix tomorrow and children act oddly for sequence diagrams because of how edges delete children, but I have identified these issues.

@EJBQ
Copy link
Contributor

EJBQ commented Feb 21, 2015

So the issue I'm having is that the "MultiLineString.clone().equals(original) = true" property is not upheld, so that clones do not equal their original, which is a pretty major issue. So my question right now is whether to do the invasive thing that Violet 2.3 did, which is add a PropertyChangeListener to the Property Sheet and the editors therein, or to attempt to change the MultiLineString class in some way. This should not be an issue with classes that preserve the .clone() property correctly. @prmr and @JoelChev, what do you think? As it is right now I've been making sure that everything else works, but that's not really worth the time if I switch to the PropertyChangeEvent.

@EJBQ
Copy link
Contributor

EJBQ commented Feb 24, 2015

At this point I think everything is properly undoable and redoable. There is an issue with movement of nodes after placement I'm still looking at and another with moving onto another node, but everything does work at this point as far as I can tell. Obviously some testing would be nice, @prmr and @JoelChev.

EJBQ added a commit that referenced this issue Feb 24, 2015
@EJBQ EJBQ closed this as completed Feb 26, 2015
EJBQ added a commit that referenced this issue Feb 28, 2015
JoelChev added a commit that referenced this issue Feb 28, 2015
@JoelChev
Copy link
Contributor

I believe I have fixed the double pasting bug in the latest commit, but I want to make sure I haven't introduced any new bugs in the process. If this commit could be looked at, I would appreciate it!

EJBQ added a commit that referenced this issue Mar 3, 2015
@prmr prmr unassigned EJBQ Aug 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A feature request
Projects
None yet
Development

No branches or pull requests

3 participants