Modify

Opened 13 years ago

Closed 13 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 13 years ago.
node-list-diff.png (36.7 KB ) - added by olejorgenb 13 years ago.
more-diff-like-way-history-list.patch (53.1 KB ) - added by olejorgenb 13 years ago.
ticket-6774-node-list-diff+remove-needless-defaulttablemodel.patch (60.3 KB ) - added by olejorgenb 13 years ago.
ticket-6774-node-list-diff.patch (58.5 KB ) - added by olejorgenb 13 years ago.

Download all attachments as: .zip

Change History (26)

by simon04, 13 years ago

Attachment: hist.diff.png added

comment:1 by stoecker, 13 years ago

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.

in reply to:  1 ; comment:2 by simon04, 13 years ago

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 by bastiK, 13 years ago

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.

in reply to:  2 ; comment:4 by olejorgenb, 13 years ago

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.

in reply to:  4 comment:5 by bastiK, 13 years ago

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 by olejorgenb, 13 years ago

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.

by olejorgenb, 13 years ago

Attachment: node-list-diff.png added

comment:7 by simon04, 13 years ago

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

comment:8 by olejorgenb, 13 years ago

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

Feel free to post some RGB values :)

in reply to:  8 comment:9 by simon04, 13 years ago

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 by olejorgenb, 13 years ago

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.

by olejorgenb, 13 years ago

comment:11 by olejorgenb, 13 years ago

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

in reply to:  10 comment:12 by simon04, 13 years ago

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 by olejorgenb, 13 years ago

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...

in reply to:  13 comment:14 by bastiK, 13 years ago

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.

by olejorgenb, 13 years ago

comment:15 by olejorgenb, 13 years ago

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 by simon04, 13 years ago

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

comment:17 by olejorgenb, 13 years ago

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 by simon04, 13 years ago

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 13 years ago by simon04 (previous) (diff)

comment:19 by simon04, 13 years ago

In [4504/josm]:

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

comment:20 by bastiK, 13 years ago

Description: modified (diff)

What's the status of this ticket / feature?

comment:21 by olejorgenb, 13 years ago

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. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.