-
Notifications
You must be signed in to change notification settings - Fork 57
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
feat: fix Brazil phone numbers before placing the call #289
base: master
Are you sure you want to change the base?
Conversation
Thanks for contributing, but next time please create an issue or discussion first (as stated in the contribution guidelines). Also, why have you added it as an option? I think it's not needed to be optional, as it only affects wrongly stored/typed Brazilian numbers. |
Sorry for the oversight on procedure, I'll create an issue first next time. As for the option, my thinking was, since it's mostly just relevant for Brazil users, it could be left disabled for everyone else. But I see how it makes sense to be kept under the hood, as it will only affect invalid numbers and not affect anything else anyway. I added a new commit removing the option. I ran the unit tests again and re-did some basic on device testing: Dialing 80000000 from Brazil SIM
Dialing +180000000
|
Is there anything you need from my side? |
Not really, I'll review it later. |
What is it?
Description of the changes in your PR
This PR adds a feature disabled by default that fixes Brazil phone numbers before placing the call.
Why? Brazil added a 9 before all existing mobile numbers a couple years ago. Calls that do not include the "new" leading 9 fail. This is particularly annoying because, besides having to edit all your contacts, a lot of numbers advertised online still miss the 9, so if you click to call, the call fails. This feature recognizes it's a Brazil phone number without the 9 and adapts the number on the fly.
Why just Brazil? The feature is implemented in a generic way such that new country-specific rules can easily be plugged in the future. I think Italy has a similar use case with leading zeroes. If or when new countries are supported the setting text can be changed from "fix Brazil phone numbers" to "fix phone numbers" or something like that.
Why not use a library? I considered using Google's libphonenumber for this, but it does not support formatting Brazil mobile numbers, nor does it allow modifying a
PhoneNumber
, which is necessary for this feature: although we can usesetNationalNumber
to modify aPhoneNumber
andformatInOriginalFormat
to get it back in the original format, the latter falls back to the (unmodified) raw input if the formated phone number differs from it (see here). Since we modified thePhoneNumber
adding the9
, it just removes it back. We could also modify therawInput
but that means dealing with raw strings and if so we don't need the lib (which is what we end up doing). There are other issues with this library that make it not very suitable for our purposes. It would also add a new non-trivial dependency to the project. You can check a branch in my fork with an attempt at using libphonenumber, but some very essential test cases fail. I considered alternatives but did not find any suitable one.I note that a shortcoming of not using
libphonenumber
is that the following numbers do not get fixed:tel:863-1234;phone-context=+1-914-555
)1-800-Flowers
or*555
)I think this is an acceptable trade-off because those are not the numbers we are aiming to fix nor does one usually dial with phone extensions or IDD.
I ran several test cases, everything is inside a
try catch
block, and the feature is disabled by default, so I think it's pretty safe.The project did not have any unit tests, I took the liberty to add some for the new feature, which adds some dev dependencies.
PS: I'm assuming Android API 23+ for
getSystemService
with the class (Android 6+, 97,9% adoption).I'm not a mobile developer so please let me know of any issues and I can amend. I couldn't auto-format the code in vscode, I tried my best to format it manually, feel free to commit a patch if needed.
Besides the unit tests I ran tests on a physical Galaxy A14 with Android 14, see below for details.
Tests and evidence
I tested with real numbers, but for the sake of privacy I re-tested with a fake number.
From contacts
Old number w/o country code
From dialpad
Old number w/o country code
New number w/o country code
Old number w/ country code
Land number
Non supported country
Setting
I tested that with the setting disabled does not change the number.
Before/After Screenshots/Screen Record
Acknowledgement