Modify

Opened 3 months ago

Closed 2 months ago

#18929 closed defect (fixed)

[Patch] reverter plugin

Reported by: geow Owned by: Upliner
Priority: normal Milestone:
Component: Plugin reverter Version:
Keywords: template_report Cc: simon04

Description (last modified by skyper)

Edits on nodes won't be reverted, see osmwww:changeset/82149018 and osmwww:changeset/82149070

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-02-26 10:50:27 +0100 (Wed, 26 Feb 2020)
Build-Date:2020-02-26 09:52:41
Revision:15937
Relative:URL: ^/trunk

Identification: JOSM/1.5 (15937 en) Mac OS X 10.13.6
OS Build number: Mac OS X 10.13.6 (17G11023)
Memory Usage: 1221 MB / 2731 MB (961 MB allocated, but free)
Java version: 1.8.0_241-b07, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: Display 69678912 1920x1080
Maximum Screen Size: 1920x1080
Dataset consistency test: No problems found

Plugins:
+ DirectUpload (35248)
+ FastDraw (35360)
+ ImproveWay (29)
+ InfoMode (35248)
+ apache-commons (35362)
+ buildings_tools (35248)
+ ejml (35122)
+ geotools (35169)
+ jaxb (35092)
+ jts (35122)
+ measurement (35248)
+ opendata (35330)
+ reverter (35313)
+ terracer (35327)
+ utilsplugin2 (35334)

Tagging presets:
+ https://josm.openstreetmap.de/josmfile?page=Presets/OneClick&zip=1
+ https://raw.githubusercontent.com/yopaseopor/traffic_signs_preset_JOSM/master/DE.zip
+ https://josm.openstreetmap.de/josmfile?page=Presets/Leaftype&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Presets/Mountainbike&zip=1

Last errors/warnings:
- E: Failed to locate image 'traffic_signs_presets/ES_tertiary.png'
- W:  Tertiary: Could not get presets icon traffic_signs_presets/ES_tertiary.png
- W: No revert commands found for changeset 82149018
- W: No revert commands found for changeset 82149018
- W: No revert commands found for changeset 82149018

Attachments (2)

18929.patch (884 bytes) - added by GerdP 3 months ago.
18929.2.patch (2.2 KB) - added by GerdP 3 months ago.
ignore discardable tags only

Download all attachments as: .zip

Change History (24)

comment:1 Changed 3 months ago by skyper

Description: modified (diff)

proper interlinks no redirects

comment:2 Changed 3 months ago by GerdP

Hmm, this is quite special. Both changesets removed tags with prefix "gpx:" and those are consideted "uninteresting" like e.g. created_by or source. These tags are ignored when two object versions are compared, thus nothing is reverted.
If I got that right the error is in JOSM core.
The javadoc for method OsmPrimitive.hasEqualSemanticAttributes(OsmPrimitive other) claims that it returns true if "both have the same tags" but it calls a method which ignores the "uninteresting" tags.

A possible solution for reverter would be to make method OsmPrimitive.hasEqualSemanticAttributes(OsmPrimitive other, boolean testInterestingTagsOnly) public and call this in reverter.

comment:3 Changed 3 months ago by GerdP

Cc: simon04 added

Changed 3 months ago by GerdP

Attachment: 18929.patch added

comment:4 Changed 3 months ago by GerdP

Summary: reverter plugin[Patch] reverter plugin

Attached patch changes reverter to compare all tags. It doesn't require a change in core.
Drawback: This would also revert changes where so-called discardable tags were silently removed.
I just noticed that "created_by" is one of them, the full list is in AbstractPrimitive.

Changed 3 months ago by GerdP

Attachment: 18929.2.patch added

ignore discardable tags only

comment:5 Changed 3 months ago by Don-vip

You can simply call AbstractPrimitive.getInterestingTags()

comment:6 in reply to:  5 ; Changed 3 months ago by GerdP

Replying to Don-vip:

You can simply call AbstractPrimitive.getInterestingTags()

I think that is what is happening now.

