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

Implement OnPlatform/OnIdiom markup extensions #2615

Merged
merged 14 commits into from May 14, 2018

Conversation

Projects
@kzu
Member

kzu commented May 7, 2018

Fixes #2608.

image

image

ahoefling and others added some commits May 4, 2018

Add Default and Other properties to OnPlatformExtension
This allows setting a default value for unknown platforms, as well
as specify values for arbitrary platforms by using a named parameter
like syntax:

`{OnPlatform Android=15, iOS=10, UWP=12, Default=11, Other=Tizen:20}`

The `Other` supports multiple semi-colon separated values. By using
this format, we can make the string more concise than if we used `=`
which would have to be escaped in quotes. For example:

`{OnPlatform Default=10, Other=Tizen:22;Xbox:20;Switch=25;PlayStation=22}`

Added unit tests that verify all the supported combinations.
Add OnIdiomExtension
The extension allows the following syntax:

`{OnIdiom Phone=23, Tablet=25, Desktop=26, TV=30, Watch=10, Default=20}`

At least one value or `Default` must be specified. `Default` is returned
whenever the specific idiom was not specified.
Add missing known platforms and return Default if missing
Add all strings that are provided in `Device`.
Convert individual asserts into test cases for better reporting.
Also, whenever a matching platform value isn't specified, return Default
instead of null.
Turn OnIdiom asserts into test cases
This makes for easier to read, document and report tests.
@kzu

This comment has been minimized.

Member

kzu commented May 7, 2018

Gotta love having WiFi on the airplane on my way to //build :)

@jassmith all yours!

@kzu kzu requested a review from jassmith May 7, 2018

@dansiegel

This comment has been minimized.

dansiegel commented May 7, 2018

Default should be added as a ContentProperty so that you can have:

<Label FontSize="{OnPlatform 10, Android=12, UWP=11}" />

verses the more verbose:

<Label FontSize="{OnPlatform Default=10, Android=12, UWP=11}" />
@StephaneDelcroix

it's indeed more concise, unfortunately it doesn't work with XamlC on

namespace Xamarin.Forms.Xaml
{
public class OnIdiomExtension : IMarkupExtension

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 7, 2018

Member

add a [ContentProperty("Default")]

else if (match == "OnPlatform")
markupExtension = new OnPlatformExtension();
else if (match == "OnIdiom")
markupExtension = new OnIdiomExtension();

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 7, 2018

Member

this isn't required, but it allows bypassing constructing the markup through reflection

public object TV { get; set; }
public object Watch { get; set; }
public object ProvideValue(IServiceProvider serviceProvider)

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 7, 2018

Member

the Xaml engine expects from ProvideValue a value of the correct type that can be directly assigned to the property. In this case, all the objects are of type string, and not directly assignable to FontSize, TextColor, WidthRequest...

the magic of the runtime Xaml loader compensates for the wrong type and do the type conversion, but

