Modify

Opened 8 years ago

Closed 8 years ago

#6774 closed enhancement (fixed)

[PATCH] Make History dialog more diff-like

Reported by: simon04 Owned by: olejorgenb
Priority: normal Milestone:
Component: Core Version:
Keywords: history, diff Cc:

Description (last modified by bastiK)

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/

Attachments (5)

hist.diff.png (112.9 KB) - added by simon04 8 years ago.
node-list-diff.png (36.7 KB) - added by olejorgenb 8 years ago.
more-diff-like-way-history-list.patch (53.1 KB) - added by olejorgenb 8 years ago.
ticket-6774-node-list-diff+remove-needless-defaulttablemodel.patch (60.3 KB) - added by olejorgenb 8 years ago.
ticket-6774-node-list-diff.patch (58.5 KB) - added by olejorgenb 8 years ago.

Download all attachments as: .zip

Change History (26)

Changed 8 years ago by simon04

Attachment: hist.diff.png added

comment:1 Changed 8 years ago by 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.

comment:2 in reply to:  1 ; Changed 8 years ago by 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.

comment:3 Changed 8 years ago by 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.

comment:4 in reply to:  2 ; Changed 8 years ago by 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.

comment:5 in reply to:  4 Changed 8 years ago by 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'.

comment:6 Changed 8 years ago by 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.

Changed 8 years ago by olejorgenb

Attachment: node-list-diff.png added

comment:7 Changed 8 years ago by simon04

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

comment:8 Changed 8 years ago by olejorgenb

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

Feel free to post some RGB values :)

comment:9 in reply to:  8 Changed 8 years ago by 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 … :-)

comment:10 Changed 8 years ago by 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.

Changed 8 years ago by olejorgenb

comment:11 Changed 8 years ago by olejorgenb

Keywords: history diff added
Owner: changed from team to olejorgenb
Status: newassigned
Summary: Make History dialog more diff-like[Initial PATCH] Make History dialog more diff-like

comment:12 in reply to:  10 Changed 8 years ago by 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.

comment:13 Changed 8 years ago by 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...

comment:14 in reply to:  13 Changed 8 years ago by 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.

Changed 8 years ago by olejorgenb

comment:15 Changed 8 years ago by olejorgenb

Summary: [Initial PATCH] Make History dialog more diff-like[PATCH] Make History dialog more diff-like

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)

comment:16 Changed 8 years ago by simon04

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

comment:17 Changed 8 years ago by 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.

comment:18 Changed 8 years ago by 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

Last edited 8 years ago by simon04 (previous) (diff)

comment:19 Changed 8 years ago by simon04

In [4504/josm]:

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

comment:20 Changed 8 years ago by bastiK

Description: modified (diff)

What's the status of this ticket / feature?

comment:21 Changed 8 years ago by olejorgenb

Resolution: fixed
Status: assignedclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain olejorgenb.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.