streetcomplete / StreetComplete

Easy to use OpenStreetMap editor for Android
https://streetcomplete.app
GNU General Public License v3.0
3.9k stars 357 forks source link

Forbid deleting element that's part of a relation #5851

Closed vfosnar closed 1 month ago

vfosnar commented 2 months ago

City and community maintained tourist routes need special handling as they usually get repaired and if part is missing then the rest of the route is probably too.

How to Reproduce In quest "Is it still here" on information board that is part of a tourist track choose 'delete element'

Expected Behavior Hide/disable the 'delete element' button

Versions affected

tkas commented 2 months ago

The current behavior is, that all tags are removed from such node, but the node itself is kept, which is wrong. Either the node should be removed from the relations it is part of or the 'delete element' button should be disabled for such cases. For the situation of node (info board) being part of a way, it is not possible to decide, if the node should be removed (does not hold any geometric info for the way) or should be kept. In this case, the disabled button is probably the only choice.

westnordost commented 2 months ago

What do you mean with "the disabled button is probably the only choice."?

matkoniecz commented 2 months ago

Is it a theoretical bug or something encountered? I faintly remember deletion of relation members being disabled at least in some context.

tkas commented 2 months ago

What do you mean with "the disabled button is probably the only choice."?

I mean the removal from relation is safe, removal from a way can not be decided without user intervention and study of the underlying data. If I place aditional node on a way with info board (on the wall of te building), the node should be removed. If I use some node of the building that is part of its geometry, is should not be removed.

westnordost commented 2 months ago

Hm? Removal of some node from a relation doesn't seem safe to me. Very much depends on the type of relation and the role of the node that is about to be deleted.

tkas commented 2 months ago

Is it a theoretical bug or something encountered? I faintly remember deletion of relation members being disabled at least in some context.

Encountered for second time in few days. Example is https://www.openstreetmap.org/changeset/155834201, https://www.openstreetmap.org/node/3892283189

tkas commented 2 months ago

Hm? Removal of some node from a relation doesn't seem safe to me. Very much depends on the type of relation and the role of the node that is about to be deleted.

OK, we meet it in cases, where it is fine. There may be other situations, where it is a problem. Then the generic solution is to disable the 'delete' button/function if the node is part of either a way or a relation and link the user to some advanced editor to inspect the situation. Maybe some note or other such tag can be added to this node instead of deletion?

westnordost commented 2 months ago

This might be an issue. At the time the element is shown (in the quest / overlay), StreetComplete doesn't know whether an element is part of a way or relation. That's how the data is structured. Only on upload, this can be checked.

tkas commented 2 months ago

You are able to use only OSM API calls or Overpass is available as well?

