[josm-dev] Jumbo Patch
Gabriel Ebner
ge at gabrielebner.at
Sun Dec 16 21:14:21 GMT 2007
On Sat, Dec 15, 2007 at 11:09:02AM -0800, Dave Hansen wrote:
> I do think there's a basic problem with the ReplaceCommand. It is tied
> to an individual Java object rather than all of the particular objects
> that may have been instantiated to represent the OSM object. Just as
> you say, JOSM keeps old and new copies in memory when a way is removed
> (or modified).
Maybe the internal design of JOSM isn't quite obvious and we don't have a
documentation of it, so let me sketch the current state of things:
DataSet contains lists of nodes, ways and relations. Ways and relations
contain pointers to the very same nodes/primitives that the DataSet contains.
Generally, you'll see many places in JOSM where we test for equality of two
primitives like this: node1 == node2. Yes, two primitives that have the same
id (or will have, once uploaded) are represented by the very same object in
JOSM.
Changes are only to be made (except for MergeVisitor) by calling
Main.main.undoRedo.add with the appropriate command:
- When you add a primitive, AddCommand simply appends the primitive to the
list in DataSet.
- When you delete a primitive, DeleteCommand simply removes it from the list.
- When you change a primitive, you provide two arguments to ChangeCommand, the
old primitive and the new primitive. When added to Main.main.undoRedo,
ChangeCommand will call old.cloneFrom(new), i.e. replacing the old primitive
in-place with the new primitive, so that all reference to it will stay
valid.
This means that if you want to both add a tag to a node and add the same
node to a way (in the same SequenceCommand, i.e. before the changes to retag
the node have become visible), you mustn't add the modified node to the way,
but rather the old one:
Node n = ...;
Way w = ...;
Node nnew = new Node(n);
nnew.put("foo", "bar");
Way wnew = new Way(w);
wnew.nodes.add(n); // You'll have to use the old one here.
> Take this way:
>
> http://sr71.net/~dave/osm/josm/selfintersecting.osm
>
> and try to have the JOSM validator "fix" it. You'll see the result of
> this.
Both the current validator and the patched one cannot fix this (the fix button
is disabled).
(BTW, MotorwayLinksMustTouchMotorways.java also seems to be missing from the
patches.)
> > I'd like to keep the GUI code free of such merging logic if it's easily
> > possible. It isn't easy to read as it is now, we don't have to make worse.
>
> How about something like this:
>
> http://sr71.net/~dave/osm/josm/tigerutils.patch
>
> It's much more compact.
Yes, that's much better.
> > > Index: core/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java
> > > ===================================================================
> > > --- core/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java (revision 485)
> > > +++ core/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java (working copy)
> > > @@ -114,6 +114,12 @@
> > > public boolean incomplete = false;
> > >
> > > /**
> > > + * This is to help determine when individual objects are modified.
> > > + * When making changes, a user may increment this count.
> > > + */
> > > + public int version = 0;
> >
> > I have already said before that this is a hack, and that if you want to
> > prevent objects from being overwritten, you should fix the code that does
> > that.
>
> The real problem here is a disconnection between the times when a
> Command is created and when it is executed. How do we ever tell that a
> command is still _valid_?
Commands are only valid at the point they are inserted in the undoRedo list.
There was not meant to be a FixAllDuplicateNodesCommand that you could apply
to an arbitrary data set. Instead commands are concrete, simple and
revertable operations on the data set. (Simple is a requirement for
revertable; you want to be able to redo a command no matter what, so you'd
better make sure you restore exactly the same state the command was first
applied to; and you can only reasonably verify that if the command is simple.)
Some time ago I had an idea (see attached patch) that it might be better to
apply all the commands immediately, so you could use all the standard queries
like MapView.getNearest() and not worry about all that SequenceCommand
business. This would also solve the problem of overwriting not-yet-executed
changes -- there simply wouldn't be any.
> In the Way.replaceSubObject() code, I worked
> around this by ensuring that an attempt to delete an already deleted
> node is considered success. Should we have a way for commands to
> self-validate before performing their actions?
Until now, there was no way a command could fail. They simply didn't make
assumptions on the contents of the primitives.
That's also the reason most of the modification logic is in the actions, and
the commands just provide some sort of transaction layer in order to support
undo. Actions have traditionally been making the decisions (and verified
whether they made sense), and the commands carried them out. This makes sense
from a UI POV, you want to complain immediately to the user when he's been
doing something that does not make sense, and not when the command is already
in the undo list.
I think the ReplaceSubObjectCommand might be a bit too general. 1) It doesn't
support multiple uses of the same node in a way (think roundabouts). You
can't even add a node twice (you explicitly prohibited this). 2) You can't
add nodes other than at the end of the way. 3) It isn't clear how to extend
it to support relations (how to change/specify roles?).
Wouldn't it make more sense to have an AddNodeToWayCommand, a
RemoveNodeFromWayCommand, and a ReplaceNodeInWayCommand, taking indices
instead of nodes? This would get us rid of the highly complex Way.replace
logic, and we'd be able to properly support duplicate nodes, while making it
possible to better guard against erroneous requests.
Gabriel.
-------------- next part --------------
Index: src/org/openstreetmap/josm/data/UndoRedoHandler.java
===================================================================
--- src/org/openstreetmap/josm/data/UndoRedoHandler.java (revision 492)
+++ src/org/openstreetmap/josm/data/UndoRedoHandler.java (working copy)
@@ -4,9 +4,12 @@
import java.util.Iterator;
import java.util.LinkedList;
import java.util.Stack;
+import java.util.ArrayList;
+import java.util.Collections;
import org.openstreetmap.josm.Main;
import org.openstreetmap.josm.command.Command;
+import org.openstreetmap.josm.command.SequenceCommand;
import org.openstreetmap.josm.gui.layer.Layer;
import org.openstreetmap.josm.gui.layer.OsmDataLayer;
import org.openstreetmap.josm.gui.layer.Layer.LayerChangeListener;
@@ -23,6 +26,8 @@
*/
private final Stack<Command> redoCommands = new Stack<Command>();
+ private final Stack<LinkedList<Command>> sequences = new Stack<LinkedList<Command>>();
+
public final LinkedList<CommandQueueListener> listenerCommands = new LinkedList<CommandQueueListener>();
@@ -30,26 +35,55 @@
Layer.listeners.add(this);
}
+ public void begin() {
+ sequences.push(new LinkedList<Command>());
+ }
+ public void rollback() {
+ LinkedList<Command> cmds = sequences.pop();
+
+ ArrayList<Command> cmdsArr = new ArrayList<Command>(cmds);
+ Collections.reverse(cmdsArr);
+ for (Command c : cmdsArr) {
+ c.undoCommand();
+ }
+ }
+
+ public void commit(String name) {
+ justRecord(new SequenceCommand(name, sequences.pop()));
+ }
+
/**
* Execute the command and add it to the intern command queue.
*/
public void add(final Command c) {
c.executeCommand();
- commands.add(c);
- redoCommands.clear();
- if (Main.map != null && Main.map.mapView.getActiveLayer() instanceof OsmDataLayer) {
- OsmDataLayer data = (OsmDataLayer)Main.map.mapView.getActiveLayer();
- data.setModified(true);
- data.fireDataChange();
+
+ justRecord(c);
+ }
+
+ private void justRecord(Command c) {
+ if (!sequences.isEmpty()) {
+ sequences.peek().add(c);
+ } else {
+ commands.add(c);
+ redoCommands.clear();
+ if (Main.map != null && Main.map.mapView.getActiveLayer() instanceof OsmDataLayer) {
+ OsmDataLayer data = (OsmDataLayer)Main.map.mapView.getActiveLayer();
+ data.setModified(true);
+ data.fireDataChange();
+ }
+ fireCommandsChanged();
}
- fireCommandsChanged();
}
/**
* Undoes the last added command.
*/
public void undo() {
+ if (!sequences.isEmpty()) {
+ throw new RuntimeException("Command sequences stack is not empty");
+ }
if (commands.isEmpty())
return;
final Command c = commands.removeLast();
@@ -69,6 +103,9 @@
* TODO: This has to be moved to a central place in order to support multiple layers.
*/
public void redo() {
+ if (!sequences.isEmpty()) {
+ throw new RuntimeException("Command sequences stack is not empty");
+ }
if (redoCommands.isEmpty())
return;
final Command c = redoCommands.pop();
@@ -88,12 +125,18 @@
}
public void clean() {
+ if (!sequences.isEmpty()) {
+ throw new RuntimeException("Command sequences stack is not empty");
+ }
redoCommands.clear();
commands.clear();
fireCommandsChanged();
}
public void layerRemoved(Layer oldLayer) {
+ if (!sequences.isEmpty()) {
+ throw new RuntimeException("Command sequences stack is not empty");
+ }
boolean changed = false;
for (Iterator<Command> it = commands.iterator(); it.hasNext();) {
if (it.next().invalidBecauselayerRemoved(oldLayer)) {
More information about the josm-dev
mailing list