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

Add drive-through information to place page and iOS editor #7835

Merged
merged 5 commits into from
Apr 19, 2024

Conversation

dvdmrtnz
Copy link
Contributor

@dvdmrtnz dvdmrtnz commented Apr 6, 2024

Add drive-through information to place page

@dvdmrtnz dvdmrtnz requested a review from a team as a code owner April 6, 2024 13:29
@dvdmrtnz dvdmrtnz requested a review from a team April 6, 2024 14:51
Copy link
Contributor

@NeatNit NeatNit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no Hebrew term for this. The Wikipedia article uses a transliteration: דרייב ת'רו, which is better than nothing I guess, but makes me want to puke... Wish I could just leave this blank.

That goes double for no drive-through. Can't we let users assume there's no drive-through unless specified?

data/strings/strings.txt Show resolved Hide resolved
data/strings/strings.txt Outdated Show resolved Hide resolved
Copy link

@Misalf-git Misalf-git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

de suggestions

data/strings/strings.txt Show resolved Hide resolved
data/strings/strings.txt Outdated Show resolved Hide resolved
@Jean-BaptisteC
Copy link
Member

Why not use our custom car icon on Android?

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Apr 6, 2024

Why not use our custom car icon on Android?

What custom car icon?

I took the shop=car icon and added the rearview mirrors to create the icon:

@Misalf-git
Copy link

Can't we let users assume there's no drive-through unless specified?

I second that.
For wheelchair access it's reasonable or even essential to show yes/limited/no.
For drivers I'd say it's "nice to know" if there is a Drive-Through.

@matheusgomesms
Copy link
Contributor

We have to start to think about redesigning the Place Page. It's very useful to have all this info, but when a POI has a lot of details, it gets too cluttered.

Copy link
Contributor

@matheusgomesms matheusgomesms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PT and PT-BR okay.

android/app/src/main/res/xml/locales_config.xml Outdated Show resolved Hide resolved
data/strings/strings.txt Show resolved Hide resolved
@pastk pastk requested a review from biodranik April 7, 2024 11:33
@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Apr 7, 2024

We have to start to think about redesigning the Place Page. It's very useful to have all this info, but when a POI has a lot of details, it gets too cluttered.

Related issue:

@biodranik
Copy link
Member

Showing "No drive-through" looks unnecessary and not useful, no?

@muralito
Copy link
Contributor

muralito commented Apr 7, 2024

Showing "No drive-through" looks unnecessary and not useful, no?

Yes, and it is worse in locations when this a rare feature, because waste screen that could be useful for other info.

data/strings/strings.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@billpcs billpcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added Greek suggestions, since we generally use "drive-through", so I just transliterated it.

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Apr 8, 2024

Consensus seems to be that "No drive-through" is unnecessary. I will remove it

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Apr 8, 2024

Added Greek suggestions, since we generally use "drive-through", so I just transliterated it.

@billpcs Where? Did you submit your suggestion?

@dvdmrtnz dvdmrtnz force-pushed the drivethrough branch 2 times, most recently from 4fdf0d5 to 575eb00 Compare April 8, 2024 16:10
@d4f5409d
Copy link
Contributor

d4f5409d commented Apr 9, 2024

@biodranik I'm sure we could find a place for POI editing related dates like in #6811, as we did here ;)

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Apr 13, 2024

Are there many boolean or enum-like types in OpenStreetMap.org that can be displayed in the place page?

drive_through=yes/no
takeaway=yes/no
delivery=yes/no
wheelchair=yes/no/limited
smoking=yes/no/outside
toilets=yes/no
changing_table=yes/no

What and how is stored/supported already in our code?

I don't understand what you are saying here

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Apr 13, 2024

The order of fields is defined in the ios and android code.

Where exactly in the iOS code? I can't find it

@biodranik
Copy link
Member

Do we store drive_through=no too?

Check the order in the viewDidLoad of PlacePageInfoViewController

@dvdmrtnz
Copy link
Contributor Author

Do we store drive_through=no too?

Yes, we store it too. Even if we are not showing it in the place page, it's needed for the editor to be able to show yes/no.

@biodranik
Copy link
Member

biodranik commented Apr 13, 2024

Most of these yes/no cases can be stored in one or two bits of information (considering yes/no/unknown). So your examples above can fit into 7*3 = 21 bits of information (3 bytes).

Compare it with the existing implementation where the data takes from 8*3=24 bytes (for type=no) and even more for longer strings.

@dvdmrtnz
Copy link
Contributor Author

Most of these yes/no cases can be stored in one or two bits of information (considering yes/no/unknown). So your examples above can fit into 7*3 = 21 bits of information (3 bytes).

Compare it with the existing implementation where the data takes from 8*3=24 bytes (for type=no) and even more for longer strings.

I understand. Do any of the other metadata properties use this kind of conversion to bits for optimized storage? Metadata is used all over the app and I'm not sure how to implement this change. If this is implemented somewhere else it would be helpful as an example

@dvdmrtnz
Copy link
Contributor Author

Check the order in the viewDidLoad of PlacePageInfoViewController

That's the Place Page. I am talking about the editor interface.

@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Apr 13, 2024

I've added a tri-state segmented selector in the editor to be able to distinguish empty value from "no". Idea taken from #5724 (comment).



@dvdmrtnz
Copy link
Contributor Author

dvdmrtnz commented Apr 14, 2024

The order of fields in the editor seems to be determined by the Metadata::EType order. I will leave it as is.

vector<MapObject::MetadataID> EditableMapObject::GetEditableProperties() const
{
auto props = m_editableProperties.m_metadata;
if (m_editableProperties.m_cuisine)
{
// Props are already sorted by Metadata::EType value.
auto insertBefore = props.begin();
if (insertBefore != props.end() && *insertBefore == MetadataID::FMD_OPEN_HOURS)
++insertBefore;
props.insert(insertBefore, MetadataID::FMD_CUISINE);
}
return props;
}

@dvdmrtnz
Copy link
Contributor Author

I have added drive_through to the iOS editor. It can be added to the Android editor in a separate PR.

@dvdmrtnz dvdmrtnz marked this pull request as ready for review April 14, 2024 08:34
@dvdmrtnz dvdmrtnz changed the title Add drive-through information to place page Add drive-through information to place page and iOS editor Apr 14, 2024
Copy link
Member

@vng vng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. Prefer to keep 2 commits:

  • all actual changes
  • [strings] Regenerated.

indexer/yes_no_unknown.h Outdated Show resolved Hide resolved
@dvdmrtnz
Copy link
Contributor Author

I like it. Prefer to keep 2 commits:

  • all actual changes
  • [strings] Regenerated.

I actually like to keep some changes separated into different commits (for example iOS vs Android changes and placepage vs editor). I will try to squash some commits but trying to keep logical changes separated if you don’t mind

@vng
Copy link
Member

vng commented Apr 17, 2024

ok

[strings] Add "drive_through"

Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
[ios][editor] Add MWMEditorSegmentedTableViewCell
[core] Add YesNoUnknown enum

Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
Signed-off-by: David Martinez <47610359+dvdmrtnz@users.noreply.github.com>
@dvdmrtnz
Copy link
Contributor Author

Squashed commits

Copy link
Contributor

@d4f5409d d4f5409d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fine.

@vng vng merged commit 93f80f4 into organicmaps:master Apr 19, 2024
16 checks passed
@dvdmrtnz dvdmrtnz deleted the drivethrough branch April 19, 2024 06:03
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

Successfully merging this pull request may close these issues.

Highlight that drive through is available for a POI