Replace geometry update

[Filip009]

After update 19017 in replace geometry, you need to choose also tags without a conflict.

Sometimes it is useful, but sometimes you want to use same tags for new way.

From discord conversation:

@Taylor Smock
I think that was a deliberate decision. IIRC, someone didn't want to merge more specific information when merging two ways. Example:
highway=residential + lanes=2 when merged with highway=residential should not have the outcome of highway=residential + lanes=2.

@AntiComposite
it is very annoying without a "accept all" button
I don't mind the prompt, I do mind the clicking

@Filip
From this perspective it was needed, but now from "accept all" button is needed (as mentioned by AntiComposite) or something like conflict menu choosing, where you can choose at one click tagging from one side or second side.

[Filip009]

conflicts

[GerdP]

See #23305.
You can restore the old behaviour with the preference combine-conflict-precise=false

[Filip009]

Nice, thank you very much :)

Replying to GerdP:

See #23305.
You can restore the old behaviour with the preference combine-conflict-precise=false

[GerdP]

My normal workaround for your example would be to remove the only tag from one way first so that there are no conflicts, but a new button "Accept all" or similar might also be a good solution.

[skyper]

Ticket #23560 has been marked as a duplicate of this ticket.

[watmildon]

This also really breaks the conflation plugin. For scenarios like ​https://www.openstreetmap.org/user/watmildon/diary/401407

[skyper]

Replying to GerdP:

My normal workaround for your example would be to remove the only tag from one way first so that there are no conflicts, but a new button "Accept all" or similar might also be a good solution.

How about a third checkbox on top which changes to the old behavior?

[stoecker]

Replying to skyper:

Replying to GerdP:

My normal workaround for your example would be to remove the only tag from one way first so that there are no conflicts, but a new button "Accept all" or similar might also be a good solution.

How about a third checkbox on top which changes to the old behavior?

The "accept all" button I think is a better idea than promoting a switchback to the version which silently combines.

[skyper]

Replying to stoecker:

Replying to skyper:

Replying to GerdP:

My normal workaround for your example would be to remove the only tag from one way first so that there are no conflicts, but a new button "Accept all" or similar might also be a good solution.

How about a third checkbox on top which changes to the old behavior?

The "accept all" button I think is a better idea than promoting a switchback to the version which silently combines.

Sorry, bad wording. I meant instead of a button a third checkbox Accept all keys with only one value. The advantage is that the setting is stored.

Replying to watmildon:

This also really breaks the conflation plugin. For scenarios like ​https://www.openstreetmap.org/user/watmildon/diary/401407

I guess, the conflation plugin needs a new option to temporally use the old behavior. The new behavior is much safer in case of merging nodes, combining ways and replacing geometries so disabling it completely is not the best choice but I can see problems when conflating as many tag conflicts dialogs may pop up. New ticket?

[taylor.smock]

Note: I'm taking a look at this from the conflation plugin side. It looks like we may want to make a new method that allows plugins to toggle back and forth.

For the Replace all option, can we just reuse the standard conflict dialog we have for conflicts we have with the OSM server (wiki:Help/Dialog/Conflict)?

EDIT: Specifically for the Conflation plugin, it has an option to overwrite tags without confirmation, whether or not it should merge tags, all tags, or just a subset, and so on.

[skyper]

Replying to taylor.smock:

For the Replace all option, can we just reuse the standard conflict dialog we have for conflicts we have with the OSM server (wiki:Help/Dialog/Conflict)?

This will only work for Replace Geometry as both, Merge Nodes and Combine Ways, can have more than two objects involved and therefore more than two options to choose from.

[taylor.smock]

Possible patch adding function for use by plugins

[GerdP]

What should this patch do?

[GerdP]

