[C] new OnPlatform mechanism #658

Merged
merged 3 commits into from Jan 12, 2017

Conversation

Projects
None yet
@StephaneDelcroix
Member

StephaneDelcroix commented Dec 21, 2016

Description of Change

Implement the change proposed in https://forums.xamarin.com/discussion/84632/redesign-onplatform

C# code should be ported to this:

double width;
switch(Device.RuntimePlatform){
case Device.iOS:
case Device.Android:
    width = 2; break;
case "馃殌":
    width = 4; break;
default:
    width = 0; break;
}

New xaml syntax looks like this:

<OnPlatform x:TypeArguments="x:Double">
	<On Platform="iOS">21</On>
	<On Platform="Android, 馃殌">42</On>
</OnPlatform>

(note: I tested it, it actually works with emojis as platform names. Be creative.)

Bugs Fixed

none. but allow 3rd party platforms

API Changes

Added

public static string RuntimePlatform;

public const string Device.iOS = "iOS";
public const string Device.Android = "Android";
public const string Device.WinPhone = "WinPhone";
public const string Device.Windows = "Windows";

string IPlatformServices.RuntimePlatform { get; }

public IList<On> OnPlatform<T>.Platforms { get; private set; }

public class On
{
	public IList<string> Platform { get; set; }
	public object Value { get; set; }
}

Obsoleted

public enum TargetPlatform;

public static TargetPlatform Device.OS
public static void Device.OnPlatform(Action iOS = null, Action Android = null, Action WinPhone = null, Action Default = null);
public static T Device.OnPlatform<T>(T iOS, T Android, T WinPhone);

public T OnPlatform<T>.Android
public T OnPlatform<T>.iOS
public T OnPlatform<T>.WinPhone

Removed

All Tizen overrides. Not published in any release yet.
Internal Device.OS setter

Behavioral Changes

None, hopefully

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

@StephaneDelcroix StephaneDelcroix changed the title from [C] Obsolete TargetPlatform to [C] new OnPlatform mechanism Dec 22, 2016

StephaneDelcroix added some commits Dec 21, 2016

+ public const string Android = "Android";
+ public const string WinPhone = "WinPhone";
+ public const string Windows = "Windows";
+

This comment has been minimized.

@rookiejava

rookiejava Dec 26, 2016

Contributor

Hello, This is Tizen . Please don't forget me.

public const string Tizen = "Tizen";

@rookiejava

rookiejava Dec 26, 2016

Contributor

Hello, This is Tizen . Please don't forget me.

public const string Tizen = "Tizen";

@samhouts

This comment has been minimized.

Show comment
Hide comment
@samhouts

samhouts Dec 28, 2016

Member

I may have missed it, but it doesn't look like we're testing the backcompat piece anywhere. All of the tests are updated to use the new syntax, so we don't have any tests that confirm backwards compatibility.

Member

samhouts commented Dec 28, 2016

I may have missed it, but it doesn't look like we're testing the backcompat piece anywhere. All of the tests are updated to use the new syntax, so we don't have any tests that confirm backwards compatibility.

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Jan 9, 2017

Member

@samhouts all prior tests using the Xaml <OnPatform> markup are still using the old code path, so it's tested, indirectly

Member

StephaneDelcroix commented Jan 9, 2017

@samhouts all prior tests using the Xaml <OnPatform> markup are still using the old code path, so it's tested, indirectly

@jassmith

This comment has been minimized.

Show comment
Hide comment
@jassmith

jassmith Jan 12, 2017

Member

Tizen will be added in a future update, wont be forgotten before stable release.

Member

jassmith commented Jan 12, 2017

Tizen will be added in a future update, wont be forgotten before stable release.

@jassmith jassmith merged commit b6cb64e into master Jan 12, 2017

5 of 6 checks passed

iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests failed: 1,鈥
Details
Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 351, i鈥
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3693, ignored: 10
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 344鈥
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 346鈥
Details

@StephaneDelcroix StephaneDelcroix deleted the OnPlatform2 branch Jan 31, 2017

