search

prmr / JetUML

A desktop application for fast UML diagramming.
GNU General Public License v3.0
614 stars 120 forks source link

Add undo feature #13

Closed prmr closed 9 years ago

EJBQ commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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?

EJBQ commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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.

JoelChev commented 9 years ago

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!