search

FossifyOrg / Contacts

Easy and quick contact management with no ads, handles groups and favorites too.
https://www.fossify.org
GNU General Public License v3.0
269 stars 23 forks source link

Formatting the phone number with user's region. #72

Closed theoUniv closed 5 months ago

theoUniv commented 6 months ago

Closes https://github.com/FossifyOrg/Contacts/issues/15

What is it?

Description of the changes in your PR

Before/After Screenshots/Screen Record

Acknowledgement

Netpilgrim commented 6 months ago

If I read this pull request correctly, the formatting of the phone number would always be done according to some Android specification tied to the country of the user’s locale without the user being able to either opt in or out. I am strongly against adopting this because it takes away control from the user. A majority of users might be fine with the result but many do not want all the formatting rules that come with their set locale, en_US (English, US) is especially prevalent internationally with users that want to use the language but not necessarily other rules tied to the country. This is a common problem with date and time formatting and should not be carried into other areas as well.

theoUniv commented 6 months ago

If I read this pull request correctly, the formatting of the phone number would always be done according to some Android specification tied to the country of the user’s locale without the user being able to either opt in or out. I am strongly against adopting this because it takes away control from the user. A majority of users might be fine with the result but many do not want all the formatting rules that come with their set locale, en_US (English, US) is especially prevalent internationally with users that want to use the language but not necessarily other rules tied to the country. This is a common problem with date and time formatting and should not be carried into other areas as well.

And what do you think about this thing: The user can choose to enable the phone number formatting by checking a checkbox in settings like that:

image
Netpilgrim commented 6 months ago

And what do you think about this thing: The user can choose to enable the phone number formatting by checking a checkbox in settings like that: […]

Anything that’s optional is fine. Many users will be glad to have it.

theoUniv commented 6 months ago

And what do you think about this thing: The user can choose to enable the phone number formatting by checking a checkbox in settings like that: […]

Anything that’s optional is fine. Many users will be glad to have it.

We just add a commit to make it.

Aga-C commented 6 months ago

If you add something to settings, the setting should be saved and persisted after closing the app. Now after I close the app and open it again, formatting setting is always set to true.

Aga-C commented 6 months ago

Also, the number is not formatted on the Contacts tab, only in contact details. It should be formatted in both places.

theoUniv commented 6 months ago

Also, the number is not formatted on the Contacts tab, only in contact details. It should be formatted in both places.

How we can modify the BaseConfig file ? because it is read-only.

Aga-C commented 6 months ago

Also, the number is not formatted on the Contacts tab, only in contact details. It should be formatted in both places.

How we can modify the BaseConfig file ? because it is read-only.

Use org/fossify/contacts/helpers/Config.kt.

theoUniv commented 6 months ago

Do you mean this thing by contact Tab ? We commited the modification about saving the user's choice in the config file.

image
Aga-C commented 6 months ago

Do you mean this thing by contact Tab ?

Check the Show phone numbers setting and see this tab again.

theoUniv commented 6 months ago

Do you mean this thing by contact Tab ?

Check the Show phone numbers setting and see this tab again.

Done :)

theoUniv commented 6 months ago

Any other requierement to make this merge request accepted ?

Aga-C commented 6 months ago

It's all from my side. Now you have to wait for final approval from Naveen.

seankhl commented 6 months ago

@theoUniv I just wanted to say that I appreciate this contribution!

I just looked at the code and realized it always uses the default locale to format the phone numbers. This makes a great deal of sense for numbers without a country code, but how about formatting numbers with a country code using the formatting for the corresponding country?

min7-i commented 6 months ago

Regarding the questions about how the numbers are actually formatted, this is from the Android documentation of the formatNumber function used:

If the given number doesn't have the country code, the phone will be formatted to the default country's convention.

I tested the changes made in this PR locally and can confirm that it works that way. If the number starts with a country code, e.g. +1 or +49, the number is formatted according to the country's formatting rules. Otherwise the number is formatted according to the app's language/locale; probably according to the system language on older Android versions.

Here's a comparison of the number format depending on the app's language: See the difference in the formatting for the contacts with the suffix "Local". | English (US) | system default (German) | | --- | --- | | | |
seankhl commented 6 months ago

That looks great! Thanks for answering!

theoUniv commented 5 months ago

@naveensingh, Did you had the time to see our contribution ? In case the contribution seems good to you, can we have your point of view about it ?

Thanks.

naveensingh commented 5 months ago

@theoUniv I haven't had the time but I'll check it soon. Probably tomorrow.

naveensingh commented 5 months ago

@theoUniv please link the issue you are working on by adding "Closes https://github.com/FossifyOrg/Contacts/issues/15" to your description.

theoUniv commented 5 months ago

@theoUniv please link the issue you are working on by adding "Closes #15" to your description.

Done ! Is this good for you ?

naveensingh commented 5 months ago

@theoUniv check my comments above.

theoUniv commented 5 months ago

@theoUniv check my comments above.

Thanks for giving us your POV, I just commited the changes !

naveensingh commented 5 months ago

@theoUniv looks good, thanks :)

RokeJulianLockhart commented 5 months ago

https://github.com/FossifyOrg/Contacts/pull/72#event-12244378837

This PR doesn't modify how they're stored, right - merely displayed? I ask because store mine in a format conforming to https://datatracker.ietf.org/doc/html/rfc3966#section-3, and consequently don't want spaces introduced.

naveensingh commented 4 months ago

This PR doesn't modify how they're stored, right - merely displayed?

@RokeJulianLockhart yes.