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

Do not automatically remove amenity details in Places overlay when extra details are added via NSI #5940

Closed
mnalis opened this issue Oct 2, 2024 · 2 comments · Fixed by #5944
Assignees

Comments

@mnalis
Copy link
Member

mnalis commented Oct 2, 2024

Use case
Sometimes, some existing amenity/shop has lacking (e.g. no brand, brand:wikidata etc) details, or shortened (or slightly misspelled) name (e.g. Raiffaisen bank or RBA instead of correct Raiffeisen bank).

Nice way to quickly remedy that is Places Overlay, where one just searches for correct brand in NSI, and all the ideal data (correct most popular name, brand, wikidata etc.) is filled in automatically!

That works great when existing amenity does not have much extra details, e.g. it only has amenity=bank (and maybe name=*).

However, if amenity had extra details (e.g. amenity=bank + name=RBA + opening_hours + wheelchair + drive_through + bic) when NSI for "Raiffeisen bank" is selected, all those extra details (opening_hours, wheelchair, ...) are removed without question, which makes using NSI for such improvements unusable (as it breaks more stuff than it fixes)

It seems to be because shouldAlwaysReplaceShop gets triggered.

(Inspired by Helium314#670 (comment))

Proposed Solution

I would suggest that in such cases when selected NSI does not change the type of the feature, user should be presented with confirmReplaceShop() dialog instead (that should happen automatically if shouldAlwaysReplaceShop is false), thus allowing them to choose to preserve all those pre-existing details like opening_hours etc.

Furher refinement to slightly reduce effort for mapper (but not strictly necessary) would be to set shouldNotReplaceShop instead if both the feature type and the name remain the same, which would automatically preserve pre-existing data without even asking the user.

@westnordost
Copy link
Member

westnordost commented Oct 2, 2024

The issue that lead to the current behavior of replacing the feature even when the name was not changed: #5195


Anyway, your suggestion sounds reasonable.

So the check would go

(isFeatureWithName || wasFeatureWithName) && hasChangedFeatureType

Do you want to implement this yourself?

@mnalis mnalis self-assigned this Oct 2, 2024
@westnordost
Copy link
Member

westnordost commented Oct 2, 2024

Ah I see you self-assigned this, cool!

A few hints:

  • Feature.isSuggestion denotes whether it is a brand feature or not (e.g. Raiffeisen bank, ...)
  • a Feature doesn't know it's parent (Raiffeisen bank -> Bank), but it does have an id, which is basically a path. E.g. for "Raiffeisen Bank (Croatia)", we have as id "amenity/atm/raiffeisenbank-4d8c03". Just look at the presets.json.
  • String.substringBeforeLast might be handy
  • in the overlay form, you have access to the featureDictionary (it is defined in AbstractOverlayForm)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants