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

"This shop has been vacant. What's here now" - "house" is not a valid answer. #3774

Closed
SomeoneElseOSM opened this issue Feb 17, 2022 · 26 comments
Assignees

Comments

@SomeoneElseOSM
Copy link

How to Reproduce

At this location:
https://www.openstreetmap.org/node/850026531
The "This shop has been vacant. What's here now" question appears.
Type in "house" as the answer (it's in the suggestions list)
Tap OK
A message "not recognised" is then shown.

Versions affected

Android 10 and 12, SC version v39.1.

@peternewman
Copy link
Collaborator

I think I'm seeing something similar for this:
https://www.openstreetmap.org/way/426232580

Using 40.0-beta1, although I get no suggestions whatsoever regardless of what I type.

My assumption in my case, and possibly both, is that the disused aspect of the node/way is impacting on the suggestions/tagging.

Apologies if they're entirely unrelated issues, but it's hard to tell with the different versions too.

@westnordost
Copy link
Member

It is what it is.

You need to leave a note instead in this case.

@peternewman
Copy link
Collaborator

Is my problem the same as @SomeoneElseOSM then @westnordost or entirely unrelated in which case I'll open a new issue for it?

In @SomeoneElseOSM case, can't you at least hide the inapplicable ones from the suggestion list via the same method that says it's not recognised?

@westnordost
Copy link
Member

No its not related. First check if the issue persists with v40.1 before you open a ticket

@SomeoneElseOSM
Copy link
Author

It is what it is.
You need to leave a note instead in this case.

I don't want to leave a note, since I know exactly what tag changes are needed - I'll hop over into another editor to make those changes. I was just hoping to be able to do that directly in StreetComplete.

@Helium314
Copy link
Collaborator

Out of curiosity: Why does the app suggest House if the answer is not recognised?

@westnordost
Copy link
Member

Hmm... good question

@westnordost westnordost reopened this Feb 18, 2022
@westnordost westnordost self-assigned this Feb 18, 2022
@westnordost
Copy link
Member

westnordost commented Feb 18, 2022

"House" is a clothing store brand: https://nsi.guide/index.html?t=brands&k=shop&v=clothes&tt=house#house-3937bd

@westnordost
Copy link
Member

And there is a bug that if the selected name is not the first one in the list of suggestions (when typing that exact name), the form would claim that it doesn't know that preset.

@mnalis
Copy link
Member

mnalis commented Feb 18, 2022

I'm not sure how complicated is this, but perhaps we should skip such NSI brands which conflict with more regular usage (i.e. tag descriptions)

Or at least show NSI brands under quotes to indicate they relate to names of the brands, e.g. "House" vs. house?

@SomeoneElseOSM
Copy link
Author

"House" is a clothing store brand:

... in Poland and some other countries:

https://www.housebrand.com/special/store/

but that appears to be an upstream issue:

https://github.com/osmlab/name-suggestion-index/blob/main/data/brands/shop/clothes.json#L3309

(I'd have expected the "include" list there to be a list of valid countries(

@westnordost
Copy link
Member

Sure, to make clear that something is a brand somehow would be an improvement. Though, it is not trivial to do.

@matkoniecz
Copy link
Member

I opened osmlab/name-suggestion-index#6205 upstream to partially limit confusion

@matkoniecz
Copy link
Member

Sure, to make clear that something is a brand somehow would be an improvement. Though, it is not trivial to do.

What about doing it similar to Vespucci and list

House (clothing shop)

not

House

in the list?

As bonus it may allow to disambiguate between various objects of the same brand. For example Carrefour is running both supermarkets and fuel stations what may be also source of confusion.

Or show object type after it is selected below text field?

@westnordost
Copy link
Member

Sounds reasonable, you could try that

@westnordost westnordost reopened this May 24, 2022
@matkoniecz matkoniecz assigned matkoniecz and unassigned westnordost May 24, 2022
@matkoniecz
Copy link
Member

@westnordost @FloEdelmann (pinging as I want to bother you about feedback on code design here)

I looked into it and there is one challenge:

displaying something like "House (Clothing Shop)" is easy: make getFeatureName accessible and use context.resources.getFeatureName(feature.tags, featureDictionary)

The tricky part is how given this text one may get correct Feature back?

Searching House (Clothing Shop) with https://github.com/streetcomplete/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/quests/shop_type/ShopGoneDialog.kt#L106 will now fail, as this extra (Clothing Shop) is not part of actual preset name.

Ideas:

(1) hacky solution, just remove everything in brackets before searching for original feature. Would work for now and break as soon as brand with brackets in name would appear. And be a blatant hack.

(2) somehow associate Feature with its label in AutoCompleteTextView an then just get correct object, no need for searching it again. Store all shown elements that appeared in convertResultToString and their text representation and allow conversion back? That feels wrong to me, but may be a good solution.

(3) modify FeatureDictionary to allow both presentation and search by "House (Clothing Shop)"? And use that new format in dialog, without substantial code modification there. Seems best solution even if annoying to implement (as I would need to test with a new dependency)

(4) some other smart solution that I am missing

@peternewman
Copy link
Collaborator

(2) somehow associate Feature with its label in AutoCompleteTextView an then just get correct object, no need for searching it again. Store all shown elements that appeared in convertResultToString and their text representation and allow conversion back? That feels wrong to me, but may be a good solution.

(4) some other smart solution that I am missing

I don't know if this is two or four, but if this was a HTML select, you'd have the value and the name independently. I appreciate it's a bit more complicated than that, as it's an AutoComplete, but it seems odd there's no equivalent magic function already.

It looks like you can get the position in the list and find it that way (entirely untested by me, but the theory seems sound):
https://stackoverflow.com/questions/56319958/autocompletetextview-get-id-when-select-name
https://stackoverflow.com/questions/33094482/how-to-get-id-of-selected-item-in-autocompletetextview-in-android

@westnordost
Copy link
Member

I see, so in my view, getSelectedFeature is the culprit.

The SearchAdapter is simply filled with a number of strings and clicking on them fills the associated edit text. Then, the actual feature selected is determined by again finding which features match for the text in the input text. This is indeed not very clean.

The cleanest solution would be the following:

  1. Let the search adapter not display a number of plain strings but a number of items that consist of a display string plus a value (i.e. the Feature) (the same way any RecyclerView.Adapter does)
  2. When one from the list is selected, it is memorized which Feature the user selected from the list (and only cleared if he changes back into the edit text and changes the text
  3. thus, when clicking on "OK", if the user selected a feature from the list, use that one. If he didn't (but only typed one), search for a fitting feature like now. Optionally, one could disallow NOT selecting an item from the list alltogether.

This solution enables us to also customize how the items in the result list look, e.g. we are not limited to a string ("House (Clothing Store)") but the items could be displayed e.g. like this:

👚House
Clothing Store

The unknown here is how flexible the SearchAdapter/built-in search functionality is. Worst case, one cannot use the Android components but has to listen to text changed events on the edit text oneself and display a floating popup menu with a recyclerview.adapter.


The next best solution is a subset of the above: Maybe the SearchAdapter allows a callback to be called when something is selected. At that point, it could be memorized that that feature has been selected and that feature is used when tapping on "OK" unless the user further edits the text field.

@matkoniecz
Copy link
Member

but the items could be displayed e.g. like this:

My first response was that is likely not the best idea due to limited space. But it is partially caused by this form starting in middle of the screen and leaving most of space unused...

@westnordost
Copy link
Member

westnordost commented Jun 15, 2022

Point is that if it is not just a string, it could be arranged in any form and also include icons or text formatting, e.g.

👚House Clothing Store
should not take up more vertical space.

@westnordost
Copy link
Member

I'll do this, with the icons and all

@SomeoneElseOSM
Copy link
Author

Thanks.

I wonder (and this may well be a separate task) whether it would be possible to capture "not a shop any more; not even a disused one" as a possible answer?

@westnordost
Copy link
Member

And remove the node completely then? I thought about this, but I think in any case it is better to leave a note. Because it is not always clear that indeed the node should be deleted.

It may now be a (company) office, or some other amenity that is simply not selectable in the dialog (because it is a type of place that does not usually rent a shop-space)

@westnordost
Copy link
Member

Device-2022-09-22-144916.mp4

@SomeoneElseOSM
Copy link
Author

... but I think in any case it is better to leave a note.

Leaving a note wouldn't make sense in my example - I'd only be leaving a note for myself!

Right now I have to switch over to Vespucci to fix it.

@matkoniecz
Copy link
Member

matkoniecz commented Sep 27, 2022

Leaving a note wouldn't make sense in my example - I'd only be leaving a note for myself!

It may make sense to do this, for example in case where I am unable to edit right now. Or for people disliking Vespucci.

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

No branches or pull requests

6 participants