[josm-dev] Jumbo Patch
Gabriel Ebner
ge at gabrielebner.at
Sat Dec 15 09:13:29 GMT 2007
(Commenting on the core changes.)
On Fri, Dec 14, 2007 at 04:45:56PM -0800, Dave Hansen wrote:
> OK, this is in the open source spirit of "release early release often".
> I've been hacking on JOSM for the purpose of making the TIGER merging
> easier. Frederik, this implements some of the "reverse lookup" stuff
> that we were talking about a few weeks ago. It seems to work OK,
> although it's not heavily tested.
We were somehow planning to remove all optimization stuff from OsmPrimitive in
the mid^Wlong term, so I don't know if this is a step in the right direction,
especially as it adds introduces quite a lot of changes in unrelated code.
Additionally it seems to me that, once created, a way is never removed from
the cache. When you split a way[1], JOSM keeps both the old one and new ones in
memory so it can undo the change. When you upload it then, that cache is
simply cleared; we just call commands.clear(). Currently, the old way is then
no longer referenced and can be freed by the GC. However we'd still keep a
reference in Node.wayList then, effectively creating a leak. (Same goes for
temporary ways; e.g. in CombineWayAction we create a way before we ask the
user to merge the tags. If the user says no, we just return and don't clean
up the way.)
[1] I just saw that you modified SplitWayAction not to do this, still, the
general point holds true.
Couldn't we just keep a hash table in DataSet (from nodes to lists of ways)
and update that in *Command?
(Incidentally, Node.waysUsing() is only used once, and that's the only way to
access this cache.)
> @@ -216,6 +213,8 @@
> JOptionPane.showMessageDialog(Main.parent, tr("The way cannot be split at the selected nodes. (Hint: Select nodes in the middle of the way.)"));
> return;
> }
> + System.out.println("wayChunks.size(): " + wayChunks.size());
> + System.out.println("way id: " + selectedWay.id);
>
> // build a list of commands, and also a new selection list
> Collection<Command> commandList = new ArrayList<Command>(wayChunks.size());
Idea: How about adding Main.debug() and a --debug flag, we wouldn't need to
remove all the debugging code and add new ad-hoc output every time we want
verbose output.
> @@ -223,12 +222,16 @@
>
> Iterator<List<Node>> chunkIt = wayChunks.iterator();
>
> - // First, change the original way
> - Way changedWay = new Way(selectedWay);
> - changedWay.nodes.clear();
> - changedWay.nodes.addAll(chunkIt.next());
> - commandList.add(new ChangeCommand(selectedWay, changedWay));
> newSelection.add(selectedWay);
> + // First, pull the first chunk off, it stays in
> + // the "original" way.
> + List<Node> firstChunk = chunkIt.next();
> + for (Node n : selectedWay.allNodes()) {
> + if (firstChunk.contains(n))
> + continue;
> + System.out.println("removing node: " + n + " from way: " + selectedWay.id);
> + commandList.add(new ReplaceSubObjectCommand(selectedWay, n, null));
> + }
>
> // Second, create new ways
> while (chunkIt.hasNext()) {
This might turn out to create tons of ReplaceSubObjectCommands. Say the user
wants to split a two-node way off a way with 5k nodes, then there's a 50
percent probability that we add 4999 ReplaceSubObjectCommands.
BTW, there doesn't seem to be a ReplaceSubObjectCommand.java in the patch.
Depending on the implementation it might be a bad idea to actually implement
spltting ways that way, as we might want to split a way that contains a node
multiple times, and I see no way to tell ReplaceSubObjectCommand what node to
replace.
> Index: core/src/org/openstreetmap/josm/actions/search/SearchCompiler.java
> ===================================================================
> --- core/src/org/openstreetmap/josm/actions/search/SearchCompiler.java (revision 485)
> +++ core/src/org/openstreetmap/josm/actions/search/SearchCompiler.java (working copy)
> @@ -1,4 +1,3 @@
> -// License: GPL. Copyright 2007 by Immanuel Scholz and others
> package org.openstreetmap.josm.actions.search;
>
> import java.io.IOException;
Yes, I know these copyright notices aren't the best (heck, it doesn't even say
what version of the GPL applies, or whether we have an "or any later version"
clause), but that belongs IMHO to another patch.
> Index: core/src/org/openstreetmap/josm/actions/MergeNodesAction.java
> ===================================================================
> --- core/src/org/openstreetmap/josm/actions/MergeNodesAction.java (revision 485)
> +++ core/src/org/openstreetmap/josm/actions/MergeNodesAction.java (working copy)
> @@ -169,7 +169,37 @@
> Map<String, JComboBox> components = new HashMap<String, JComboBox>();
> JPanel p = new JPanel(new GridBagLayout());
> for (Entry<String, Set<String>> e : props.entrySet()) {
> - if (e.getValue().size() > 1) {
> + if (e.getKey().equals("tiger:tlid")) {
> + TreeMap tlids = new TreeMap();
> + for (String tlid_list: e.getValue()) {
> + for (String tlid_str: tlid_list.split(":")) {
> + Integer tlid = new Integer(tlid_str);
> + tlids.put(tlid, 1);
> + }
> + }
> + String combined = "";
> + for (Object _tlid : tlids.keySet()) {
> + Integer tlid = (Integer)_tlid;
> + if (combined.length() > 0)
> + combined += ":";
> + combined += tlid;
> + }
> + newNode.put(e.getKey(), combined);
> + } else if (e.getKey().indexOf("tiger:") != -1) {
> + TreeMap attrs = new TreeMap();
> + for (String attr_list: e.getValue()) {
> + for (String attr_str: attr_list.split(":")) {
> + attrs.put(attr_str, 1);
> + }
> + }
> + String combined = "";
> + for (Object attr : attrs.keySet()) {
> + if (combined.length() > 0)
> + combined += ":";
> + combined += attr;
> + }
> + newNode.put(e.getKey(), combined);
> + } else if (e.getValue().size() > 1) {
> JComboBox c = new JComboBox(e.getValue().toArray());
> c.setEditable(true);
> p.add(new JLabel(e.getKey()), GBC.std());
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.
Maybe you could modify the tags before the loop, perhaps like this:
// Merge the Tiger ids
if (props.containsKey("tiger:tlid")) {
...
for (String tlids : props.get("tiger:tlid")) {
...
}
...
props.set("tiger:tlid", new TreeSet<String>(combined));
}
> @@ -56,6 +58,11 @@
> * Send the dataset to the server. Ask the user first and does nothing if he
> * does not want to send the data.
> */
> + static int MSECS_PER_SECOND = 1000;
> + static int SECONDS_PER_MINUTE = 60;
> + static int MINUTES_PER_HOUR = 60;
> + static int HOURS_PER_DAY = 24;
> + static int MSECS_PER_MINUTE = MSECS_PER_SECOND * SECONDS_PER_MINUTE;
> public void uploadOsm(Collection<OsmPrimitive> list) throws SAXException {
> processed = new LinkedList<OsmPrimitive>();
> initAuthentication();
You should at least make these constants final, private would be even better.
> @@ -65,13 +72,31 @@
>
> NameVisitor v = new NameVisitor();
> try {
> + long start = System.currentTimeMillis();
> + Time startTime = new Time(start);
> for (OsmPrimitive osm : list) {
> if (cancel)
> return;
> + long now = System.currentTimeMillis();
> + long elapsed = now - start;
> + if (elapsed == 0)
> + elapsed = 1;
> + int progress = Main.pleaseWaitDlg.progress.getValue();
> + float uploads_per_ms = (float)progress / elapsed;
> + float uploads_left = list.size() - progress;
> + int ms_left = (int)(uploads_left / uploads_per_ms);
> + int minutes_left = ms_left / MSECS_PER_MINUTE;
> + int seconds_left = (ms_left / MSECS_PER_SECOND) % SECONDS_PER_MINUTE ;
> + String time_left_str = Integer.toString(minutes_left) + ":";
> + if (seconds_left < 10)
> + time_left_str += "0";
> + time_left_str += Integer.toString(seconds_left);
> osm.visit(v);
> - Main.pleaseWaitDlg.currentAction.setText(tr("Upload {0} {1} ({2})...", tr(v.className), v.name, osm.id));
> + Main.pleaseWaitDlg.currentAction.setText(tr("Upload {0} {1} (id: {2}) {3}% {4}/{5} ({6} left)...",
> + tr(v.className), v.name, osm.id, 100.0*progress/list.size(), progress, list.size(), time_left_str));
> osm.visit(this);
> Main.pleaseWaitDlg.progress.setValue(Main.pleaseWaitDlg.progress.getValue()+1);
> + Main.pleaseWaitDlg.progress.setValue(progress+1);
> }
> } catch (RuntimeException e) {
> e.printStackTrace();
IMHO we have too much run-together code as-is, maybe it would be better to put
that into a formatDuration function or something.
> + public HashSet<Pair<Node,Node>> getNodePairs(boolean sort) {
> + HashSet<Pair<Node,Node>> chunkSet = new HashSet<Pair<Node,Node>>();
> + Node lastN = null;
> + for (Node n : this.nodes) {
> + if (lastN == null) {
> + lastN = n;
> + continue;
> + }
> + Pair<Node,Node> np = new Pair<Node,Node>(lastN, n);
> + if (sort) {
> + Pair.sort(np);
> + }
> + chunkSet.add(np);
> + lastN = n;
> + }
> + return chunkSet;
> + }
I have used this lastN hack quite a few times to iterate over way segments,
maybe it would be useful to keep this, perhaps change it to use a list though,
otherwise the segments get all shuffled.
> 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.
> @@ -161,6 +167,13 @@
> return id == ((OsmPrimitive)obj).id;
> }
>
> + public Object replace(OsmPrimitive x, OsmPrimitive y) {
> + return this.replace(x, y, null);
> + }
> + public Object replace(OsmPrimitive x, OsmPrimitive y, Object cookie) {
> + System.out.println("Called OsmPrimitive::replace() on: " + this.getClass().getName());
> + return null;
> + }
OsmPrimitive.replace isn't used anywhere in the patch.
> @@ -254,6 +270,9 @@
> (keys == null ? osm.keys==null : keys.equals(osm.keys));
> }
>
> + public boolean mayDelete() {
> + return false;
> + }
OsmPrimitive.mayDelete isn't used anywhere else either.
Gabriel.
More information about the josm-dev
mailing list