[Android] Multiple place phones #845

strump · 2021-07-12T14:04:53Z

strump commented

2021-07-12 14:04:53 +00:00

POIs and places in OSM could have more than one phone number. Tag phone allows storing semicolon-separated list of numbers. E.g. https://osm.org/node/2143314850 https://osm.org/node/1693972693
But Organicmaps supports only single phone number field. This PR adds support of multiple phone numbers for a place.

Place details

Before:

After:

Each phone has separate line. Tap to call, long tap to copy.

Call button

Before:

Pressing call button would initiate call to a concatenated phone number.

After:

If more than one phone is avaiilable a popup will appear

Editing

Before:

Single edit text to edit all phone numbers

After:

New phone editing UI

https://user-images.githubusercontent.com/720808/125300069-5d698b00-e332-11eb-80b8-231c724a7222.mp4

TODO

New string "Add phone" needs translation.

Screenshots:

Dark theme:

RTL Layout:

POIs and places in OSM could have more than one phone number. Tag `phone` allows storing semicolon-separated list of numbers. E.g. https://osm.org/node/2143314850 https://osm.org/node/1693972693 But Organicmaps supports only single phone number field. This PR adds support of multiple phone numbers for a place. ## Place details **Before:** <img src="https://user-images.githubusercontent.com/720808/125298832-31013f00-e331-11eb-89f9-8c80eaf51899.png" width="250"/> **After:** <img src="https://user-images.githubusercontent.com/720808/125298857-38284d00-e331-11eb-9fd4-a71999872f15.png" width="250"/> Each phone has separate line. Tap to call, long tap to copy. ## Call button **Before:** Pressing call button would initiate call to a concatenated phone number. **After:** If more than one phone is avaiilable a popup will appear <img src="https://user-images.githubusercontent.com/720808/125299111-7de51580-e331-11eb-8bfa-e3c1b885b22e.png" width="250"/> ## Editing **Before:** Single edit text to edit all phone numbers **After:** New phone editing UI https://user-images.githubusercontent.com/720808/125300069-5d698b00-e332-11eb-80b8-231c724a7222.mp4 ## TODO New string "Add phone" needs translation. ## Screenshots: **Dark theme:** <img src="https://user-images.githubusercontent.com/720808/126669452-06284327-9219-4de6-a5fb-127d1b49f363.png" width="500"/> **RTL Layout:** <img src="https://user-images.githubusercontent.com/720808/126669527-f79a0c07-ab46-4547-a7cb-81f234ad7d62.png" width="500"/>

❤️ 1

Endem10n (Migrated from github.com) requested changes 2021-07-12 15:51:36 +00:00

android/res/layout/fragment_editor.xml

Endem10n (Migrated from github.com) commented

2021-07-12 15:25:48 +00:00

Redundant "android:layout_toLeftOf". Min api level is 21.

Endem10n (Migrated from github.com) commented

2021-07-12 15:26:07 +00:00

Redundant "android:layout_toLeftOf". Min api level is 21.

android/res/layout/item_phone.xml

Endem10n (Migrated from github.com) commented

2021-07-12 15:27:45 +00:00

Redundant tags "android:paddingLeft" and "android:paddingRight". Min api level is 21.

Endem10n (Migrated from github.com) commented

2021-07-12 15:34:40 +00:00

Should change three lines to support RTL. Could you, please, check in other places.

android/res/layout/item_phone.xml

					
				@ -0,0 +14,4 @@

				        style="@style/MwmWidget.Editor.MetadataIcon"

				        tools:ignore="ContentDescription"

				        tools:src="@drawable/ic_phone"

				        android:layout_alignParentLeft="true"/>

Endem10n (Migrated from github.com) commented

2021-07-12 15:32:38 +00:00

Should use "android:layout_alignParentStart"

android/res/layout/place_page_details.xml

Endem10n (Migrated from github.com) commented

2021-07-12 15:50:26 +00:00

nit: As we already will have in this big vertical linear layout duplicating layout items, it would be great, if you will be able to refactor it to RecyclerView.

android/src/com/mapswithme/maps/editor/EditorHostFragment.java

Endem10n (Migrated from github.com) commented

2021-07-12 15:37:53 +00:00

Opening brace should be at the beginning of the next line according to project code style. Could you, please, check in other places.

android/src/com/mapswithme/maps/editor/PhoneListAdapter.java

Endem10n (Migrated from github.com) commented

2021-07-12 15:38:57 +00:00

In case of single line "if" should not use braces according to project code style.