  • I can't guarantee that to work in all cases
  • if fails with XamlC on

This comment has been minimized.

@kzu

kzu May 7, 2018

Member

Addressed in latest commits. Thanks!

{
// See Device.Idiom
public object Default { get; set; }

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 7, 2018

Member

could just be string in your case...

as the properties aren't typed, you won't get intelissense completion unless special-cased

This comment has been minimized.

@kzu

kzu May 7, 2018

Member

By making these object, we can allow further markup extensions to set those values (i.e. StaticResource?)

This comment has been minimized.

@ahoefling

ahoefling May 8, 2018

@kzu when I wrote the original change the idea was to make everything an object so we could use this on more than just strings. Since this is a new platform markup extension we should support more than just strings with it. This will open up a lot of flexibility for developers to have let's say different images or objects inserted into their XAML based on the platform or idiom which is a powerful thing.

kzu added some commits May 7, 2018

Perform type conversion as expected by XamlC
Leverage the conversion that is used elsewhere, to return a
properly typed object that can be assigned directly to the
property. Update tests with typed values since now we get
integers, rather than strings out of the parser.
@ChaseFlorell

This comment has been minimized.

ChaseFlorell commented May 7, 2018

@ahoefling that 'other' property is what I was referring to in my previous comment. This is a great addition to Forms 👍

@kzu

This comment has been minimized.

Member

kzu commented May 7, 2018

Ok, feedback addressed, should now work just fine with XamlC 👍

@dansiegel

This comment has been minimized.

dansiegel commented May 7, 2018

it's indeed more concise, unfortunately it doesn't work with XamlC on

This brings up an interesting point. @kzu I imagine there are going to be cases then that we'll need to provide a Converter where the default converters just won't work. Probably will want to add a Converter property as well to handle those cases.

@kzu

This comment has been minimized.

Member

kzu commented May 7, 2018

If the property is already annotated with a converter, that's already supported ;)

But I guess I could add that too...

@kzu

This comment has been minimized.

Member

kzu commented May 7, 2018

Ok, added Converter/ConverterParameter as seen elsewhere in the codebase. Not sure if the "right" culture to use for the conversion should be CultureInfo.CurrentUICulture (as in Binding), or InvariantCulture as the TypeConversionExtensions does.

I've settled on the former since that's what's passed in in all instances I found of IValueConverter.Convert. Hopefully that's the right one?

Incorporated feedback in newer commits!

@ahoefling

Added some feedback to why I originally chose object instead of string

{
// See Device.Idiom
public object Default { get; set; }

This comment has been minimized.

@ahoefling

ahoefling May 8, 2018

@kzu when I wrote the original change the idea was to make everything an object so we could use this on more than just strings. Since this is a new platform markup extension we should support more than just strings with it. This will open up a lot of flexibility for developers to have let's say different images or objects inserted into their XAML based on the platform or idiom which is a powerful thing.

@StephaneDelcroix

ok for me, modulo the Unsupported and Other values

if (Default == null && Unsupported == null && Phone == null &&
Tablet == null && Desktop == null && TV == null && Watch == null)
{
throw new XamlParseException("OnIdiomExtension requires a value to be specified for at least one idiom or Default.", lineInfo ?? new XmlLineInfo());

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 8, 2018

Member

so, doing {OnIdiom Phone={x:Null}} will throws even though a value is actually specified ?

I'd change the message requesting one of the value to be non-null

This comment has been minimized.

@kzu

kzu May 8, 2018

Member

Makes sense!

public object ProvideValue(IServiceProvider serviceProvider)
{
var lineInfo = (serviceProvider.GetService(typeof(IXmlLineInfoProvider)) as IXmlLineInfoProvider)?.XmlLineInfo;

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 8, 2018

Member

I recently added a generic GetService<T> you might want to use.

also, do serviceProvider?.GetService...

throw new XamlParseException("OnIdiomExtension requires a value to be specified for at least one idiom or Default.", lineInfo ?? new XmlLineInfo());
}
var valueProvider = serviceProvider.GetService<IProvideValueTarget>() ?? throw new ArgumentException();

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 8, 2018

Member

serviceProvider?.

This comment has been minimized.

@kzu

kzu May 8, 2018

Member

Can the service provider itself be null?

// See Device.Idiom
public object Default { get; set; }
public object Unsupported { get; set; }

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 8, 2018

Member

I'd remove the Unsupported property

public object Tizen { get; set; }
public object UWP { get; set; }
public object WPF { get; set; }
public string Other { get; set; }

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 8, 2018

Member

Other should't be of type string, as it limits the values to be literals. Unless you plan to parse the potential markups...

I'd remove the Other for now, so we can think about a better solution for it (I could, e.g., modify the markup extension parser to add values in a directory instead of assigning properties)

This comment has been minimized.

@ChaseFlorell

ChaseFlorell May 8, 2018

I thought Other being of type string was to allow string parsing like this in case they have their own custom platforms.

<Button WidthRequest="{OnPlatform Other='Foo:21,Bar:25'}" />

This comment has been minimized.

@kzu

kzu May 8, 2018

Member

Yep, indeed. So, @StephaneDelcroix removing it means we don't support custom platforms, but I guess that's fine too for a "v1"?

This comment has been minimized.

@ChaseFlorell

ChaseFlorell May 8, 2018

Alternatively, the extension can be inherited by the user with their custom platform. You abstract away all the shared bits and then implement a new virtual method HandlePlatform

// pseudo code
public class MyOnPlatformExtension : OnPlatformExtension
{
    public object MyFooPlatform { get; set; }
    public override object HandlePlatform()
    {
        if(MyFooPlatform != null && Device.RuntimePlatform == nameof(MyFooPlatform))
        {
            return MyFooPlatform;
        } else {
            return base.HandlePlatform();
        }
    }
}

Usage

<Button WidthRequest="{xaml:MyOnPlatform Default=25, MyFooPlatform=22}" />

This comment has been minimized.

@kzu

kzu May 8, 2018

Member

Why wouldn't the Default be enough if all the other platforms have their own values? Effectively, the Default would become the value for their custom platform...

This comment has been minimized.

@ChaseFlorell

ChaseFlorell May 8, 2018

excepting for the fact that they could potentially have more than one custom platform. Isn't that why Forms moved to stringly typed anyways?

At any rate, if you move the switch logic into a separate virtual method, we'd be able to override easily using my example above.

obviously you don't have to use HandlePlatform it's just an example 😉

@jassmith jassmith removed their request for review May 8, 2018

@kzu

This comment has been minimized.

Member

kzu commented May 8, 2018

Ok @StephaneDelcroix @jassmith I think this is ready now :)

public object Tizen { get; set; }
public object UWP { get; set; }
public object WPF { get; set; }
public string Other { get; set; }

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 9, 2018

Member

you removed Other from the switch, but left the property

var lineInfo = serviceProvider?.GetService<IXmlLineInfoProvider>()?.XmlLineInfo;
if (Android == null && GTK == null && iOS == null &&
macOS == null && Tizen == null && UWP == null &&
WPF == null && Default == null && string.IsNullOrEmpty(Other))

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 14, 2018

Member

doesn't build. Other is no longer defined

This comment has been minimized.

@kzu

kzu May 14, 2018

Member

🤦‍♂️

Remove Other from OnPlatformExtension
As suggested, this might come back in the future in some other form.

@StephaneDelcroix StephaneDelcroix merged commit dc62dc1 into xamarin:master May 14, 2018

2 of 3 checks passed

VSTS: Xamarin Forms OSX PR-2615 - (1684491) failed
Details
VSTS: Xamarin Forms (PR Builds) PR-2615 - (1684470) succeeded
Details
VSTS: Xamarin Forms Windows VS2017 PR-2615 - (1684474) succeeded
Details

vNext+1 (master) automation moved this from In Progress to Done May 14, 2018

@kzu kzu deleted the kzu:onmarkup branch May 14, 2018

@ghuntley

This comment has been minimized.

ghuntley commented May 14, 2018

this is rad

@samhouts samhouts added this to the 3.2.0 milestone Jun 26, 2018

@samhouts samhouts removed this from Done in vNext+1 (master) Jun 26, 2018

@samhouts samhouts added this to Done in v3.2.0 Jun 26, 2018

@akamud

This comment has been minimized.

akamud commented Aug 11, 2018

This should be added to the documentation, how are the documentation changes being tracked and linked to the Pull Requests?

@EmilAlipiev

This comment has been minimized.

EmilAlipiev commented Oct 4, 2018

It doesnt seem to be working using like below. I am getting

No property, bindable property, or event found for 'Default', or mismatching type between value and property.

<OnIdiom x:TypeArguments="x:Double" Phone="150" Tablet="250" Default="300" />

that seems to be working however

`   HeightRequest="{OnIdiom Phone=200, Tablet=300, Default=350}"`

is that intended?

@StephaneDelcroix

This comment has been minimized.

Member

StephaneDelcroix commented Oct 4, 2018

@EmilAlipiev we have currently 2 issues about the OnPlatform/OnIdiom markup extensions: #3862 and #3920. If your issue relate to any of those, please add a comment. Otherwise, please open a new issue, as we do not follow-up on closed PRs. Thanks.

@samhouts samhouts added the a/Xaml </> label Nov 2, 2018

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