[PATCH] Changeset tag `created_by` added to all uploads without informing the user

[skyper]

Do not know if this is a problem in the reverter plugin and/or core.

What steps will reproduce the problem?

1. Have a layer A with modified data
2. Revert a a changeset (layer B)
3. Switch back to layer A
4. Upload layer A

What is the expected result?

No additional cs tag or tag value is added

What happens instead?

reverter plugin is added to cs's created_by values.

Please provide any additional information below. Attach a screenshot if possible.

The reverter plugin should only be added as created_by value if at least one object of a revert is uploaded.

Additionally, the user should always be informed about the automatically added value. Maybe a change in background color of the tab title can work as indicator.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-11-03 00:09:37 +0100 (Tue, 03 Nov 2020)
Revision:17292
Build-Date:2020-11-03 02:30:51
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (17292 en) Linux Debian GNU/Linux 10 (buster)
Memory Usage: 301 MB / 768 MB (88 MB allocated, but free)
Java version: 11.0.9+11-post-Debian-1deb10u1, Debian, OpenJDK 64-Bit Server VM
Plugins:
+ reverter (35579)

[taylor.smock]

Looking at ReverterUploadHook​, it looks like it was first implemented in 2010. We should probably update the hook for r13028 (2017).

[taylor.smock]

Call UploadHook#modifyChangeSetTags prior to showing the user the upload dialog _for non-late upload hooks_.

[taylor.smock]

Add coloring to the Settings tab if the changeset tags have been programatically modified

[taylor.smock]

Use modifyChangesetTags instead of checkUpload in ReverterUploadHook

[taylor.smock]

Use a listener for tabs instead of a listener on the table model, as the source/comment fields trigger updates for the table

[taylor.smock]

I'll go ahead and leave the core patch up for a week or two. It fixes the second part of the problem ("Additionally, the user should always be informed about the automatically added value. Maybe a change in background color of the tab title can work as indicator.")

Specifically, I'm not particularly happy with the background color I picked, but it (roughly) matches the color used for UI feedback for comments/sources (I was going to use the same colour, but it wasn't noticeable).

EDIT: #22080 is a related ticket

[skyper]

Instead of background color an icon might be used as indication if that is easier to accomplish.

[taylor.smock]

Replying to skyper:

Instead of background color an icon might be used as indication if that is easier to accomplish.

Got an icon? If so, I'll take it. Otherwise, it is easy enough to change the colour to something that looks better/makes more sense. Right now, I'm using an ​orangish color.

src/org/openstreetmap/josm/gui/io/UploadDialog.java

@@ -64 +68 @@
  * @since 2025
  */
 public class UploadDialog extends AbstractUploadDialog implements PreferenceChangedListener, PropertyChangeListener {
+    /** A warning color to indicate something is non-default in the changeset tags */
+    private static final Color WARNING_BACKGROUND = new NamedColorProperty(
+            marktr("Changesets: Non-default advanced settings"), new Color(0xF89042)).get();
     /** the unique instance of the upload dialog */
     private static UploadDialog uploadDialog;
 

[skyper]

Thought about .

[taylor.smock]

I'm not certain I like the way the modal behaves with the icon. We're probably going to want another icon (maybe ) since otherwise the UI will shift when the user clicks on "Settings".

[taylor.smock]

Add icon hints when changeset tags have been changed by upload hooks

[taylor.smock]

@stoecker: Are we doing a release this weekend or the weekend after? If the latter, I'll merge this patch tomorrow, otherwise I'll wait a week after the next release.

[stoecker]

Replying to taylor.smock:

@stoecker: Are we doing a release this weekend or the weekend after? If the latter, I'll merge this patch tomorrow, otherwise I'll wait a week after the next release.

There were enough changes, so I'd say this weekend.

[taylor.smock]

Replying to stoecker:

There were enough changes, so I'd say this weekend.

OK. Good to know. I'll move the milestone back a month. And I'll try to remember to apply the patch Thursday or Friday next week.

[taylor.smock]

In 35972/osm:

ReverterUploadHook: Use modifyChangesetTags instead of checkUpload

This also fixes an issue where any reverter change on any OSM layer
would cause the changeset created_by tag to be changed, even if
the reverter plugin was not used on that OSM layer.

See #20025 for more information.

[taylor.smock]

In 35973/osm:

reverter (dist)

ReverterUploadHook: Use modifyChangesetTags instead of checkUpload

This also fixes an issue where any reverter change on any OSM layer
would cause the changeset created_by tag to be changed, even if
the reverter plugin was not used on that OSM layer.

See #20025 for more information.

[skyper]

@taylor.smock
Sorry, I still have not much time.
One more question: What happens in case of merging a layer or an object from a layer which has been reverted?

[taylor.smock]

Replying to skyper:

@taylor.smock
Sorry, I still have not much time.
One more question: What happens in case of merging a layer or an object from a layer which has been reverted?

For the reverter plugin, nothing. It will be unable to detect that. I want to say I disabled the merge layer functionality in the MapWithAI plugin to not have to worry about that. That, and avoiding users from easily importing large areas without human review.

Part of that is due to the merge layer action not being undoable. I had to do a lot of work to get equivalent functionality done in the MapWithAI plugin so that people could undo/redo.

If we could detect the merging of a layer or an object from a layer that had been reverted, then it would be doable to add logic in the reverter plugin to cover that eventuality (pseudocode: if (command instanceof MergeDataCommand mergeCommand && revertedDataSets.contains(mergeCommand.getOriginalDataSet()))

In order to support that properly, we would have to add a new command type (DualDataCommand or something like that) which does operations on two separate datasets. In this case, copy from one dataset and paste into the other.

[taylor.smock]

In 18467/josm:

Fix #20025, #22080: Notify users when changeset tags are programmatically modified

This only works for non-late upload hooks.

[pch14@…]

Perhaps re-open? Reverter now does not show as creator/user-agent at all.

Start JOSM, do not load any data, select revert changeset from data menu, paste url, let it work, hit upload, inspect properties: created_by by only shows JOSM, no mention of reverter being used.

[skyper]

Replying to pch14@…:

Perhaps re-open? Reverter now does not show as creator/user-agent at all.

Better create a new ticket mentioning this one in the description.

Start JOSM, do not load any data, select revert changeset from data menu, paste url, let it work, hit upload, inspect properties: created_by by only shows JOSM, no mention of reverter being used.

I cannot reproduce.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2022-07-04 08:25:14 +0200 (Mon, 04 Jul 2022)
Revision:18507
Build-Date:2022-07-05 01:30:56
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (18507 en) Linux Debian GNU/Linux 11 (bullseye)
Java version: 17.0.3+7-Debian-1deb11u1, Debian, OpenJDK 64-Bit Server VM

VM arguments: [--module-path=/usr/share/openjfx/lib, --add-modules=java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web, -Djosm.restart=true, -Djosm.dir.name=JOSM-latest, -Djava.net.useSystemProxies=true, --add-exports=java.base/sun.security.action=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.plugins.jpeg=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.spi=ALL-UNNAMED]
Dataset consistency test: No problems found

Plugins:
+ reverter (35980)
+ utilsplugin2 (35970)

[taylor.smock]

Replying to pch14@…:

Perhaps re-open? Reverter now does not show as creator/user-agent at all.

Start JOSM, do not load any data, select revert changeset from data menu, paste url, let it work, hit upload, inspect properties: created_by by only shows JOSM, no mention of reverter being used.

Are you running JOSM r18467 or later? If not, the reverter plugin will not show up in the UI, as the hook that changes the created_by field in the UI is called after you hit upload. With r18467 and later, non-late upload hooks are called prior to you seeing the UI, and so you will see the correct created_by tag at that point.

The last stable was r18463, so I'm going to presume that you are running r18463 instead of the current latest version (r18507).

I've been avoiding doing the release process -- I'd prefer if stoecker/Don-vip did that. With that said, if we do not have a stable release at the end of July, and no recent activity from Don-vip (r18490) or stoecker (r18492), I'll probably make a release at the beginning of August (August 6th probably), assuming there were no code-changing commits in the previous 3-4 days.

[stoecker]

Replying to taylor.smock:

I've been avoiding doing the release process -- I'd prefer if stoecker/Don-vip did that.

Ah. Another month gone. Sorry, didn't really notice. To much other things to do. No need to avoid that. There is a reason it's not restricted to admins.

[taylor.smock]

Replying to stoecker:

Ah. Another month gone. Sorry, didn't really notice. To much other things to do. No need to avoid that. There is a reason it's not restricted to admins.

I know it isn't restricted to admins -- I've just been trying to avoid stepping on anyone's toes accidentally.

I'll go ahead and go through the release process Wednesday (I want to give people running josm-latest a chance to update to r18506, although I strongly doubt it introduced any new bugs).

[pch14@…]

Replying to taylor.smock:

The last stable was r18463, so I'm going to presume that you are running r18463 instead of the current latest version (r18507).

Yes, this is with the stable release 18463. later I did upload, and no reverter showed in the public ledger neither; Not sure though, if I did upload the layer where the reverter got used, but it is very likely so. Reverting in any case deserves a mention.

[taylor.smock]

Replying to pch14@…:

Yes, this is with the stable release 18463. later I did upload, and no reverter showed in the public ledger neither; Not sure though, if I did upload the layer where the reverter got used, but it is very likely so. Reverting in any case deserves a mention.

Do you have a changeset where you definitely reverted something and it didn't show up in the changeset tags?

[pch14@…]

I don't revert that often, but in this ​https://nrenner.github.io/achavi/?changeset=123165198 I noticed the missing attribution in the upload properties pane. The CS fully reverts 122879851 and then adds a single node and a single note to an existing node close by.

[ I had the CS open for quite a while before committing, in order to research the source of their "local_knowledge" - actually they misread a map of our alpine club, thinking the name of the locality was the name of the memorial there. They also managed to put one node exactly on top of another, a bit up the path. I thankfully let the reverter handle the fake POI there. I would not have deleted the superficial node in the straight line, if this did not start from the reverter. So yes, this is definitely from the plug-in. ]

[taylor.smock]

I wasn't able to reproduce on the dev api:

• ​https://master.apis.dev.openstreetmap.org/changeset/239687 (the changeset I was reverting)
• ​https://master.apis.dev.openstreetmap.org/changeset/239688 (the changeset that did the reverting, full revert)
• ​https://master.apis.dev.openstreetmap.org/changeset/239689 (revert 239688 so I can do a partial test revert)
• ​https://master.apis.dev.openstreetmap.org/changeset/239690 (test upload to make certain that the created_by tag from the reverter plugin wasn't sticky)
• ​https://master.apis.dev.openstreetmap.org/changeset/239691 (partially revert 239690)
• ​https://master.apis.dev.openstreetmap.org/changeset/239692 (revert 239691)
• ​https://master.apis.dev.openstreetmap.org/changeset/239693 (revert + add data)

EDIT: FTR, it looks like I need to change the reverter plugin to merge its string with the JOSM string.

EDIT2: reverter created_by overriding JOSM created_by was fixed in r35999/osm and dist was r36000/osm.

[anonymous]

Today my ubuntu system received an update of JOSM, I tried to revert my revert and got upload warnings, of course ;) The reverter plug-in now is mentioned in the created_by property and a warning sign appears in the upload dialogue.

Curiously, the deb package seems to think, bash was in /usr/bin? I get JOSM from https://josm.openstreetmap.de/apt As has been custom for ages, the Bash shell here lives in /bin/ …

[taylor.smock]

Replying to anonymous:

Today my ubuntu system received an update of JOSM, I tried to revert my revert and got upload warnings, of course ;) The reverter plug-in now is mentioned in the created_by property and a warning sign appears in the upload dialogue.

Curiously, the deb package seems to think, bash was in /usr/bin? I get JOSM from https://josm.openstreetmap.de/apt As has been custom for ages, the Bash shell here lives in /bin/ …

My fault, see #22193. It looks like no one without usrmerge installed or installed buster or later is running josm-latest. Should be fixed in r18513. While will hopefully be released today/tomorrow (I'm waiting on the automated release system to do the release.)

[skyper]

See #22291 for a follow-up.