Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3777 closed enhancement (fixed)

Potlach 2 doesn't warn, when corrupting relations

Reported by: petr.dlouhy@… Owned by: potlatch-dev@…
Priority: minor Milestone:
Component: potlatch2 Version:
Keywords: Cc:

Description

When merging two ways, which one of them is part of a relation and the second isn't, Potlach doesn't warn user. This leads to corruption of the relations

Change History (10)

comment:1 Changed 8 years ago by Richard

Priority: criticalminor
Type: defectenhancement

a) This is not remotely critical.

b) "Corruption" is not remotely the right word. Are the relations still well-formed? Yes. Therefore they are not corrupted.

c) In most cases, Potlatch 2 renders ways that are part of relations (routes, turn restrictions, multipolygons) differently. It is therefore easy for the user to see the effect of their actions. Several years' experience shows me that accidental merges of a relation-ed way with another are almost entirely done in JOSM, which AIUI until the new P2-like MapCSS rendering didn't actually show relation-ed ways differently at all. For example, http://www.openstreetmap.org/browse/changeset/8198740 was mending extensive damage to a route relation by a JOSM user. Generally I have to do this several times a week.

d) If you there is a particular type of relation that is not adequately rendered in P2 and so the user can't see what they've done, please cite it here, rather than coming up with a vague handwavy ticket like this.

e) Popping up warnings ("Do this! Look at that! Do the other") is terrible UI and is generally ignored by most people.

f) Please learn to spell Potlatch, kthxbye.

comment:2 Changed 8 years ago by woodpeck

Could either of you, for the benefit of others reading this, explain *what* exactly Potlatch does in the case discusssed? Will the new, combined way be part of the relation or will it not? Or does it depend on whichever way was selected first? Properly documenting this would go a long way to avoiding problems, even without any change to the code, I should think.

As for popping up warnings, I think Potlatch has on a number of other occasions already gone a different way - instead of "are you sure you want to delete 12345 nodes", Potlatch would say "deleted 12345 nodes - hit Ctrl-Z to undo" or something. Maybe it is possible to do something similar with way merges in the situation that Peter described.

comment:3 Changed 8 years ago by Richard

The new combined way will be part of the relation. This has always been the case in all versions of Potlatch 1 and 2.

Because Potlatch (1 and 2) generally renders relations, it should be immediately apparent to the user what their action has been. If there is a relation that is not adequately rendered by P2 - and that's certainly possible, because in P2 relation rendering is dictated by the stylesheet rather than being automatic as in P1 - then I'd very much like to hear of it so that I can fix it.

P2 already floats a "check tags" advice bubble after merging ways (the warning is far too verbose and is therefore probably ignored by most users, which needs fixing). That could potentially be "check tags and relations" in these cases, but again, users don't read warnings. Fixing rendering is the first half of the proper way to do it; the second half is to have a friendly "mapping quality" palette (in a much less prescriptive way than the existing validator tools) but that's a long-term aim that will take time to do properly.

comment:4 Changed 8 years ago by Petr Dlouhý

This ticket is based on following commit (although it is not very clear from the commit page, that it do harm): http://www.openstreetmap.org/browse/changeset/8021768

I am not sure, how far you want to go in rendering the relations. In JOSM is routes plugin (http://wiki.openstreetmap.org/wiki/JOSM/Plugins/Routes), which visualizes the hiking/bicycle/other relations (it can even draw parallel lines for more relations on one way).

comment:5 Changed 8 years ago by Richard

Potlatch 2 renders route relations such as that. It has always done so, without any need for a plugin or alternative stylesheet or whatever. I don't see any evidence that this is an editor issue. It simply looks like a mapper who made a mistake. We can't completely stop that happening, you know. :)

comment:6 Changed 8 years ago by Petr Dlouhý

He doesn't made a mistake because he was distracted. He made a mistake, because he doesn't know, what he was doing and the editor didn't gave him an advice even if it could.

Why does Potlatch warn if merging two ways with different tags, but does not when merging two ways with different relation membership? Isn't the situation identical?

comment:7 Changed 8 years ago by Richard

Sure. Like I said above, it could do. The reason why it doesn't yet, I guess, is because neither you nor I nor anyone has fixed it to do so. I'll do that at some point but my willingness to do so is not best helped by trac tickets screaming "critical" and ZOMG CORRUPTION THIS MUST BE FIXED.

But as I keep saying: Potlatch _does_ make the user aware that they have extended the relation membership to a part of the way that was not previously a member. It does this by clearly rendering the way differently. The user is looking at the map so will see this.

After all, should Potlatch pop up an alert saying "You changed the highway type from trunk to primary" every time you do that? Even though the change is reflected in a different rendering? And so on.

comment:8 Changed 8 years ago by Petr Dlouhý

Sorry for my overreaction, but I really think, it is serious problem. Here is another damaging edit from different user: http://www.openstreetmap.org/browse/changeset/8093999

comment:9 Changed 8 years ago by Richard

Resolution: fixed
Status: newclosed

Fixed in v2.2; it's now included in the "tags merged" pop-up advice.

Perhaps now JOSM can make users aware of merged relations, too, so that JOSM users stop destroying the National Cycle Network. ;)

comment:10 Changed 8 years ago by Petr Dlouhý

Thanks

Note: See TracTickets for help on using tickets.