Didn't you mean this?
if (!onlyConflictingTags && Config.getPref().getBoolean("combine-conflict-precise", true)) {

[taylor.smock]

It should allow plugins to say "only show the dialog if there are actual conflicting tags".

So yes, I meant !onlyConflictingTags. Which I probably would have discovered as soon as I get a patch put together for utilsplugin2 and conflation.

[taylor.smock]

Add enum for different tag merge strategies. Notably only ASK is actually used right now (everything else is effectively KEEP_NON_CONFLICTING right now). See ​https://github.com/JOSM/conflation/pull/19 for the conflation plugin.

[stoecker]

Many @since for public stuff missing in the patch.

[GerdP]

I don't like (or understand) the meaning of Strategy.ASK. Both the if and the else part in this block

        if (Strategy.ASK == strategy && Config.getPref().getBoolean("combine-conflict-precise", true)) {
            tagModel.prepareDefaultTagDecisions(getResolvableKeys(tagsOfPrimitives.getKeys(), primitives));
        } else {
            tagModel.prepareDefaultTagDecisions(false);
        }

are meant to show a dialog asking the user for a response when there are conflicts. The difference is that the if part implements what I described with "adds code to check if a conflict exists where one or more tagged ways don't have the same tag value and - if so - show the conflict dialog." in the svn commit. (I should have written OSM objects instead of ways.)

I wouldn't want to have a different behaviour in action Replace Geometry and Combine Ways, they should just depend on the user preference. So I see no reason to change the code in utilsplugin2 (besides the refactoring changes).

My understanding of the problem with the conflation plugin is that users may want to ignore the setting combine-conflict-precise=true when using this plugin. Always or just in special cases where many objects are treated?

[GerdP]

Sorry, bad wording. I meant instead of a button a third checkbox Accept all keys with only one value. The advantage is that the setting is stored.

I'd also prefer this solution. I just noticed that the changes for #23305 somehow disabled these checkboxes, they no longer have an effect unless the preference combine-conflict-precise is set to false. That's a regression. Hm, depends on whether you count null as a values or not.

[GerdP]

The problem with the additional checkbox is that it "comes too late". The current code prepares a solution and if conflicts are found the dialog is displayed. It is difficult to separate the automatic decisions from those made by the user once the dialog is visible.
It's also hard to say what a user wants when the checkbox is unselected. Should all decisions be reset to undecided? I'll attach a patch that just adds a new button when combine-conflict-precise=true.

Reg. the conflation plugin: Would it be an option to temporirally set the preference combine-conflict-precise to false and restore the original value after the conflate action was performed?

[GerdP]

add new button to dialog to accept all tags with one non-null value

[GerdP]

dialog with new button (when preference combine-conflict-precise=true)

[GerdP]

after "Accept simple" was pressed

[Msiipola]

I don't need any new button, because the setting combine-conflict-precise=false solves my issues.

But I don't understand why the Replace function was changed in the first place?
The change has created much trouble for many.

[GerdP]

The Replace Geometry action uses the same dialog as the Combine ways action, see #23305 for the original reason to change the dialog.

[taylor.smock]

Replying to GerdP:

I don't like (or understand) the meaning of Strategy.ASK.

ASK is for if there are any conflicts including when one object has tags that the other object does not.

I wouldn't want to have a different behaviour in action Replace Geometry and Combine Ways, they should just depend on the user preference. So I see no reason to change the code in utilsplugin2 (besides the refactoring changes).

The problem is that the ReplaceGeometry class is used by other plugins (in this case, conflation), and they may or may not have code already that checks for conflicts and special cases some things (e.g. addresses onto buildings).

My understanding of the problem with the conflation plugin is that users may want to ignore the setting combine-conflict-precise=true when using this plugin. Always or just in special cases where many objects are treated?

The plugin has the following configuration options:

• Replace Geometry
• Merge Tags
  • All
  • Specific tags
  • Except tags
• Overwrite tags without confirmation
  • none, or specific

Technically, the conflation plugin could set combine-conflict-precise to false, but I think it is a worse solution long-term.

When I was writing the enum, I was trying to think of possible "defaults" that might be useful to show the user:

• Action wants to use tags from the source, but wants to give the user a chance to use tags from the target
• Action wants to use tags from the target, but wants to give the user a chance to use tags from the source
• Action wants to use tags from both, but wants to give the user a chance to avoid that
• Action wants to only use non-conflicting tags, but wants to give the user a chance to correct conflicts (old behavior)
• Action wants to only use non-conflicting tags that match, and has to give the user a chance to select the "right" tag (new behavior)

Of course, none of those besides the old behavior is implemented.

Replying to stoecker

Many @since for public stuff missing in the patch.

The stuff missing @since is actually in utilsplugin2 -- I generated the patch from the root checkout.

[GerdP]

Not sure what you mean with source and target here. We merge the tags of two or more source objects to apply them to one target object.
At least when you combine two or more ways or merge multiple nodes. Maybe other actions really have only two objects and one is kept as target.
I am not sure but I think the current code in the doesn't even know where the tags come from.

[taylor.smock]

From the current javadocs,

    /**
     * [...]
     * @param tagsOfPrimitives The tag collection of the primitives to be combined.
     *                         Should generally be equal to {@code TagCollection.unionOfAllPrimitives(primitives)}
     * @param primitives The primitives to be combined
     * @param targetPrimitives The primitives the collection of primitives are merged or combined to.
     * [...]
     */

I don't think the current code knows where the tagsOfPrimitives collection comes from, it is just what the caller passes to the method.
As far as getting where tags come from, the function could call TagCollection.unionOfAllPrimitives(primitives|targetPrimitives), and use that to prefill the dialog selections (and mark the prefill selections with red or yellow or something).

[GerdP]

@Taylor: I keep getting mails from github instance(s) about changes in JOSM plugins, e.g. for conflation, but I have no idea what to do with them. I guess these come from your private git experiments?

[GerdP]

reg. Strategy.ASK: My understanding is that the strategy should be about the possible preparations of MultiValueResolutionDecision.
As of now, the only purpose is to ignore the combine-conflict-precise preference.
So the enum could be something like this:

    public enum Strategy {
        /** Prepare decision so that only identical tags in all objects are resolved */
        PRECISE,
        /** Prepare tags with only one value as resolved, even if some objects don't have that tag */
        RELAXED
    }

[GerdP]

Maybe the better solution is to revert the change r18988 (and further ones) which were made for #23305? I wouldn't mind to do that.

[taylor.smock]

Replying to GerdP:

@Taylor: I keep getting mails from github instance(s) about changes in JOSM plugins, e.g. for conflation, but I have no idea what to do with them. I guess these come from your private git experiments?

For the conflation plugin, no (I was working on significantly improving performance while I was experimenting with how the conflict dialog changes worked out). I've got to figure out what is going on with the reports. Or just remove them.

As far as the enum goes, we could go with what you suggest. Or we could revert r18988 since that is (apparently) breaking a lot of things for users.

How about, instead of fully reverting r18988, we overload the method to take a strategy (as I was thinking originally), but instead put the preference value check in the calling methods (or add another method in CombinePrimitiveResolverDialog that will return the "suggested" strategy).

[skyper]

I just tried to combine two ways with identical tags but one membership conflict (in addition to many route relations in common) and was not able to get the "OK" button enabled (e.g. combine the ways). Looks like there are more problems beside the change of behavior and the improperly working checkboxes on top.

[GerdP]

@Skyper: Cannot reproduce that. Please open a new ticket with all details.

[GerdP]

How about, instead of fully reverting r18988, we overload the method to take a strategy (as I was thinking originally), but instead put the preference value check in the calling methods (or add another method in CombinePrimitiveResolverDialog that will return the "suggested" strategy).

I thought about this as well but that would mean that a user has to set/change a lot of preferences. We use this dialog in many places and I simply was not aware that some automated edits are done using this dialog. Knowning all this now I'd prefer to revert the changes.

[taylor.smock]

OK. Works for me then. Although I think we should add a test or comment somewhere so we don't forget and do this again in 5 years.

[GerdP]

I agree reg. a unit test, but I don't know how to write it regarding the headless environment. Up to now there is no unit test for class CombinePrimitiveResolverDialog.

[taylor.smock]

OK. I'll see if I can make one.

[GerdP]

Ticket #23579 has been marked as a duplicate of this ticket.

[GerdP]

In 19022/josm:

see #23305 and #23555:
-revert most of the changes in r18988, it caused too many problems with plugins which relied on the old behaviour

[skyper]

When replacing geometry of an existing way without any tags nor relation memberships with a new way with several tags and many memberships I, now, get a dialog where I have to click for each membership to keep. If I remember correctly there was not conflict dialog created in the past and all memberships were kept automatically. I think it is save to suppress the tags/membership dialog if only one of the two involved objects has tags and memberships at least for Replace Geometry. Another more general option would be an option (button) to keep all memberships.

Replying to skyper:

I just tried to combine two ways with identical tags but one membership conflict (in addition to many route relations in common) and was not able to get the "OK" button enabled (e.g. combine the ways).

Replying to GerdP:

@Skyper: Cannot reproduce that. Please open a new ticket with all details.

I cannot reproduce either. I probably missed one membership conflict scrolling through the over 100 memberships.

[GerdP]

Cannot we simply close this because of r19022?