Skip to content
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

Adds FontImage Markup Extension for FontImageSource #6398

Merged
merged 12 commits into from Jun 28, 2019

Conversation

@ahoefling
Copy link
Contributor

commented Jun 4, 2019

Description of Change

Adds Markup Extension for FontImageSource to simplify usage from verbose XAML

<ImageButton>
    <ImageButton.Source>
        <FontImageSource
            FontFamily="{StaticResource MyFontFamily}"
            Glyph="{StaticResource SmileFace}"
            Color="{StaticResource PrimaryColor}" />
    </Image.Source>
</ImageButton>
<ImageButton Source="{FontImage FontFamily={StaticResource MyFontFamily}, Glyph={StaticResource SmileFace}, Color={StaticResource White}" />

Issues Resolved

API Changes

Added MarkupExtension: FontImageExtension to Xamarin.Forms.Xaml

Added:

string FontFamily { get; set; }
string Glyph { get; set; }
Color Color { get; set; }

Platforms Affected

  • Core/XAML (all platforms)
  • Any platform that implements FontImageSource

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

This markup extension has been tested in the Sandbox project and a confidential project I am working on.

I added positive and negative unit tests in the Xamarin.Forms.Xaml.UnitTests project

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
ahoefling added 4 commits Jun 4, 2019

@ahoefling ahoefling requested a review from StephaneDelcroix as a code owner Jun 4, 2019

@@ -24,6 +24,8 @@ public object Parse(string match, ref string remaining, IServiceProvider service
markupExtension = new OnIdiomExtension();
else if (match == "DataTemplate")
markupExtension = new DataTemplateExtension();
else if (match == "FontImage")

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Jun 4, 2019

Member

not required. I'm not even sure we save any measurable time doing this

This comment has been minimized.

Copy link
@ahoefling

ahoefling Jun 4, 2019

Author Contributor

I can remove this, no problem


namespace Xamarin.Forms.Xaml
{
[AcceptEmptyServiceProvider]

This comment has been minimized.

Copy link
@StephaneDelcroix

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Jun 4, 2019

Member

I'd add the Glyph as ContentProperty attribute

Suggested change
[AcceptEmptyServiceProvider]
[AcceptEmptyServiceProvider]
[ContentProperty("Glyph")]

This comment has been minimized.

Copy link
@ahoefling

ahoefling Jun 4, 2019

Author Contributor

This is not my first MarkupExtension and I noticed the other platform ones have [ContentProperty] but I am not sure exactly what it is for. How is it used?

Adding this change is not a problem at all

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@StephaneDelcroix I have the Xamarin.Forms.Sandbox project working with this change but didn't commit it. Should I commit that change as well to make it easier for you to see this change in action?

ahoefling added 2 commits Jun 4, 2019
@ChaseFlorell

This comment has been minimized.

Copy link

commented Jun 4, 2019

ContentProperty makes it so that you don't have to define Glyph=.

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@samhouts @StephaneDelcroix
Is there a tipping point on adding too many properties on the markup extension and how much we really gain? I am using this in one of my projects and came to a point where I really wanted to use Size.

I am thinking about adding the property (Size) in because it is really simple but want to check with everyone before I do that.

@ChaseFlorell

This comment has been minimized.

Copy link

commented Jun 5, 2019

@samhouts @StephaneDelcroix
Is there a tipping point on adding too many properties on the markup extension and how much we really gain? I am using this in one of my projects and came to a point where I really wanted to use Size.

I am thinking about adding the property (Size) in because it is really simple but want to check with everyone before I do that.

I think all options need to be there, or the extension isn't fully useful. (side note: default 30 for size is absurd).

I also put together a proposal whereby we could set some defaults in order to keep verbosity down.
#6421

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

I left it out originally because my target use case was for the Shell TabBar but now I am using it in another part of my app and found it really needs Size.

@samhouts samhouts requested a review from StephaneDelcroix Jun 5, 2019

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@ahoefling having all properties exposed adds versatility. that's what we want (see Binding). If all the options were mandatory, that would be verbose, but that's not the case here

@ChaseFlorell

This comment has been minimized.

Copy link

commented Jun 6, 2019

I gave this a try and got a strange result.

<!-- works good -->
<ImageButton Source="{FontImage FontFamily={StaticResource MyFontFamily}, Glyph={x:Static xml:Images.SmileFace}, Color={StaticResource White}" />

but

<!-- doesn't properly set Glyph -->
<ImageButton Source="{FontImage {x:Static xml:Images.SmileFace}, FontFamily={StaticResource MyFontFamily}, Color={StaticResource White}" />

In other words, the ContentProperty doesn't pick up the path the same way

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Could it be because we are using

[ContentProperty(nameof(Glyph))]

instead of

[ContentProperty("Glyph")]

I don't think this would cause that, but I don't know much about how the ContentProperty works except what we discussed earlier

ahoefling added 2 commits Jun 6, 2019
@ahoefling

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

I just added more unit tests to cover this and did some manually testing as well. It appears that [ContentProperty] is not properly picking up the markup extension of x:Static in the usage you demonstrated (@ChaseFlorell). When I attached the debugger to the unit test is shows the string literal of

{x:Static xml:Images.SmileFace

Yes, that is correct and it is missing the trailing } (curly brace). I then updated the code to pass in a hardcoded Glyph and it all appears to work.

@ChaseFlorell I think you discovered a bug with how the [ContentProperty] works with nesting usage of markup extensions.

ahoefling added 2 commits Jun 6, 2019
@ChaseFlorell

This comment has been minimized.

Copy link

commented Jun 6, 2019

I'm going to be submitting a proposal today that will require that the [ContentProperty(nameof(Glyph))] not be included on this markup extension.

Once the proposal is in, I will link back to this for further discussion.

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

After running several tests today I don't see a lot of value in [ContentProperty(nameof(Glyph)] since you can't use a markup extension with it to access your static reference. #6398 (comment).

I'm recommending that we remove the [ContentProperty(nameof(Glyph))] attribute so we can add a more useful [ContentProperty] in the future that won't be a breaking change.

public string FontFamily { get; set; }
public string Glyph { get; set; }
public Color Color { get; set; }
public double Size { get; set; } = 30d;

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Jun 6, 2019

Member

not sure about setting a default. does FontImageSource requires a Size ?

This comment has been minimized.

Copy link
@ChaseFlorell

ChaseFlorell Jun 6, 2019

Size already defaults to 30 on FontImageSource, but if you don't set the default here, it'll revert to 0.

Perhaps you can use FontImageSource.SizeProperty.DefaultValue

This comment has been minimized.

Copy link
@ChaseFlorell

ChaseFlorell Jun 6, 2019

alternatively, I don't know if FontImageSource has a concept of a magic value -1 to roll over to default.

This comment has been minimized.

Copy link
@ahoefling

ahoefling Jun 6, 2019

Author Contributor

My thought process behind this is the default size for FontImageSource is 30d. If someone uses this markup extension it will default to 0 unless they explicitly provide a size.

Perhaps we add a static default in FontImageSource and that way both the markup extension and FontImageSource can reference the default size.

FontImageSource

public class FontImageSource : ImageSource
{
    // ...
    public static double DefaultSize = 30d;
    // ...
}

FontImageExtension

public class FontImageExtension : IMarkupExtension<ImageSource>
{
    // ...
    public double Size { get; set; } = FontImageSource.DefaultSize;
    // ...
}

This comment has been minimized.

Copy link
@ChaseFlorell

ChaseFlorell Jun 6, 2019

You can access it directly off of the SizeProperty

public double Size { get; set; } = (double)FontImageSource.SizeProperty.DefaultValue;

This comment has been minimized.

Copy link
@ahoefling

ahoefling Jun 6, 2019

Author Contributor

That is a great suggestion, I'll go an update that to use the FontImageSource.SizeProperty.DefaultValue

Updated default value for Size to use the FontImageSource default's v…
…alue. This will keep it in sync if that value ever changes
@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

The ContentProperty has to be a literal, not another markup extension. I don't think it's bug

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Is there a reason why ContentProperty doesn't support MarkupExtensions? Is this something that should be included in a proposal and feature request?

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Is there a reason why ContentProperty doesn't support MarkupExtensions? Is this something that should be included in a proposal and feature request?

Here are the reasons I can think of:

  1. the need never showed up
  2. I have no idea if it's supported in other XAML variants,
  3. It would be very-very odd to write {Binding {StaticResource path}}

Now, if 2. is false and the syntax is supported in WPF or UWP (and it's supported by the XLS (/cc @mgoertz-msft)) we could consider it, but we're not going to make a precedent because of 1.

I suggested adding a ContentProperty not to support StaticResource for the Glyph, but for the cases when you prefer to use the glyph code instead of a resource.

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@StephaneDelcroix thanks for the detailed explanation, I am not as familiar with the reasoning behind it and wanted to understand better.

At this point I am in agreement with adding [ContentProperty(nameof(Glyph))] because it requires a string literal and we can't use a markup extension. Considering the proposal that @ChaseFlorell is working on I don't think ContentProperty would create any breaking changes as we would have to use the declarative syntax of

 <ImageButton Source="{FontImage Glyph={x:Static xml:Images.SmileFace}}" />

The ContentProperty is added here as a convenience utility for people that want to use a string literal

@samhouts samhouts requested a review from StephaneDelcroix Jun 7, 2019

@samhouts samhouts added the API-change label Jun 25, 2019

@samhouts samhouts added the approved label Jun 25, 2019

@samhouts samhouts merged commit efa9cf4 into xamarin:master Jun 28, 2019

7 checks passed

Xamarin Forms Build #4.2.0.2806173+22-sha.4be9b507-azdo.2806173 succeeded
Details
Xamarin Forms (Build Windows Phase Debug,any cpu) Build Windows Phase Debug,any cpu succeeded
Details
Xamarin Forms (Nuget Phase) Nuget Phase succeeded
Details
Xamarin Forms (OSX Phase) OSX Phase succeeded
Details
Xamarin Forms (Prepare Build Phase) Prepare Build Phase succeeded
Details
Xamarin Forms (Test Phase Debug) Test Phase Debug succeeded
Details
license/cla All CLA requirements met.
Details

vCurrent (4.2.0) automation moved this from In Progress to Done Jun 28, 2019

@samhouts samhouts removed this from Done in vCurrent (4.2.0) Jul 4, 2019

@samhouts samhouts added this to the 4.2.0 milestone Jul 16, 2019

@samhouts samhouts added this to Done in vCurrent (4.2.0) Jul 16, 2019

@samhouts samhouts added the F100 label Jul 25, 2019

beeradmoore added a commit to beeradmoore/Xamarin.Forms that referenced this pull request Jul 25, 2019
Adds FontImage Markup Extension for FontImageSource (xamarin#6398)
* Added FontImage markup extension which simplifies usage of FontImageSource into any ImageSource

* Added unit tests to cover TabBar which is used in Shell

* Cleaned up xaml for test code

* Removed class references from unit test project which were added by mistake

* Removed FontImageExtension from markup parser

* Added ContentProperty to markup extension

* Added Size to FontImageExtension

* Updated unit tests for new properties

* added unit tests to certify each property was set correctly by the markup extension

* Added content property test to certify that Glyph is being set correctly

* Flipped order in unit tests of expected and actual

* Updated default value for Size to use the FontImageSource default's value. This will keep it in sync if that value ever changes

fixes xamarin#6376
@jamesmontemagno

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Question...
It looks like this doesn't work with DynamicResources in the FontFamily?

Source="{FontImage FontFamily={DynamicResource MaterialFontFamily},
                                    Glyph={StaticResource IconUpNext}, 
                                    Color={StaticResource PrimaryColor},
                                    Size=15}"
            <OnPlatform x:Key="MaterialFontFamily" x:TypeArguments="x:String">
                <On Platform="iOS" Value="Material Design Icons" />
                <On Platform="Android" Value="materialdesignicons-webfont.ttf#Material Design Icons" />
                <On Platform="UWP" Value="Assets/Fonts/materialdesignicons-webfont.ttf#Material Design Icons" />
            </OnPlatform>

Compiler gives error about FontFamily:
. No property, bindable property, or event found for 'FontFamily', or mismatching type between value and property.
1>Done building project "Hanselman.csproj" -- FAILED.

if I changeto StaticResource compiles, but throws an exception at runtime.

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

the extension is fully resolved at the time it's parsed, that's why DynamicResource won't work

@jamesmontemagno

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Gotcha... any way to solve this to clean it up? Suggestions?

@jamesmontemagno

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Something like:

FontFamily={OnPlatform 
                        iOS={StaticResource MaterialFontFamilyiOS},
                        Android={StaticResource MaterialFontFamilyAndroid}}

?

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

@jamesmontemagno don't use the markup extension, it doesn't provide a lot of benefits as it doesn't save a lot of precious keystrokes

@jamesmontemagno

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Yeah, but it looks better! Can i define a StaticResource and update it in the App.xaml.cs?

@jamesmontemagno

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

NM it all works with StaticResource in place :) yay... for some reason Xamarin.Forms didn't update when I updated Xamarin.Forms.Visual.Material... weird.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.