[PATCH] Make History dialog more diff-like

[simon04]

Currently, in the history dialog it is hard to see which nodes of a way have been added/removed/moved. One added node makes the subsequent list unreadable. See attached screenshot.

Therefore, I suggest to make the history dialog more diff-like, i.e., to emphasis added nodes in green, removed nodes in red, moved nodes in whatever color. One added node should not affect the analysis of the subsequent nodes.

Perhaps, one could make use of this library: ​http://code.google.com/p/java-diff-utils/

[stoecker]

It already should be the case. The light red is "moved" nodes, the dark red is "modified" (add/removed). But it can surely be improved.

[simon04]

Replying to stoecker:

It already should be the case. The light red is "moved" nodes

Moved in the sense that their coordinates were changed or their index within the way was changed? For the screenshot, most nodes haven't been touched at all. They are coloured orange because a node was inserted before them.

[bastiK]

There may be licensing problems with the java-diff-utils lib. In the ​file header it says Apache License 1.1. This can probably be resolved by contacting the authors.

[olejorgenb]

Replying to simon04:

Moved in the sense that their coordinates were changed or their index within the way was changed? For the screenshot, most nodes haven't been touched at all. They are coloured orange because a node was inserted before them.

I've recently looked at this code, and no, the coordinates are not considered. In fact, as far as I can see, they're not even fetched.

[bastiK]

Replying to olejorgenb:

Replying to simon04:

Moved in the sense that their coordinates were changed or their index within the way was changed? For the screenshot, most nodes haven't been touched at all. They are coloured orange because a node was inserted before them.

I've recently looked at this code, and no, the coordinates are not considered. In fact, as far as I can see, they're not even fetched.

When way nodes move, this doesn't necessarily increase the way version. The current API is not well suited to analyze these kind of modifications to a way. Basically you'd need the full history of each way node. Anyway, I think this is not the topic of this ticket.

This is more about the list of node ids and how it has changed from the previous version. E.g. when a single node was inserted in the middle of the way, the second half of the id list shouldn't be displayed as 'moved'.

[olejorgenb]

I've coded up a suggestion. The code isn't really fully functional yet, but I've attached a screenshot of the result.

Green is inserted, red deleted and yellow changed nodes. This should be about the same you get from

diff -y <(wget http://api.openstreetmap.org/api/0.6/way/88595704/1 -qO -) <(wget http://api.openstreetmap.org/api/0.6/way/88595704/7 -qO -)

Feedback wanted before I continue.

[simon04]

This looks very nice. Could you make the green and the yellow a bit less garish? :-)

[olejorgenb]

Hehe, sure, the colors can be tweaked. I just grabbed whatever predefined colors Swing had defined.

Feel free to post some RGB values :)

[simon04]

Replying to olejorgenb:

Feel free to post some RGB values :)

I have no idea. Perhaps, you could gain some inspiration from Trac's or Github's diff view … :-)

[olejorgenb]

Working patch attached. (still might need some cleanup)

I replaced the old diff method completely. Is it desirable to keep it, and make it configurable instead?

I used ​http://www.bmsi.com/java/#diff (GPL) instead of reimplementing the core algorithm.

[simon04]

Nice work! This really improves the usefulness of the history dialog.

I replaced the old diff method completely. Is it desirable to keep it, and make it configurable instead?

IMHO, the diff algorithm is superior to the current implementation. So there is no need for keeping the old implementation nor making it configurable.

A thought that might be rather difficult to implement: When viewing the history of a way, it would be useful to see which nodes have been moved in the same changeset.

[olejorgenb]

Ok, then I'll clean out all the old code and tweak the new one. Aiming to get it done during this week :)

I'll look into the changeset thing, but I'm not sure it's worth the trouble.

An annoying thing about the history dialog is that you can't change the reference version without changing the "current" version. (double-click selects the "current", then the reference, resulting in one version being both). I assume a single-click event is delivered prior to the double-click one. So to fix it you'd have to keep track of the events, and not change "current" until you're sure a double-click wont happen...

[bastiK]

Replying to olejorgenb:

An annoying thing about the history dialog is that you can't change the reference version without changing the "current" version. (double-click selects the "current", then the reference, resulting in one version being both). I assume a single-click event is delivered prior to the double-click one. So to fix it you'd have to keep track of the events, and not change "current" until you're sure a double-click wont happen...

If you're in the mood, you can improve the gui, e.g. two rows of radiobuttons like in mediawiki.

[olejorgenb]

Somehow my last comment got lost..

I think the patch above should be ok. The one with +remove-needless-defaulttablemodel.patch changes the base-class of the other models to AbstractTableModel too. (They don't seem to use any features from (the more heavyweight) DefaultTableModel)

[simon04]

I'd like to await the next stable release (see DevelopersGuide/Schedule) before committing your patch.

[olejorgenb]

yeah, I assumed that, but thanks for letting me know :)

I've found a bugish case that should be fixed too; If the order of the nodes have been changed the diff will show it as deletes/adds. A new color should be added for this case. (note to self: circular ways might need special treatment)

I don't think I've seen any diff tools handle this. It's not clear how it should be done for eg. text diffs in general, but I'd think some clever heuristics could make it useful.

[simon04]

In [4498/josm]:

see #6774 - Make History dialog more diff-like (patch by olejorgenb)

Applied attachment:ticket-6774-node-list-diff+remove-needless-defaulttablemodel.patch​

[simon04]

In [4504/josm]:

see #6774, fix #6936 - Make History dialog more diff-like (add missing file)

[bastiK]

What's the status of this ticket / feature?

[olejorgenb]

I made a new ticket for the re-ordering issue: #6994