GH-25: Add PhoneDialer API #37

f1nzer · 2018-02-27T04:42:06Z

Will be updated according to related discussions.

PR Checklist

Has tests (if omitted, state reason in description)
Rebased on top of master at time of PR
Changes adhere to coding standard
Consolidate commits as makes sense
Has Samples

mattleibow

Thanks for this PR, it is good so far, but I think we need to change the API a bit (I updated the spec issue).

I think we are not going to support the name parameter for v1, as this is not really supported across the board. Also, I think we should throw if the dialer is not supported rather than just quietly doing nothing.

For whitespace, until we merge all the rules, just make sure that you are using tabs everywhere.

mattleibow · 2018-02-27T05:07:55Z

Caboodle/PhoneDialer/PhoneDialer.android.cs

+				if (Build.VERSION.SdkInt < BuildVersionCodes.Lollipop)
+				{
+#pragma warning disable CS0618
+					phoneNumber = PhoneNumberUtils.FormatNumber(number);


Why is this in an if block? Shouldn't we be formatting this every time, not just for old devices. This method is deprecated, not removed.

mattleibow · 2018-02-27T05:14:38Z

Caboodle/PhoneDialer/PhoneDialer.android.cs

+			Open(number, null);
+		}
+
+		public static void Open(string number, string name)


Because on UWP supports a name, we should probably remove this for v1.

mattleibow · 2018-02-27T05:14:43Z

Caboodle/PhoneDialer/PhoneDialer.ios.cs

+			Open(number, null);
+		}
+
+		public static void Open(string number, string name)


I am thinking for v1 of the API, let's leave name out.

mattleibow · 2018-02-27T05:16:01Z

Caboodle/PhoneDialer/PhoneDialer.android.cs

+				throw new ArgumentNullException(nameof(number));
+			}
+
+            if (IsSupported)


If it is not supported, then throw, rather than doing nothing. Pending discussion here #19

mattleibow · 2018-02-27T05:16:07Z

Caboodle/PhoneDialer/PhoneDialer.ios.cs

+				throw new ArgumentNullException(nameof(number));
+			}
+
+			if (IsSupported)


If it is not supported, then throw, rather than doing nothing. Pending discussion here #19

mattleibow · 2018-02-27T05:18:09Z

Caboodle/PhoneDialer/PhoneDialer.uwp.cs

+			Open(number, null);
+		}
+
+		public static void Open(string number, string name)


~~Leave name out for v1.~~ On second thoughts, leave this here for UWP - let's see what happens to the discussion after this comment: #25 (comment)

mattleibow · 2018-02-27T05:18:27Z

Caboodle/PhoneDialer/PhoneDialer.uwp.cs

+				throw new ArgumentNullException(nameof(number));
+			}
+
+			if (IsSupported)


Throw if not supported. Pending discussion here #19

f1nzer · 2018-02-27T07:00:57Z

I've added temporary exception classes.

mattleibow

Looks good to me.

jamesmontemagno · 2018-02-27T16:21:29Z

The other question I have is what should the user pass in here as it looks like ANdroid does special formatting....

Do they need +1 for instance? does 555.555.5555 work?

jamesmontemagno · 2018-02-27T16:15:26Z

Caboodle/PhoneDialer/PhoneDialer.android.cs

+		{
+			get
+			{
+				var packageManager = Application.Context.PackageManager;


My thoughts here are to also check telephony is actually on the device.

jamesmontemagno · 2018-02-27T16:17:54Z

Caboodle/PhoneDialer/PhoneDialer.ios.cs

+		{
+			get
+			{
+				var nsUrl = CreateNsUrl("0000000000");


shouldn't this append with: tel:

jamesmontemagno · 2018-02-27T16:18:33Z

Caboodle/PhoneDialer/PhoneDialer.ios.cs

+					using (var netInfo = new CTTelephonyNetworkInfo())
+					{
+						var mnc = netInfo.SubscriberCellularProvider?.MobileNetworkCode;
+						return !string.IsNullOrEmpty(mnc) && mnc != "65535"; // 65535 stands for NoNetwordProvider


don't need to check against null here.

All these properties might be null.
https://developer.apple.com/documentation/coretelephony/cttelephonynetworkinfo/1616900-subscribercellularprovider
https://developer.apple.com/documentation/coretelephony/ctcarrier/1620324-mobilenetworkcode

jamesmontemagno · 2018-02-27T16:19:00Z

Caboodle/PhoneDialer/PhoneDialer.ios.cs

+		public static void Open(string number)
+		{
+			if (string.IsNullOrWhiteSpace(number))
+			{


@mattleibow are we standardizing on need or not needing { around a single line if?

@jamesmontemagno I prefer always using braces for the simple reason that it avoids confusion due to the grouping. Also it makes changes easier - if we ever have to add another line, then we don't have to also add braces. But, this is a very opinionated thing.

If we are following the .net foundation guides, then they say that we don't need it. So I guess we don't need them.
https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md

Then again, we don't really follow most of them.

jamesmontemagno · 2018-02-27T16:19:43Z

Caboodle/PhoneDialer/PhoneDialer.ios.cs

+			UIApplication.SharedApplication.OpenUrl(nsUrl);
+		}
+
+		private NSUrl CreateNsUrl(string number)


Should use expression bodied method here.

jamesmontemagno · 2018-02-27T16:20:02Z

Caboodle/PhoneDialer/PhoneDialer.uwp.cs

+		public static bool IsSupported =>
+			 Windows.Foundation.Metadata.ApiInformation.IsTypePresent("Windows.ApplicationModel.Calls.PhoneCallManager");
+
+		public static void Open(string number)


Expression bodied Method here

jamesmontemagno · 2018-02-27T16:20:25Z

Caboodle/PhoneDialer/PhoneDialer.ios.cs

+
+				if (canCall)
+				{
+					using (var netInfo = new CTTelephonyNetworkInfo())


What all is this check doing?

we are checking for SIM-card & network availability

…tNewActivity does not exist.

mattleibow · 2018-03-01T13:38:43Z

@f1nzer to save you time merging and resolving conflicts, I did it here: f1nzer#1

NOTE: I left your files as tabs so that it (hopefully) won't break any local changes you are working on.

[Dialer] Merge from master

jamesmontemagno · 2018-03-07T16:37:31Z

We now have checks in place for StyleCop + Unit Tests + Samples ready to go.

Are you able to ensure everything pulls up from master and validates against StyleCop. Additionally can you add Unit Tests, Tests for Device Runners, and Samples.

jamesmontemagno · 2018-03-17T04:31:04Z

I think the only thing we are trying to figure out is if we want "IsSupported" or some enum there.

jamesmontemagno · 2018-03-17T04:31:45Z

Samples/Caboodle.Samples/View/PhoneDialerPage.xaml.cs

+
+namespace Caboodle.Samples.View
+{
+    [XamlCompilation(XamlCompilationOptions.Compile)]


We can remove this as it is on the assembly level

jamesmontemagno · 2018-03-23T17:16:17Z

Pulling into a branch so I can work on it

* GH-25: Add PhoneDialer API (#37) * added phone dialer api * fixes according to comments * Merge branch 'master' into feature/phone-dialer * Added the UWP SDK references and fix the Android intent logic as StartNewActivity does not exist. * fixed style errors * added phone dialer sample * added phone dialer tests * Cleanup exceptions from merge * Cleanup dial API and is supported * Cleanup dialers is supported * Cleanup tests and samples. * Add phone dialer docs. * Cleanup UWP * Add remarks * Changes based on feedback to cleanup samples. * Add platform helpers.

mattleibow added enhancement DO-NOT-MERGE Don't merge it.... don't do it! labels Feb 27, 2018

mattleibow added this to the Preview-1 milestone Feb 27, 2018

mattleibow suggested changes Feb 27, 2018

View reviewed changes

added phone dialer api

51addd5

f1nzer force-pushed the feature/phone-dialer branch from fcd57de to 51addd5 Compare February 27, 2018 06:58

mattleibow previously approved these changes Feb 27, 2018

View reviewed changes

jamesmontemagno suggested changes Feb 27, 2018

View reviewed changes

fixes according to comments

c06e2bf

f1nzer force-pushed the feature/phone-dialer branch from a3482df to c06e2bf Compare February 28, 2018 02:52

mattleibow added 2 commits March 1, 2018 15:20

Merge branch 'master' into feature/phone-dialer

3f899e7

Added the UWP SDK references and fix the Android intent logic as Star…

11adb19

…tNewActivity does not exist.

mattleibow and others added 4 commits March 1, 2018 15:42

Merge branch 'master' into dialer-merge

5435b35

Merge pull request #1 from mattleibow/dialer-merge

45913cf

[Dialer] Merge from master

Merge branch 'master' into feature/phone-dialer

67ea577

fixed style errors

6cf6e25

Redth added this to PR Available in Triage Mar 7, 2018

Redth added this to Ready in v0.5.0-preview Mar 7, 2018

Redth moved this from Ready to In Progress in v0.5.0-preview Mar 7, 2018

f1nzer added 2 commits March 8, 2018 18:40

added phone dialer sample

2155fbe

added phone dialer tests

3c2f15a

Redth removed this from PR Available in Triage Mar 10, 2018

jamesmontemagno changed the title ~~[WIP] PhoneDialer API~~ GH-25: Add PhoneDialer API Mar 12, 2018

Redth removed this from In Progress in v0.5.0-preview Mar 12, 2018

Redth removed the enhancement label Mar 12, 2018

Merge branch 'master' into feature/phone-dialer

1d6f0ea

jamesmontemagno dismissed mattleibow’s stale review via 1d6f0ea March 17, 2018 04:31

jamesmontemagno reviewed Mar 23, 2018

View reviewed changes

Merge branch 'master' into feature/phone-dialer

5cf43ee

jamesmontemagno changed the base branch from master to feature/phone-dialer March 23, 2018 17:15

jamesmontemagno merged commit 6061828 into xamarin:feature/phone-dialer Mar 23, 2018

mattleibow mentioned this pull request Mar 23, 2018

GH-25: Add PhoneDialer API #109

Merged

5 tasks

mattleibow added this to Done in v0.5.0-preview Mar 28, 2018

johanlangevillecab mentioned this pull request Feb 2, 2022

[Bug] WebAuthenticationCallbackActivity missing android:exported #1963

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-25: Add PhoneDialer API #37

GH-25: Add PhoneDialer API #37

f1nzer commented Feb 27, 2018 •

edited

mattleibow left a comment •

edited

mattleibow Feb 27, 2018

mattleibow Feb 27, 2018

mattleibow Feb 27, 2018

mattleibow Feb 27, 2018

mattleibow Feb 27, 2018

mattleibow Feb 27, 2018 •

edited

mattleibow Feb 27, 2018

f1nzer commented Feb 27, 2018

mattleibow left a comment

jamesmontemagno commented Feb 27, 2018

jamesmontemagno Feb 27, 2018

jamesmontemagno Feb 27, 2018

jamesmontemagno Feb 27, 2018

f1nzer Feb 28, 2018

jamesmontemagno Feb 27, 2018

mattleibow Feb 27, 2018

mattleibow Feb 27, 2018

jamesmontemagno Feb 27, 2018

jamesmontemagno Feb 27, 2018

jamesmontemagno Feb 27, 2018

f1nzer Feb 28, 2018

mattleibow commented Mar 1, 2018

jamesmontemagno commented Mar 7, 2018

jamesmontemagno commented Mar 17, 2018

jamesmontemagno Mar 17, 2018

jamesmontemagno commented Mar 23, 2018

GH-25: Add PhoneDialer API #37

GH-25: Add PhoneDialer API #37

Conversation

f1nzer commented Feb 27, 2018 • edited

PR Checklist

mattleibow left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattleibow Feb 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1nzer commented Feb 27, 2018

mattleibow left a comment

Choose a reason for hiding this comment

jamesmontemagno commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattleibow commented Mar 1, 2018

jamesmontemagno commented Mar 7, 2018

jamesmontemagno commented Mar 17, 2018

Choose a reason for hiding this comment

jamesmontemagno commented Mar 23, 2018

f1nzer commented Feb 27, 2018 •

edited

mattleibow left a comment •

edited

mattleibow Feb 27, 2018 •

edited