( https://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL#Recurse_.28n.2C_w.2C_r.2C_bn.2C_bw.2C_br.29 )

tkas commented 2 months ago

Well looks OSM API should be fine as well: https://wiki.openstreetmap.org/wiki/API_v0.6#Ways_for_node:_GET_/api/0.6/node/#id/ways (next call in the docs for relations)

So a check before the upload occurs should be possible, right?

westnordost commented 2 months ago

Yes, as I wrote, a check can be done during upload. The app should and does work completely offline, so no calls to the OSM API are made during usage, only during download and upload.

westnordost commented 2 months ago

Sorry, I misremembered. It is actually possible in SC to check if a node is part of a relation when opening the element / quest, also offline.

However, and that is an issue, relations of the following types are discarded (i.e. not persisted in local database) on download by StreetComplete because of (very real) performance reasons:

"route", "route_master", "superroute", "network", "disused:route","boundary","water", "waterway", "watershed", "collection", "person","power", "pipeline", "railway"

What does that mean? That StreetComplete is simply not aware of the existence of these relations and thus also doesn't know if any element is part of any such relation EXCEPT during upload. It must be aware of these relations during upload because SC must be able to correctly modify such relations when e.g. uploading the splitting of ways. It specifically queries the OSM API during upload for this with the OSM API calls you mentioned.

tkas commented 2 months ago

O, so would you be able to either perform the check before you discard the relations or before the upload with OSM API? With info boards where we have global quality control for Czech Republic, we wind these cases (at least most of), but it is a global problem, so would be nice to fix it and not leave orphans in the data.

westnordost commented 2 months ago

The check is performed before upload to the OSM API. Only because of the check, the element is not deleted but only the tags are cleared. But what should the check do if the element turns out to be part of a relation after all? If the change is then simply discarded, the quest will just pop up for the user (and the next user, ...) again.

tkas commented 2 months ago

OK, I see. Then either we must find a way how to check before the quests are offered (to not offer this at all) or mark it in OSM data so it is not presented to the user again. With the second option - you set the check_date tag with current date, right? Could this be exploited - e.g. do not offer objects tested recently?

tkas commented 2 months ago

Another (suboptimal) option could be to use lifecycle prefixes instead of object deletion in these cases - e.g. removed: etc. ?

mnalis commented 2 months ago

Perhaps on upload, if to-be-deleted node is determined to be part of relation(s), instead of clearing the tags, we can auto-create a note at that node location saying that "The mapper on the ground indicated this no longer exists, but as it is part of relation, it needs separate checking and handling" ? (just like the user has chosen to "leave a note" manually).

That should both provide useful information to mappers with more capable editors, and skip quests for other StreetComplete users at the same time.

westnordost commented 2 months ago

check_date

Yeah, but setting the check date to now even though the user replies that it does not exist anymore seems like... adding wrong data to OSM. The check date is set to now if the user replied that indeed it still exists.

removed:

This could work, but also requires to find out first which tag to actually prepend with removed: first. Also, that's somewhat of a troll tag, I guess.

create note

This occured to me too. It would be somewhat of a precedence, as SC does not usually create automated notes. But I think this is going to be technically difficult, because the osm edit uploader knows nothing of notes. It would be very hacky, at best. At the very least, the auto-generated note would first appear as another edit in the undo history and probably not even uploaded in the same go.

mnalis commented 2 months ago

If auto-generating a note is an issue (and I see both the technical and the ideological problem there), perhaps instead that to-be-deleted element could be modified with fixme=* tag of the same content; and element selection of CheckExistence quest modified so it skips elements that have (all or only such) fixme tags?

westnordost commented 2 months ago

I fear this is not a solution because elements can be deleted in a number of different quests. In many quests, you have the other answer option to state that it does not exist.

westnordost commented 2 months ago

To the example at hand. The deletion was wrong because after all, the element existed but at a slightly different position? Or what exactly was the new situation?

matkoniecz commented 2 months ago

This could work, but also requires to find out first which tag to actually prepend with removed: first.

all of them?

Also, that's somewhat of a troll tag, I guess.

There are problems with some lifecycle prefix tagging (when used for old, historic objects not kept temporarily). But these are not troll tags in meaning of "and this tag inverts/negates meaning of another tag"

matkoniecz commented 2 months ago

Though I am not sure whether removed: prefixing is better than blanking tags.

I guess it looks less weird, but effect is basically the same. Maybe it saves confused mapper time needed to check object history?

tkas commented 2 months ago

To the example at hand. The deletion was wrong because after all, the element existed but at a slightly different position? Or what exactly was the new situation?

It kept a node without any tags on original position being part of the relation. (In the mean time I corrected this as the contributor confirmed it to be a mistake in this case and the board is there. For others I simply removed the node from the relation and delete the empty node to fix the situation)

mnalis commented 2 months ago

Though I am not sure whether removed: prefixing is better than blanking tags.

I'd say it is.

I guess it looks less weird, but effect is basically the same. Maybe it saves confused mapper time needed to check object history?

Yes it is easier, but even more so it makes it easy to search for those (in OverPass, or JOSM find, etc) and handle them properly

lgommans commented 2 months ago

StreetComplete is simply not aware of the existence of these relations and thus also doesn't know if any element is part of any such relation EXCEPT during upload

Has it been considered to set a boolean on the object, e.g. cannot_delete_cuz_relation=0|1, during download, when the to-be-discarded data is still there? Then showing the delete button can be greyed out if the bool is true, ideally with long-pressing resulting in a toast message that says "To avoid breaking a relation, this object cannot be deleted."

a bad 1dea Call the boolean `in_a_relationship` and, on April 1st, show the toast "To avoid heartbreak, StreetComplete does not allow breaking up relationships."
westnordost commented 1 month ago

So, regarding the latest suggestion, to mark elements during download if they are part of a relation and then based on this value, don't offer a "delete POI" option: Yes, it is possible, but this wouldn't solve the issue fully, as the node could have become part of a relation since it has been downloaded. With the current behavior already rarely being an issue, if that solution was implemented, it would just become even more rarely an issue, but not resolve it. So, I am not in favor of that.

To prepend all tags with removed: seems to be the most useful option to me, however, what exactly is the advantage over the current behavior, clearing all tags? @mnalis mentioned, that it would be easier to find such cases via Overpass.

First of all, not sure. Because there are a lot of removed: tags, how to find those that were created by users in StreetComplete deleting a node in a relation?

So, to me, the better solution is, and it was already suggested, also by @mnalis, additionally to clearing the tags (current behavior), add a fixme with a predefined text, such as "object not found on a survey, check if it can be deleted (from the relation)". Then, it becomes very easy to find this via Overpass. To find what it has been tagged as before is also trivial for people looking into it, just look into the history of that node.

westnordost commented 1 month ago

I settled for object not found on a survey, check if it should be deleted or deleted from the relation