android/src/com/mapswithme/maps/widget/placepage/PlacePageView.java

Endem10n (Migrated from github.com) commented

2021-07-12 15:42:18 +00:00

Should not comment, but just delete.

android/src/com/mapswithme/maps/widget/placepage/PlacePageView.java

Endem10n (Migrated from github.com) commented

2021-07-12 15:30:41 +00:00

Possibly, should not comment, but just delete. It will stay in git history. Could you, please, check in other places.

strump reviewed 2021-07-13 13:44:45 +00:00

android/res/layout/fragment_editor.xml

strump commented

2021-07-13 13:44:45 +00:00

@Endem10n I see that attribute android:layout_toLeftOf is used in fragment_editor.xml on Wi-Fi checkbox row.
Should I replace RelativeLayout with LinearLayout to get rid of android:layout_toLeftOf property?
Or how to correctly layout phone icon, phone number text view, and edit phone buton in a single row?

@Endem10n I see that attribute `android:layout_toLeftOf` is used in fragment_editor.xml on Wi-Fi checkbox row. Should I replace RelativeLayout with LinearLayout to get rid of `android:layout_toLeftOf` property? Or how to correctly layout phone icon, phone number text view, and edit phone buton in a single row?

Endem10n (Migrated from github.com) reviewed 2021-07-14 12:28:25 +00:00

android/res/layout/fragment_editor.xml

Endem10n (Migrated from github.com) commented

2021-07-14 12:28:25 +00:00

@strump my point is that you already have in the next line "android:layout_toStartOf="@+id/edit_phone", so all will work fine even without "android:layout_toLeftOf" at line 161.

strump commented

2021-07-16 14:41:44 +00:00

Added RecyclerView to display multiple phones on place details fragment.
Removed unused commented code. Fixed formatting.
Changed layout to support RTL languages and removed redundant attributes.

Added RecyclerView to display multiple phones on place details fragment. Removed unused commented code. Fixed formatting. Changed layout to support RTL languages and removed redundant attributes.

biodranik (Migrated from github.com) reviewed 2021-07-20 06:24:11 +00:00

android/res/values-nl/strings.xml

					
				@ -598,6 +598,7 @@

					<string name="bookmarks_error_title_list_name_already_taken">Deze naam is al in gebruik</string>

					<string name="bookmarks_error_message_list_name_already_taken">Kies alstublieft een andere naam</string>

					<string name="please_wait">Even geduld aub…</string>

					<string name="phone_number">Telefoonnummer</string>

biodranik (Migrated from github.com) commented

2021-07-20 06:24:11 +00:00

Did you manually add it? I see that these strings were already present in the master branch, in data/strings/strings.txt and in XML android resources.

biodranik (Migrated from github.com) reviewed 2021-07-20 06:37:23 +00:00

biodranik (Migrated from github.com) left a comment

Did you test it for RTL languages?
Did you test dark and light themes?
Did you test it in a large tablet layout?

1. Did you test it for RTL languages? 2. Did you test dark and light themes? 3. Did you test it in a large tablet layout?

android/src/com/mapswithme/maps/editor/EditorFragment.java

biodranik (Migrated from github.com) commented

2021-07-20 06:26:46 +00:00

Why is it commented?

android/src/com/mapswithme/maps/editor/EditorFragment.java

biodranik (Migrated from github.com) commented

2021-07-20 06:27:23 +00:00

Why is it removed? Are phones properly stored and sent to OSM?

android/src/com/mapswithme/maps/editor/PhoneListAdapter.java

biodranik (Migrated from github.com) commented

2021-07-20 06:31:43 +00:00

We format simple ifs in two lines for easier debugging.

biodranik (Migrated from github.com) commented

2021-07-20 06:32:47 +00:00

How to properly define icons from XML, so they will work in both light and dark themes?

biodranik (Migrated from github.com) commented

2021-07-20 06:34:20 +00:00

nit: we insert spaces after slashes.

android/src/com/mapswithme/maps/widget/placepage/PlacePageView.java

biodranik (Migrated from github.com) commented

2021-07-20 06:33:50 +00:00

Missed space after // and the dot at the end.

android/src/com/mapswithme/maps/widget/placepage/PlacePhoneAdapter.java

biodranik (Migrated from github.com) commented

2021-07-20 06:36:19 +00:00

We try not to pollute logs with debug strings (esp. warning ones).

strump reviewed 2021-07-20 07:43:35 +00:00

android/res/values-nl/strings.xml

					
				@ -598,6 +598,7 @@

					<string name="bookmarks_error_title_list_name_already_taken">Deze naam is al in gebruik</string>

					<string name="bookmarks_error_message_list_name_already_taken">Kies alstublieft een andere naam</string>

					<string name="please_wait">Even geduld aub…</string>

					<string name="phone_number">Telefoonnummer</string>

strump commented

2021-07-20 07:43:35 +00:00

Viacheslav Greshilov removed "phone_number" string in commit #cca70039 and regenerated strings.xml in commit #2b46f73d
I restored "phone_number" in strings.xml, but looks like I need to revert data/strings/strings.txt first

Viacheslav Greshilov removed "phone_number" string in commit [#cca70039](https://git.omaps.dev/organicmaps/organicmaps/commit/cca7003972cd540c6f1c7649250eb08fe5abc775) and regenerated strings.xml in commit [#2b46f73d](https://git.omaps.dev/organicmaps/organicmaps/commit/2b46f73d3a8b65cfafe9383eb77bdf97a233f7a9) I restored "phone_number" in strings.xml, but looks like I need to revert data/strings/strings.txt first

strump reviewed 2021-07-20 07:48:02 +00:00

android/src/com/mapswithme/maps/editor/EditorFragment.java

strump commented

2021-07-20 07:48:02 +00:00

I moved Editor.nativeSetPhone(phone); call to EditorHostFragment class simmilar to cuisine, opening house, street editors. Becase I created PhoneFragment to edit multiple phones, this fragment has "Save" button

I moved `Editor.nativeSetPhone(phone);` call to `EditorHostFragment` class simmilar to cuisine, opening house, street editors. Becase I created `PhoneFragment` to edit multiple phones, this fragment has "Save" button

strump reviewed 2021-07-20 07:51:04 +00:00

android/src/com/mapswithme/maps/widget/placepage/PlacePhoneAdapter.java

strump commented

2021-07-20 07:51:04 +00:00

I forgot to remove those logs. It's no use to log this text any more

biodranik (Migrated from github.com) reviewed 2021-07-20 21:59:32 +00:00

android/res/values-nl/strings.xml

					
				@ -598,6 +598,7 @@

					<string name="bookmarks_error_title_list_name_already_taken">Deze naam is al in gebruik</string>

					<string name="bookmarks_error_message_list_name_already_taken">Kies alstublieft een andere naam</string>

					<string name="please_wait">Even geduld aub…</string>

					<string name="phone_number">Telefoonnummer</string>

biodranik (Migrated from github.com) commented

2021-07-20 21:59:32 +00:00

Wow, this string was unused, interesting. Thanks!

vng (Migrated from github.com) approved these changes 2021-07-22 03:42:13 +00:00

vng commented

2021-07-22 03:47:45 +00:00

(Migrated from github.com)

@Endem10n @biodranik Гляньте фиксы, я думаю пора мержить уже.

biodranik (Migrated from github.com) reviewed 2021-07-22 05:42:38 +00:00

data/strings/strings.txt

biodranik (Migrated from github.com) commented

2021-07-22 05:42:38 +00:00

@ma4ko What is the Bulgarian translation?

biodranik (Migrated from github.com) approved these changes 2021-07-22 05:43:31 +00:00

Endem10n (Migrated from github.com) approved these changes 2021-07-22 07:14:31 +00:00

biodranik commented

2021-07-23 07:36:45 +00:00

(Migrated from github.com)

Uups, sorry, looks like conflicts should be resolved. We usually remove all commits with regenerated strings, rebase, and then generate strings again.

rtsisyk approved these changes 2021-07-24 14:49:29 +00:00

rtsisyk left a comment

Thank you for contribution! I don't see any blockers, but please try to address code style comments from other reviewers. This patch will be merged as soon as we fix localization in the master.

rtsisyk commented

2021-07-24 15:20:39 +00:00

@strump, to save time, I squashed and rebased this PR.

See https://github.com/organicmaps/organicmaps/tree/multiple-place-phones

Please address comments from other reviewers and force-push the final version into this PR.

@strump, to save time, I squashed and rebased this PR. See https://github.com/organicmaps/organicmaps/tree/multiple-place-phones Please address comments from other reviewers and force-push the final version into this PR.

biodranik commented

2021-07-24 22:27:04 +00:00

(Migrated from github.com)

Let's polish it afterward to speed up and unblock other tasks with strings/localizations.

👍 1

This repo is archived. You cannot comment on pull requests.

No reviewers

vng

biodranik

Endem10n

rtsisyk