@gtbuchanan

This comment has been minimized.

Show comment
Hide comment
@gtbuchanan

gtbuchanan Mar 3, 2017

I understand the need for this change but was there a particular reason to forgo type-safety in the new On type? I know On<T> would be more verbose in XAML due to the need for x:TypeArguments, but I'd almost prefer the verbosity in this case (the new approach is already way more verbose than it was anyway). A couple of things I've already run into:

Previously implicit casts now become runtime errors. In this example, the previous code worked (new OnPlatform<double> { iOS = 35, Android = 32 }) but the new code fails at runtime without adding an explicit d.
image

VS color feature breaks (the green is from ReSpeller)
image

I imagine previous compile-time errors from type mismatches also become runtime errors now as well.

I understand the need for this change but was there a particular reason to forgo type-safety in the new On type? I know On<T> would be more verbose in XAML due to the need for x:TypeArguments, but I'd almost prefer the verbosity in this case (the new approach is already way more verbose than it was anyway). A couple of things I've already run into:

Previously implicit casts now become runtime errors. In this example, the previous code worked (new OnPlatform<double> { iOS = 35, Android = 32 }) but the new code fails at runtime without adding an explicit d.
image

VS color feature breaks (the green is from ReSpeller)
image

I imagine previous compile-time errors from type mismatches also become runtime errors now as well.

@nexussays

This comment has been minimized.

Show comment
Hide comment
@nexussays

nexussays Apr 6, 2017

The loss of type safety and added verbosity seems far worse than simply having a slightly bloated OnPlatform class with Tizen and MacOS and whatever else would need to be added. Really disliking this change.

The loss of type safety and added verbosity seems far worse than simply having a slightly bloated OnPlatform class with Tizen and MacOS and whatever else would need to be added. Really disliking this change.

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Apr 6, 2017

Member

@gtbuchanan we went for non-generic On to avoid a too verbose Xaml. We could have avoided creating the On class, and replacing the Platforms property by something likeDictionary<IList<string>, T>, but unfortunately there's no nice Xaml syntax for creating Dictionaries in XAML.

As for the conversion issue, please file an issue and I'll look at it

Member

StephaneDelcroix commented Apr 6, 2017

@gtbuchanan we went for non-generic On to avoid a too verbose Xaml. We could have avoided creating the On class, and replacing the Platforms property by something likeDictionary<IList<string>, T>, but unfortunately there's no nice Xaml syntax for creating Dictionaries in XAML.

As for the conversion issue, please file an issue and I'll look at it

@appbureauet

This comment has been minimized.

Show comment
Hide comment
@appbureauet

appbureauet Apr 6, 2017

I agree that the loss of type safety seems to be a step in the wrong direction.
Have a look at the code I used before when accessing a platform specific ressource from code:

var menuButtonSize = (OnPlatform<double>)Application.Current.Resources["MenuButtonSize"];
return new GridLength(Device.OS == TargetPlatform.iOS ? menuButtonSize.iOS : menuButtonSize.Android);

And look at the amount of type casts I have to do now (please correct me if this can be done in a better way):

var menuButtonSize = (OnPlatform<double>)Application.Current.Resources["MenuButtonSize"];
var v = menuButtonSize.Platforms.First(p => p.Platform.First().Equals(Device.RuntimePlatform)).Value;
return new GridLength(double.Parse((string)v));

Is this intended? It doesn't feel right writing this code.

The xaml is also slightly more verbose now, it has gone from this:

<OnPlatform x:TypeArguments="x:Double" iOS="40" Android="60" x:Key="MenuButtonSize" />

to this:

<OnPlatform x:TypeArguments="x:Double" x:Key="MenuButtonSize">
  <On Platform="iOS">40</On>
  <On Platform="Android">60</On>
</OnPlatform>

I don't mind the slightly more verbose xaml, but I don't really like the loss of type safety.

appbureauet commented Apr 6, 2017

I agree that the loss of type safety seems to be a step in the wrong direction.
Have a look at the code I used before when accessing a platform specific ressource from code:

var menuButtonSize = (OnPlatform<double>)Application.Current.Resources["MenuButtonSize"];
return new GridLength(Device.OS == TargetPlatform.iOS ? menuButtonSize.iOS : menuButtonSize.Android);

And look at the amount of type casts I have to do now (please correct me if this can be done in a better way):

var menuButtonSize = (OnPlatform<double>)Application.Current.Resources["MenuButtonSize"];
var v = menuButtonSize.Platforms.First(p => p.Platform.First().Equals(Device.RuntimePlatform)).Value;
return new GridLength(double.Parse((string)v));

Is this intended? It doesn't feel right writing this code.

The xaml is also slightly more verbose now, it has gone from this:

<OnPlatform x:TypeArguments="x:Double" iOS="40" Android="60" x:Key="MenuButtonSize" />

to this:

<OnPlatform x:TypeArguments="x:Double" x:Key="MenuButtonSize">
  <On Platform="iOS">40</On>
  <On Platform="Android">60</On>
</OnPlatform>

I don't mind the slightly more verbose xaml, but I don't really like the loss of type safety.

@fuse314

This comment has been minimized.

Show comment
Hide comment
@fuse314

fuse314 Apr 6, 2017

@appbureauet : this works fine for me:
var menuButtonSize = (double)(OnPlatform<double>)Application.Current.Resources["MenuButtonSize"]; return new GridLength(menuButtonSize);

fuse314 commented Apr 6, 2017

@appbureauet : this works fine for me:
var menuButtonSize = (double)(OnPlatform<double>)Application.Current.Resources["MenuButtonSize"]; return new GridLength(menuButtonSize);

@appbureauet

This comment has been minimized.

Show comment
Hide comment
@appbureauet

appbureauet Apr 6, 2017

@fuse314 : thanks that works!
Now I can cut it down to:

new GridLength((OnPlatform<double>)Application.Current.Resources["MenuButtonSize"]);

Just not quite sure why it works?

@fuse314 : thanks that works!
Now I can cut it down to:

new GridLength((OnPlatform<double>)Application.Current.Resources["MenuButtonSize"]);

Just not quite sure why it works?

@akamud

This comment has been minimized.

Show comment
Hide comment
@akamud

akamud Apr 6, 2017

@appbureauet it works because OnPlatform<T> has an implicit operator.

akamud commented Apr 6, 2017

@appbureauet it works because OnPlatform<T> has an implicit operator.

@appbureauet

This comment has been minimized.

Show comment
Hide comment
@appbureauet

appbureauet Apr 6, 2017

@akamud thank you - that makes sense.
The implicit operator fulfils my needs, then I don't have any problems with the changes anyway.

@akamud thank you - that makes sense.
The implicit operator fulfils my needs, then I don't have any problems with the changes anyway.

@kapitspo

This comment has been minimized.

Show comment
Hide comment
@kapitspo

kapitspo Apr 8, 2017

So how this construction will be looking now?

new Setter { Property = Label.FontSizeProperty, Value = Device.GetNamedSize(NamedSize.Medium, typeof(Label)) * Device.OnPlatform(1.0, 1.0, 0.7) };

kapitspo commented Apr 8, 2017

So how this construction will be looking now?

new Setter { Property = Label.FontSizeProperty, Value = Device.GetNamedSize(NamedSize.Medium, typeof(Label)) * Device.OnPlatform(1.0, 1.0, 0.7) };

@appbureauet

This comment has been minimized.

Show comment
Hide comment
@appbureauet

appbureauet Apr 8, 2017

@kapitspo you could do it like this:

new Setter
{
    Property = Label.FontSizeProperty,
    Value = Device.GetNamedSize(NamedSize.Medium, typeof(Label))
                  * (Device.RuntimePlatform == Device.WinPhone ? 0.7 : 1.0)
};

@kapitspo you could do it like this:

new Setter
{
    Property = Label.FontSizeProperty,
    Value = Device.GetNamedSize(NamedSize.Medium, typeof(Label))
                  * (Device.RuntimePlatform == Device.WinPhone ? 0.7 : 1.0)
};
@kapitspo

This comment has been minimized.

Show comment
Hide comment
@kapitspo

kapitspo Apr 8, 2017

Thanks... Not sure if I like it new way. I have projects where I target all 3 major platforms, and combination of Device.OnPlatform and Device.Idiom did the job for me. Why fixing things which were fit to purpose in 99% of scenarios? At least, please remove "deprecated" warning for now. You've touched one of the sensitive areas in cross-platform design, so I guess it would be wise to give us "old timers" a bit more time to get used to the change. Or challenge it all the way back to where it was.

kapitspo commented Apr 8, 2017

Thanks... Not sure if I like it new way. I have projects where I target all 3 major platforms, and combination of Device.OnPlatform and Device.Idiom did the job for me. Why fixing things which were fit to purpose in 99% of scenarios? At least, please remove "deprecated" warning for now. You've touched one of the sensitive areas in cross-platform design, so I guess it would be wise to give us "old timers" a bit more time to get used to the change. Or challenge it all the way back to where it was.

@thenderson21

This comment has been minimized.

Show comment
Hide comment
@thenderson21

thenderson21 Apr 17, 2017

How would I set a default value in xaml? I have tried:

//This used to work
<ToolbarItem Text="Save" Clicked="Save_Clicked" >
	<ToolbarItem.Text>
		<OnPlatform x:TypeArguments="x:String">
			<On Platform = "iOS" Value="Done" />
		</OnPlatform>
	</ToolbarItem.Text>
</ToolbarItem>

and

<ToolbarItem Clicked="Save_Clicked" >
	<ToolbarItem.Text>
		<OnPlatform x:TypeArguments="x:String">
			<On Platform = "iOS" Value="Done" />
			<On Platform = "Default" Value="Save" />
		</OnPlatform>
	</ToolbarItem.Text>
</ToolbarItem>

But neither work?

thenderson21 commented Apr 17, 2017

How would I set a default value in xaml? I have tried:

//This used to work
<ToolbarItem Text="Save" Clicked="Save_Clicked" >
	<ToolbarItem.Text>
		<OnPlatform x:TypeArguments="x:String">
			<On Platform = "iOS" Value="Done" />
		</OnPlatform>
	</ToolbarItem.Text>
</ToolbarItem>

and

<ToolbarItem Clicked="Save_Clicked" >
	<ToolbarItem.Text>
		<OnPlatform x:TypeArguments="x:String">
			<On Platform = "iOS" Value="Done" />
			<On Platform = "Default" Value="Save" />
		</OnPlatform>
	</ToolbarItem.Text>
</ToolbarItem>

But neither work?

@akamud

This comment has been minimized.

Show comment
Hide comment
@akamud

akamud Apr 18, 2017

Indeed, default doesn't seem to be working.

akamud commented Apr 18, 2017

Indeed, default doesn't seem to be working.

johnjore added a commit to johnjore/Kala that referenced this pull request Apr 19, 2017

@brianlagunas

This comment has been minimized.

Show comment
Hide comment
@brianlagunas

brianlagunas Apr 20, 2017

Collaborator

I'm just curious, but why use magic strings when you can use an Enum?

	public enum RuntimePlatform
	{
		Android,
		iOS,
		Windows, 
		WinPhone
	}
Collaborator

brianlagunas commented Apr 20, 2017

I'm just curious, but why use magic strings when you can use an Enum?

	public enum RuntimePlatform
	{
		Android,
		iOS,
		Windows, 
		WinPhone
	}
@nexussays

This comment has been minimized.

Show comment
Hide comment
@nexussays

nexussays Apr 21, 2017

@brianlagunas According to the original thread the reasoning is:

The current OnPlaform implementation is not easily extendable if we don鈥檛 want it to become an API mess.
Also, it is impossible for a 3rd party Platform creator to implement and use OnPlatform without having our approval.

Personally, I think this solution as implemented is far worse than the stated problem above.

