Skip to content
This repository has been archived by the owner on May 15, 2024. It is now read-only.

GH-26: Add SMS API #91

Merged
merged 8 commits into from
Mar 21, 2018
Merged

GH-26: Add SMS API #91

merged 8 commits into from
Mar 21, 2018

Conversation

mattleibow
Copy link
Contributor

@mattleibow mattleibow commented Mar 15, 2018

Description of Change

Adding the SMS API as designed in #26.

Bugs Fixed

Provide links to issues here. Ensure that a GitHub issue was created for your feature or bug fix before sending PR.

API Changes

Adding the SMS API as designed in #26.

Behavioral Changes

Describe any non-bug related behavioral changes that may change how users app behaves when upgrading to this version of the codebase.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Updated documentation (see walkthrough)

{
public static partial class Sms
{
public static bool IsComposeSupported
Copy link
Member

Choose a reason for hiding this comment

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

I think we still need to decide if a bool and this naming is what we want to end up with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. We may want to have some support level thing: Unsupported, Supported, SupportedWithBackgroundSend or something along those lines. This is more a feature that works when there is a SIM card of some sort.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that probably each class should be small enough that a single Enum should represent what it needs...

So we could introduce "SmsBackground" for instance and that is a new API with a new Enum. I think that is almost better to be honest with you.

Body = message.Body,
Recipients = new[] { message.Recipient }
};
messageController.Finished += (sender, e) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this get triggered no matter what? even if it is canceled? Is it better ot fire and forget?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This appears to be the case. The docs do say we have to explicitly hide the controller in this event and we are provided a result (success, canceled, failed)

{
}

public SmsMessage(string body, string recipient)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be one with just the body? also what is a recipient? phone number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could have overloads for each combo, or we could just let the devs use the properties.
The recipient... each platform just takes a string, so we could leave it as is and hope for the best :)

if (string.IsNullOrWhiteSpace(Body))
throw new ArgumentException("SMS body must not be empty.", nameof(Body));

if (string.IsNullOrWhiteSpace(Recipient))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it can be empty. I think that is fine.


namespace Caboodle.DeviceTests
{
public class Utils
{
public static bool IsiOSSimulator
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be a method that we pass in the device platform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was more just for the tests, as the iOS sim was quite different

Copy link
Collaborator

Choose a reason for hiding this comment

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

ahhh gotcha <3 I see it is devicetests cool.

@jamesmontemagno jamesmontemagno added this to the Preview-1 milestone Mar 16, 2018
@jamesmontemagno jamesmontemagno added enhancement awaiting-review This PR needs to have a set of eyes on it labels Mar 16, 2018
@jamesmontemagno
Copy link
Collaborator

My only feedback here is to allow empty recipients in general. Also... does it hurt if the message is blank? Maybe they just want to launch the sms app?

@Redth
Copy link
Member

Redth commented Mar 20, 2018

  1. Let's have a ComposeAsync() (but please double check we can open sms compose on all platforms with no recipient/body.
  2. Don't require recipient/body.
  3. Make IsComposeSupported internal for now, and check it when we execute ComposeAsync and throw the same exception across platforms if it's unsupported at runtime.

@mattleibow
Copy link
Contributor Author

I think this is done.
I haven't really been able to properly test the Android part - I think my old Android device is a bit messed - I don't appear to have a messaging app... Or a SIM card for it. I could reset and use my SIM, but there are others that are Android users, I think? Is it possible for someone with a real device to test?

@jamesmontemagno jamesmontemagno merged commit 13bb42c into xamarin:master Mar 21, 2018
@mattleibow mattleibow added this to Done in v0.5.0-preview Mar 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting-review This PR needs to have a set of eyes on it
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants