Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn before deleting a member of a relation [$15] #1461

Closed
yvecai opened this issue May 9, 2013 · 41 comments
Closed

Warn before deleting a member of a relation [$15] #1461

yvecai opened this issue May 9, 2013 · 41 comments

Comments

@yvecai
Copy link
Contributor

yvecai commented May 9, 2013

User can delete a relation member without warning.
Either deletion should not be allowed at all, or an explicit message should pop.

@brycenesbitt
Copy link
Contributor

Having fixed (or helped to fix) damaged relations created by well meaning P2 editors, I think the problem predates iD. An intrusive "Are you sure, really sure: newbie!" would be intrusive. But that's not the only option. Consider the educational opportunity. A message more on the order of:

"Hey, we just noticed that you've edited part of a relation for the first time. Relations can be a bit tricky, want to learn more? [YES] [NO] [Don't Bug Me I'm An Expert]"

iD is already brim full of helpful little messages, why not for a subjects that are really opaque, like relations (and multipologons)?

@ftrebien
Copy link

I don't see a problem with it being "intrusive". JOSM is too, and that's one of the reasons why JOSM-based edits tend to present significantly fewer mistakes. (The other one is the required steep learning curve.)

If relations can't be handled automatically (and they can't, unless you're merging two consecutive members of the same relation), they must be handled manually, and the user should be instructed on how they work, what they mean, and how to avoid problems when handling them.

Why knowing and caring for relations is important: they are used to define multipolygons, country/state/city/suburb/neighbourhood boundaries, public transit routes, turn restrictions, and some others (http://wiki.openstreetmap.org/wiki/Types_of_relation). All those pieces of information can be damaged without proper attention and care, so this is a very important issue. Not having this generates great corrective strain on map maintainers.

@PolyglotOpenstreetmap
Copy link

I went out of my comfort zone to try and delete a way that I recently
added, which is part of 2 boundary relations. No warning whatsoever was
given that this would cause problems.

I don't understand such behaviour. It took me a whole weekend to add all
those boundaries and they can be so easily broken. That's a bit
discouraging, really.

Polyglot

2014-02-14 23:36 GMT+01:00 ftrebien notifications@github.com:

I don't see a problem with it being "intrusive". JOSM is too, and that's
one of the reasons why JOSM-based edits tend to present significantly fewer
mistakes. (The other one is the required steep learning curve.)

If relations can't be handled automatically (and they can't, unless you're
merging two consecutive members of the same relation), they must be handled
manually, and the user should be instructed on how they work, what they
mean, and how to avoid problems when handling them.

Why knowing and caring for relations is important: they are used to define
multipolygons, country/state/city/suburb/neighbourhood boundaries, public
transit routes, turn restrictions, and some others (
http://wiki.openstreetmap.org/wiki/Types_of_relation). All those pieces
of information can be damaged without proper care, so this is a very
important issue. Not having this puts great corrective effort in the
shoulder of map maintainers.


Reply to this email directly or view it on GitHubhttps://github.com//issues/1461#issuecomment-35131933
.

@ftrebien
Copy link

I thought a little more and there's a key reason why there should be an intrusive warning: relations are essentially invisible. Well, they are visible, but only indirectly, you must know they exist and look for them. They're not like nodes and ways that you see right away, can pull around, delete, edit, they're "things" linked to these objects with no obvious representation on the screen. The only way an average user can handle the invisible is by making it somehow more visible.

Because relation members are highlighted, deleting them may or may not issue a warning (after all, there was a warning, even though you'd need to know the visual language; still, I think a warning is a good idea). A bigger problem is when you're merging ways that are part of relations. This will alter relations in inconsistent ways which are very easy to go completely unnoticed.

@yvecai
Copy link
Contributor Author

yvecai commented Feb 15, 2014

Relations could be highlighted when one of its members is selected.
But, given that relations very often extends beyond viewport at the zoom level you can edit, definitively a warning box should appear.

@ftrebien
Copy link

Exactly. This is how I think handling relations should work when:

  • deleting a member of any kind: display a warning dialog, providing a checkbox allowing the user to skip the warning for the rest of the session (but not forever, a user may stay away from OSM for months and forget how this works)
  • merging two ways: display a different warning dialog with its own skipping option checkbox; the checkboxes should be independent of each other because the user may not have realized that merging implies deleting one way and modifying the other (quite likely for new users and for those that are not familiar with programming logic)
  • splitting a way: a warning should be displayed when it is not possible to safely alter the relations; for instance, if the relation contains members A>B>C (> means "followed by") and B is split into B1 and B2, it will become A>B1>B2>C if B1 is connected to A, or A>B2>B1>C if B2 is; if none are or if both are, it is impossible to decide and a warning is necessary; an analogous test is necessary if the role is at the beginning of the member list; as Polyglot pointed out, on turn restrictions, membership is not copied but instead given only to the way that is connected to a "via"-object in the relation (if both are, then it's not possible to decide and a warning must be issued).

For the latter case, if the logic becomes too complicated, then it's best to always issue a warning, as JOSM does.

Simple messages for both cases could be more or less like this:

  • "You are trying to delete an object which is part of a relation. If you are not sure that this relation should lose this object, please do not delete this object. To learn more about how relations work, read about relations or ask the community."
  • "You are trying to merge a way that is part of a relation with another way which is not. As a result, the information represented by this relation may become incorrect. If you are not sure that this relation will remain consistent, please do not merge these ways. To learn more about how relations work, read about relations or ask the community."
  • "You are trying to split a way that is part of a relation but the members are not sorted in a way that allows iD to safely modify them. (...)" or "You are trying to split a way that is part of a relation. The new ways will be added to it, possibly in the wrong order. As a result, the information represented by this relation may become incorrect. (...)"

@PolyglotOpenstreetmap
Copy link

When splitting, the order in which the ways are included matters. For turn
restrictions I suppose it's not needed to add both members (with from or to
roles).

Polyglot

2014-02-15 17:14 GMT+01:00 ftrebien notifications@github.com:

Exactly. This is how I think handling relations should work when:

  • deleting a member of any kind: display a warning dialog, providing a
    checkbox allowing the user to skip the warning for the rest of the session
    (but not forever, a user may stay away from OSM for months and forget how
    this works)
  • merging two ways: display a different warning dialog with its own
    skipping option checkbox; the checkboxes should be independent of each
    other because the user may not have realized that merging implies deleting
    one way and modifying the other (quite likely for new users and for those
    that are not familiar with programming logic)
  • splitting a way: no warning, just make sure both new ways replace
    the role of the old way in its relations

JOSM actually issues a warning for all 3 situations, but no warning is
necessary for the latter if the relation was already valid before editing.

Simple messages for both cases be like this:

"You are trying to delete an object which is part of a relation. If
you do not know what this means, please do not delete this object. If you
wish to learn more about how relations work, [url="
http://wiki.openstreetmap.org/wiki/Relation"]read this[/url] and/or
[url="http://wiki.openstreetmap.org/wiki/Contact"]ask the
community[/url]."

"You are merging a way that is part of a relation with another way
which is not. As a result, the information represented by this relation may
become incorrect. If you do not know what this means, please do not merge
these ways. If you wish to learn more about how relations work, [url="
http://wiki.openstreetmap.org/wiki/Relation"]read this[/url] and/or
[url="http://wiki.openstreetmap.org/wiki/Contact"]ask the
community[/url]."


Reply to this email directly or view it on GitHubhttps://github.com//issues/1461#issuecomment-35159885
.

@ftrebien
Copy link

You're correct, Polyglot. I've added these special cases to my previous post.

@nighto
Copy link

nighto commented Apr 22, 2014

Any news on this subject? This seems to me as very important and would ease the maintenance that hard-users need to do to keep the relations in areas they manage in a OK state.

@naoliv
Copy link

naoliv commented Apr 22, 2014

Like I said in #2014, it's annoying to keep fixing relations (mainly administrative boundaries); I could be fixing or mapping other things with this time.

This warning message should have a higher priority.

@brycenesbitt
Copy link
Contributor

iD must overcome a user's worry 'they might break something'. Undo is a feature that addresses this worry. Strong warnings on dangerous edits are another. I view it as a beginner-friendly reduce-the-barrier type of behavior!

@Skippern
Copy link

The amount of data being broken, the number of new inexperienced users keeping committing these errors, and the lack of hands to solve this is beginning to strain peoples patience, especially (as I know) in Brazil. Isn't there any way a warning, even a friendly warning, can come up when a user is editing objects with relations?

@daganzdaanda
Copy link

+11, please!
Also, the messages that @ftrebien proposes sound nice and helpful to me.

As a basic rule, maybe a user should not be allowed to edit anything that is not completely visible, i.e. in the viewport? I guess that would make it impossible to edit huge multipolygons in iD, but it may just be the wrong tool for that. JOSM gives a warning if you want to delete something that is not completely downloaded, because it might have connections to other things.

Another, maybe stupid idea: have a "superuser" mode that can only be switched on after maybe 150 edits. Only as a superuser, you can edit boundaries and coastlines.

@naoliv
Copy link

naoliv commented Sep 29, 2014

@daganzdaanda the number of changesets, unfortunately, doesn't mean anything today. New iD users create a lot of changesets with very simple edits. See #703, #1598 and others, for example.

@daganzdaanda
Copy link

Yeah, I've noticed. It might need a calculation like "x active mapping days within the last 3 months"... I don't think its easy to find out who is experienced enough to be "superuser". That's partly why I wrote that the idea is probably stupid.

@nighto
Copy link

nighto commented Sep 29, 2014

I think a simple setting - "Enable relations edit" or something like that -
would be enough.
Em 29/09/2014 14:46, "daganzdaanda" notifications@github.com escreveu:

Yeah, I've noticed. It might need a calculation like "x active mapping
days within the last 3 months"... I don't think its easy to find out who is
experienced enough to be "superuser". That's partly why I wrote that the
idea is probably stupid.


Reply to this email directly or view it on GitHub
#1461 (comment).

@naoliv
Copy link

naoliv commented Dec 26, 2014

I will bring cake in May, for the 2 year anniversary.

@PolyglotOpenstreetmap
Copy link

You can already bring cake in February for the 1 year anniversary of this
bug, if it still exists. I didn't test anymore.

Polyglot

2014-12-26 19:20 GMT+01:00 Nelson A. de Oliveira notifications@github.com:

I will bring cake in May, for the 2 year anniversary.


Reply to this email directly or view it on GitHub
#1461 (comment).

@bhousel bhousel changed the title Warn before deleting a member of a relation Warn before deleting a member of a relation [$15] Dec 30, 2014
@bhousel bhousel added the bounty label Dec 30, 2014
@bhousel
Copy link
Member

bhousel commented Dec 30, 2014

Rather than complaining about it, I'll just buy an actual cake for whomever fixes this.

@brycenesbitt
Copy link
Contributor

I'll contribute the cost of mailing the cake to the person who fixes it.

@malenki
Copy link

malenki commented Jan 1, 2015

In the German OSM Forum I created a thread to collect issues matching this topic.
During the last ten days the following was reported:

@malenki
Copy link

malenki commented Jan 8, 2015

@Nakaner
Copy link

Nakaner commented Jan 19, 2015

New examples are:

This issue is already open. The German community is happy that new users join OpenStreetMap every day but the price we (the mappers) pay reaches its upper limit. Relations break easily if the editor does not warn its users. Relations are important for geocoding (boundary relations).

README.md says:

It lets you do the most basic tasks while not breaking other people's data.

This is wrong at the moment. Please keep this in mind that iD exists to help the community, not to harm it.

iD is still (thank you for fixing it) the synonym for bad changes. The German mapping community expects you to fix this issue. It is almost two years old!

I suggest you to:
(1) either prevent such damaging relation-related changes

  • by disabling deletions of ways which belong to relations
  • by disabling deletion of whole relations
  • by disabling splitting and merging of ways which belong to a relation (not necassary if iD already cares about this)

(2) or add methods to ensure that edits do not break relations

  • by good visible warnings during editing that the user breaks something (kerosin proposed nice texts)
  • If the user presses "ignore, I'm an expert", he should see another warning (like the current one below changeset comment field) which warns "You are going to upload one defect multipolygon. Repair/Show | Ignore" and below all warnings "Ignoring warnings might get you into truble with the community." (you might use a more friendly text)

@PolyglotOpenstreetmap
Copy link

I fully agree with Michael Reichert's message. It could, of course, also be
route and other types of relations, which are being broken.

How difficult can it really be to show a warning message to users? And if
design decisions are not contributing to improvement of the data, they'll
have to be reconsidered.

Polyglot

2015-01-19 17:13 GMT+01:00 Michael Reichert notifications@github.com:

New examples are:

This issue is already open. The German community is happy that new users
join OpenStreetMap every day but the price we (the mappers) pay reaches its
upper limit. Relations break easily if the editor does not warn its users.
Relations are important for geocoding (boundary relations).

README.md says:

It lets you do the most basic tasks while not breaking other people's data.

This is wrong at the moment. Please keep this in mind that iD exists to
help the community, not to harm it.

iD is still #542 (thank you
for fixing it) the synonym for bad changes. The German mapping community
expects you to fix this issue. It is almost two years old!

I suggest you to:
(1) either prevent such damaging relation-related changes

  • by disabling deletions of ways which belong to relations
  • by disabling deletion of whole relations
  • by disabling splitting and merging of ways which belong to a
    relation (not necassary if iD already cares about this)

(2) or add methods to ensure that edits do not break relations

  • by good visible warnings during editing that the user breaks
    something (kerosin proposed nice texts
    Handle potentially dangerous changes to relations #2503 (comment))
  • If the user presses "ignore, I'm an expert", he should see another
    warning (like the current one below changeset comment field) which warns
    "You are going to upload one defect multipolygon. Repair/Show | Ignore"
    and below all warnings "Ignoring warnings might get you into truble with
    the community." (you might use a more friendly text)


Reply to this email directly or view it on GitHub
#1461 (comment).

@brycenesbitt
Copy link
Contributor

iD is positioned as the easy editor meant to bring beginners into OSM editing. As such this is a big deal. An iD user who breaks something is having a bad experience, not to mention the people who have to come after and fix it up.

@malenki
Copy link

malenki commented Feb 10, 2015

@Skippern
Copy link

With Changeset comments I think it is a more bad experience for the beginning user getting complaints from us who are maintaining areas, then a warning banner when they are about to break something.

When I see change sets with bad data I do not check to see what editor they use or how much experience they have, I quickly post a comment on the changeset informing them about the damage and kindly ask them to fix it themselves. I think if iD gave a friendly warning at the time of edit, the beginners would be aware of the situation instead of getting an (at times) angry message from some total stranger.

Often it is the same users I see doing the same mistakes or damages within the area I monitor, and almost always it turns out to be done with iD.

@Skippern
Copy link

In a relatively short editing spree, a somewhat experienced user broke several relations I had used a week or so putting in place. This experienced user are mainly using iD, and instead of iD notifying him when breaking relations, I have to do that job and remind him. This user edits all over Brazil, not only the area I monitor, so it is likely that he have broken even more relations without me noticing. I've had enough of reminders to him, and are just about starting to revert any changeset he touches containing relations, either until iD sort things out with themselves, or this user changes to a proper editor.

Lists of change sets for today:

@naoliv
Copy link

naoliv commented Feb 11, 2015

I pay another cake for whoever fix this.

@tmcw
Copy link
Contributor

tmcw commented Feb 12, 2015

Since a few people asked for context that would be useful if they wanted to solve this issue:

User Experience

Going to assume / best guess that the desired UX is

  • click on feature within relation to delete
  • click delete
  • have to confirm whether you want to delete a member of a relation

This brings up the issues that

  • deleting members of relations repeatedly will be a chore
  • lots of objects are in relations and should be deletable without incident.

Would love to see clever solutions to those.

Anyway

The code:

Basic jist would be to hook into the delete operation, check to see if the things being deleted have parent relations, and ask for confirmation with an iD.ui.Modal if so.

@systemed
Copy link
Contributor

Right. The PR I've submitted above is pretty much the same, but it requires the user to expressly remove the way from the relations rather than using a modal, because (he says glibly) modals are often not great UI - experience with P2 is that people just click whatever gets the modal out of the way fastest.

By prompting the user (in the tooltip) to expressly remove the way from the parent relation, it makes clear what's happening, giving the opportunity for a "wait, maybe I don't want to do that" moment.

@naoliv
Copy link

naoliv commented Feb 12, 2015

@systemed seems a good approach

@Skippern
Copy link

@systemed I like that approach. That is slightly stricter than the approach in JOSM, I cannot vouch for how it is done in other desktop editors, Go Map!! have completely disabled deleting and cutting, so there is no risk for this, and I guess other mobile editors follow the same line (also not fully to my knowledge). It is actually tempting to install Merkaartor to see how it is handled there.

@jfirebaugh
Copy link
Member

Fixed by #2526. @bhousel and @naoliv, I'll leave it to you to arrange cake delivery to @systemed.

@bhousel
Copy link
Member

bhousel commented Feb 12, 2015

sweet! 🍰

@systemed can go to bountysource, claim his $15, then buy a cake.

Or you can also buy something else, or pay it forward to another opensource project if that's what you want...

@naoliv
Copy link

naoliv commented Feb 12, 2015

I've sent him an e-mail asking the best way to pay my part.

@yvecai
Copy link
Contributor Author

yvecai commented Feb 12, 2015

Congratulation to all, lobbyists, bounty hunters included :)

On 12 février 2015 20:17:55 UTC+01:00, "Nelson A. de Oliveira" notifications@github.com wrote:

I've sent him an e-mail asking the best way to pay my part.


Reply to this email directly or view it on GitHub:
#1461 (comment)

Envoyé de mon téléphone Android avec K-9 Mail. Excusez la brièveté.

@ftrebien
Copy link

I don't think this has been properly handled yet. The user cannot delete a way that belongs to a relation, but he/she can still combine that way with other ways without getting any warning saying the relation (that was previously fine) makes no sense at all after combining its members. I just tried that on a bus route and a district boundary, all I needed to do was to combine two streets, one member to a relation, the other not a member. The resulting relation has the combined way as a member, which would be ok if the user got warned that the relation could become broken. JOSM handles this very nicely by warning the user and letting him/her choose to accept or not the consequences. In the way iD currently handles this, the user never knows that he/she has broken something. I see a pattern here: issue #2358 is analogous, but for tags. How hard is it really to warn the users? I'm sure nobody has the intention of breaking data, so why be against adding a simple warning?

@PolyglotOpenstreetmap
Copy link

I totally agree with you. It seems like the developers op iD either don't
understand the data model of OSM, but why would one want to develop an
editor of said data without understanding it?

They seem to have 'principles' which according to them are more important
than the data. But in reality nothing is more important than the data, when
you're developing an editor for it. The data and the time wasted by other
users who constantly need to patrol and try to mend everything broken by
users of an editor which is really broken.

So, I'll keep trying to convince users to start using a real editor as
quickly as possible, as iD is really doing them a disservice. One of these
days I'm going to get on my soapbox and write a blog entry about it. It's
really unbelievable this is still a problem, after all this time.

Polyglot

2015-06-26 9:40 GMT+02:00 ftrebien notifications@github.com:

I don't think this has been properly handled yet. The user cannot DELETE a
way that belongs to a relation, but he/she can still combine that way with
other ways without getting any warning saying the relation (that was
previously fine) makes no sense at all after combining its members. Just
tried on a bus route and a district boundary, all I needed to do was to
combine two streets, one member to a relation, the other not a member. The
resulting relation has the combined way as a member, which would be ok if
the user got a warning that the relation could become broken. JOSM handles
this very nicely by warning the user and letting him/her choose to accept
or not the consequences. In the way iD currently handles this, the user
never knows that he/she has broken something. I see a pattern here: issue
2358 is analogous, but for tags. How hard is it really to just display a
warning for the users? I'm sure nobody has the intention of breaking data,
so why be against adding a simp le warni ng?


Reply to this email directly or view it on GitHub
#1461 (comment).

@systemed
Copy link
Contributor

@ftrebien @PolyglotOpenstreetmap thank you for reminding me why I was so relieved to step down from maintaining the default OSM editor. (Basically, you two were the reason.)

@tmcw
Copy link
Contributor

tmcw commented Jun 26, 2015

This issue, titled "Warn before deleting a member of a relation", is closed, because iD warns before deleting a member of a relation. If you'd like to continue insulting the volunteers who make this free and open source editor so that you can use it, do so on the correct ticket, #2358, or, even better, become one of them and stop trolling.

Locking this ticket.

@openstreetmap openstreetmap locked and limited conversation to collaborators Jun 26, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.