There are a finite number of platforms even into the forseeable future (eg, Tizen, MacOS) and adding a handful of new entries to the enum over time seems eminently reasonable.

Also, for 3rd parties you could just add a "release-valve" Other option to the enum to account for that scenario. I mean realistically how many new supported platforms are in active development between the time Xamarin can add them to the enum?

@brianlagunas According to the original thread the reasoning is:

The current OnPlaform implementation is not easily extendable if we don鈥檛 want it to become an API mess.
Also, it is impossible for a 3rd party Platform creator to implement and use OnPlatform without having our approval.

Personally, I think this solution as implemented is far worse than the stated problem above.

There are a finite number of platforms even into the forseeable future (eg, Tizen, MacOS) and adding a handful of new entries to the enum over time seems eminently reasonable.

Also, for 3rd parties you could just add a "release-valve" Other option to the enum to account for that scenario. I mean realistically how many new supported platforms are in active development between the time Xamarin can add them to the enum?

@dansiegel

This comment has been minimized.

Show comment
Hide comment
@dansiegel

dansiegel Apr 21, 2017

Totally agree there are a finite number of new platforms, and I would say there doesn't need to be a requirement that the Platform implementation should have to be part of the Xamarin Forms repository like Tizen. That said magic string seems more error prone than to simply require Tizen or whoever to submit a PR adding their platform to an OS/RuntimePlatform enum. Think of it like a developer adding their RSS feed to Planet Xamarin. Not to mention somehow we ended up with WinPhone and Windows in 2.3.4 and then completely changing direction and going with WinPhone, UWP, & WinRT in 2.3.5 just to confuse people and break code (and I'm sure we'll completely drop WinPhone in the near future anyway...)

Totally agree there are a finite number of new platforms, and I would say there doesn't need to be a requirement that the Platform implementation should have to be part of the Xamarin Forms repository like Tizen. That said magic string seems more error prone than to simply require Tizen or whoever to submit a PR adding their platform to an OS/RuntimePlatform enum. Think of it like a developer adding their RSS feed to Planet Xamarin. Not to mention somehow we ended up with WinPhone and Windows in 2.3.4 and then completely changing direction and going with WinPhone, UWP, & WinRT in 2.3.5 just to confuse people and break code (and I'm sure we'll completely drop WinPhone in the near future anyway...)

@brianlagunas

This comment has been minimized.

Show comment
Hide comment
@brianlagunas

brianlagunas Apr 21, 2017

Collaborator

I have a feeling that the developer experience was not considered when creating this API. Meaning the API was not used in a production app scenario, but instead created from inside a sandbox. I'm assuming/hoping there is some technical reason why this was a string and not Enum. Otherwise it's just a poor coding practice.

Collaborator

brianlagunas commented Apr 21, 2017

I have a feeling that the developer experience was not considered when creating this API. Meaning the API was not used in a production app scenario, but instead created from inside a sandbox. I'm assuming/hoping there is some technical reason why this was a string and not Enum. Otherwise it's just a poor coding practice.

@renzska

This comment has been minimized.

Show comment
Hide comment
@renzska

renzska Apr 25, 2017

In a scenario like this:

Content = new StackLayout
{
         Padding = new Thickness(0, Device.OnPlatform<int>(20, 0, 0), 0, 0),
         Children = {
             settingsButton,
             logoutButton,
             }
};

What would be the best way to rewrite this given this new API? I'm finding that it requires a switch statement outside of the scenario to store the Thickness property to then apply it in the Content construction:

var tempThickness = new Thickness();

switch (Device.RuntimePlatform)
	{
		case Device.iOS:
			tempThickness = new Thickness(0, 20, 0, 0);
			break;
		default:
			tempThickness = new Thickness(0, 0, 0, 0);
			break;
	}

Content = new StackLayout
{
	Padding = tempThickness,
        Children = {
             settingsButton,
             logoutButton,
        }
};

Or am I just missing something easy? /crossesfingers

renzska commented Apr 25, 2017

In a scenario like this:

Content = new StackLayout
{
         Padding = new Thickness(0, Device.OnPlatform<int>(20, 0, 0), 0, 0),
         Children = {
             settingsButton,
             logoutButton,
             }
};

What would be the best way to rewrite this given this new API? I'm finding that it requires a switch statement outside of the scenario to store the Thickness property to then apply it in the Content construction:

var tempThickness = new Thickness();

switch (Device.RuntimePlatform)
	{
		case Device.iOS:
			tempThickness = new Thickness(0, 20, 0, 0);
			break;
		default:
			tempThickness = new Thickness(0, 0, 0, 0);
			break;
	}

Content = new StackLayout
{
	Padding = tempThickness,
        Children = {
             settingsButton,
             logoutButton,
        }
};

Or am I just missing something easy? /crossesfingers

@brianlagunas

This comment has been minimized.

Show comment
Hide comment
@brianlagunas

brianlagunas Apr 25, 2017

Collaborator

@renzska You just pointed out another major issue with this new API. IMO, it's not very well thought out. Unless I too am missing something obvious.

Collaborator

brianlagunas commented Apr 25, 2017

@renzska You just pointed out another major issue with this new API. IMO, it's not very well thought out. Unless I too am missing something obvious.

@gtbuchanan

This comment has been minimized.

Show comment
Hide comment
@gtbuchanan

gtbuchanan Apr 25, 2017

@renzska I used an IIFE in the first example of my original comment to avoid the intermediary variable. It's not pretty, but you can use a similar solution.

@renzska I used an IIFE in the first example of my original comment to avoid the intermediary variable. It's not pretty, but you can use a similar solution.

@opcodewriter

This comment has been minimized.

Show comment
Hide comment
@opcodewriter

opcodewriter Apr 25, 2017

I don't agree with strings instead of a type and losing type safety. Also, I don't see anywhere mentioned what happens if the platform string is misspelled.
I don't think extending OnPlatform is a real issue. It's not like new platforms are going to appear every day/month/year.
Shouldn't we be more concerned for making Xamarin Forms great at least on iOS and Android?

I don't agree with strings instead of a type and losing type safety. Also, I don't see anywhere mentioned what happens if the platform string is misspelled.
I don't think extending OnPlatform is a real issue. It's not like new platforms are going to appear every day/month/year.
Shouldn't we be more concerned for making Xamarin Forms great at least on iOS and Android?

@akamud

This comment has been minimized.

Show comment
Hide comment
@akamud

akamud Apr 25, 2017

Is this how default should work in the new way? Because it is not working at the moment:

<Label Text="This should be the default">
    <Label.Text>
        <OnPlatform x:TypeArguments="x:String">
            <On Platform="iOS">iOS</On>
        </OnPlatform>
    </Label.Text>
</Label>

If I run this sample on Android, the text is empty.

akamud commented Apr 25, 2017

Is this how default should work in the new way? Because it is not working at the moment:

<Label Text="This should be the default">
    <Label.Text>
        <OnPlatform x:TypeArguments="x:String">
            <On Platform="iOS">iOS</On>
        </OnPlatform>
    </Label.Text>
</Label>

If I run this sample on Android, the text is empty.

@dansiegel

This comment has been minimized.

Show comment
Hide comment
@dansiegel

dansiegel Apr 25, 2017

@akamud no what you are doing is overriding the Text Value, not setting a default. While there are some arguments that the new OnPlatform is more extensible, it wasn't fully thought out. The current releases would require that you specify the platform otherwise you effectively get default(T). See PR #873, handling a Default Value was just merged in. You can see how to implement it in the Evolution forum

@akamud no what you are doing is overriding the Text Value, not setting a default. While there are some arguments that the new OnPlatform is more extensible, it wasn't fully thought out. The current releases would require that you specify the platform otherwise you effectively get default(T). See PR #873, handling a Default Value was just merged in. You can see how to implement it in the Evolution forum

@xamarin xamarin locked and limited conversation to collaborators Apr 25, 2017

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.