comment:7 in reply to:  6 Changed 3 months ago by Don-vip

Replying to GerdP:

Replying to Don-vip:

You can simply call AbstractPrimitive.getInterestingTags()

I think that is what is happening now.

? I don't understand. You don't need to write all this code:

private static Map<String, String> getNonDiscardableTags(OsmPrimitive p) {
    Map<String, String> result = new HashMap<>();
    for (Map.Entry<String, String> e : p.getKeys().entrySet()) {
    if (!AbstractPrimitive.getDiscardableKeys().contains(e.getKey()))
        result.put(e.getKey(), e.getValue());
    }
    return result;
}

Simply call:

p.getInterestingTags()

comment:8 Changed 3 months ago by GerdP

The result would not be the same. Look at the implementation of isUninterestingKey which is called in getInterestingTags

comment:9 Changed 3 months ago by skyper

So make the call of isUninterestingKey optional.

comment:10 Changed 3 months ago by GerdP

I see no need for an optiion. In addition to the discardable tags the following tag keys are considered "uninteresting":

"source", "source_ref", "source:", "comment","watch", "watch:", "description", "attribution", "note", "fixme", "FIXME"

In addition all tag keys starting with "gpx:" and all tags listed in preference tags.workinprogress
In my eyes those additional tags should not be ignored when a cs is reverted.

comment:11 Changed 2 months ago by GerdP

@Vincent: Do you see a better solution?

comment:12 Changed 2 months ago by Don-vip

@Gerd: I don't. Go ahead :)

comment:13 in reply to:  12 Changed 2 months ago by skyper

Replying to Don-vip:

@Gerd: I don't. Go ahead :)

stoecker on #18673 comment 8:

JOSM does not always follow "global decision and the discussions". We simply do something (like replacing 3 variants of boolean with the yes/no standard we have now).

this should not be reverted.

comment:14 Changed 2 months ago by GerdP

@skyper: What do you mean?

comment:15 Changed 2 months ago by skyper

e.g. reverter should not revert if true/false was replaced with yes/no. I did not look into code but I am not sure if these value changes are ignored.

comment:16 Changed 2 months ago by GerdP

I think a revert should revert everything The discardable tags are already an exception which I find problematic. If want to allow that the removement of gpx:ele can be reverted why not also revert changes between boolean values?

Version 0, edited 2 months ago by GerdP (next)

comment:17 in reply to:  16 Changed 2 months ago by skyper

Ok, if you gonna use JOSM to upload, the discardable tags will be removed again, so you are right. Better revert everything or at least let the user decide with options.

comment:18 in reply to:  15 Changed 2 months ago by stoecker

e.g. reverter should not revert if true/false was replaced with yes/no. I did not look into code but I am not sure if these value changes are ignored.

I don't think that is important, as true/false is mainly gone from the database for boolean values.

I think a revert should revert everything. The discardable tags are already an exception which I find problematic. If we want to allow that the removement of gpx:ele can be reverted why not also revert changes between boolean values?

Because JOSM silently will re-revert them when you do anything with the tags :-) A revert thus means you'll upload a NULL-change to the server. For boolean-value it probably needs any action with the tags for the change to trigger.

Last edited 2 months ago by stoecker (previous) (diff)

comment:19 Changed 2 months ago by GerdP

@Dirk: So you also agree that 18929.2.patch is okay?

comment:20 in reply to:  19 Changed 2 months ago by stoecker

Replying to GerdP:

@Dirk: So you also agree that 18929.2.patch is okay?

Probably. Sounds sane to me, but I didn't test it, so I cannot say if it IS also sane. But we'll see when applied :-)

comment:21 in reply to:  16 Changed 2 months ago by Don-vip

Replying to GerdP:

I think a revert should revert everything.

The reverter plugin is incredibly complex. Reverting everything will allow to simplify it, so I agree with you.

comment:22 Changed 2 months ago by GerdP

Resolution: fixed
Status: newclosed

see [o35381:35383] (18929.2.patch)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Upliner.
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.