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

VisualStateManager phase 1 #1405

Merged
merged 60 commits into from Jan 5, 2018

Conversation

@hartez
Member

hartez commented Dec 14, 2017

Description of Change

Adds the capability to manage properties on Forms VisualElements via declarative states. States can be declared in code or in XAML.

The Xamarin.Forms.Controls\GalleryPages\VisualStateManagerGalleries folder contains example code for using this feature.

Also adds the Platform Specific IsLegacyColorModeEnabled to provide the option of disabling the partial state/color handling introduced in earlier versions of Forms.

Bugs Fixed

API Changes

Added:

VisualStateManager:

public static Collection<VisualStateGroup> GetVisualStateGroups(VisualElement visualElement)
public static void SetVisualStateGroups(VisualElement visualElement, Collection<VisualStateGroup> value)
public static bool GoToState(VisualElement visualElement, string name)

AndroidSpecific, iOSSpecific, WindowsSpecific:

public static bool GetIsLegacyColorModeEnabled(BindableObject element)
public static void SetIsLegacyColorModeEnabled(BindableObject element, bool value)
public static bool GetIsLegacyColorModeEnabled(this IPlatformElementConfiguration<Android, VisualElement> config)
public static IPlatformElementConfiguration<Android, VisualElement> SetIsLegacyColorModeEnabled( this IPlatformElementConfiguration<Android, VisualElement> config, bool value)

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
@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Dec 14, 2017

CLA assistant check
All CLA requirements met.

dnfclas commented Dec 14, 2017

CLA assistant check
All CLA requirements met.

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Dec 14, 2017

Member

build

Member

hartez commented Dec 14, 2017

build

@StephaneDelcroix
  1. api changes part of the description misses VSG
  2. I'd have to do a 2nd pass on this Xaml/XamlC changes
  3. if you do not define your VSM in Style, XamlG will probably try to generate multiple fields with the name "Normal". The x:Name handling and RuntimeNamePropertyAttribute should be handled in the visitor handling x:Name and namescopes (creading a new node, skipping the x:Name setting)

[OOPS, clicked submit to early. continuing in another review]

@StephaneDelcroix

Part 2 of the review:

  1. I only reviewed the Xaml and Core parts.
  2. it misses Xaml unit tests for the xaml changes

Minus the comments, it looks all good. I'd have to make time to review the Xaml and XamlC changes

Show outdated Hide outdated Xamarin.Forms.Core/RuntimeNamePropertyAttribute.cs
Show outdated Hide outdated Xamarin.Forms.Core/Setter.cs
Show outdated Hide outdated Xamarin.Forms.Core/VisualStateManager.cs
Show outdated Hide outdated Xamarin.Forms.Core/VisualStateManager.cs
Show outdated Hide outdated Xamarin.Forms.Core/VisualStateManager.cs
internal VisualStateGroup Clone()
{
var clone = new VisualStateGroup {TargetType = TargetType, Name = Name, CurrentState = CurrentState};

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 15, 2017

Member

isn't this the right place to set the CurrentState as "Normal" ?

@StephaneDelcroix

StephaneDelcroix Dec 15, 2017

Member

isn't this the right place to set the CurrentState as "Normal" ?

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 15, 2017

Member

that seems a right place as well to throw if the name is null (x:Name is mandatory on VSG)

@StephaneDelcroix

StephaneDelcroix Dec 15, 2017

Member

that seems a right place as well to throw if the name is null (x:Name is mandatory on VSG)

This comment has been minimized.

@hartez

hartez Dec 15, 2017

Member

Setters aren't the only way to set the VisualStateGroups for an element. They can also be set directly in Xaml or from code. And we don't want to set CurrentState to normal directly on the property - we need it to be set with GoToState so the values for the Normal state are applied to the element.

@hartez

hartez Dec 15, 2017

Member

Setters aren't the only way to set the VisualStateGroups for an element. They can also be set directly in Xaml or from code. And we don't want to set CurrentState to normal directly on the property - we need it to be set with GoToState so the values for the Normal state are applied to the element.

Show outdated Hide outdated Xamarin.Forms.Core/VisualStateManager.cs
Show outdated Hide outdated Xamarin.Forms.Core/VisualStateManager.cs
Show outdated Hide outdated Xamarin.Forms.Core/VisualStateManager.cs
Show outdated Hide outdated Xamarin.Forms.Core/VisualStateManager.cs

@StephaneDelcroix StephaneDelcroix added this to Ready in v3.1.0 via automation Dec 15, 2017

@StephaneDelcroix StephaneDelcroix moved this from Ready to In Review in v3.1.0 Dec 15, 2017

@@ -88,8 +89,8 @@ public partial class VisualElement : Element, IAnimatable, IVisualElementControl
public static readonly BindableProperty MinimumHeightRequestProperty = BindableProperty.Create("MinimumHeightRequest", typeof(double), typeof(VisualElement), -1d, propertyChanged: OnRequestChanged);
[EditorBrowsable(EditorBrowsableState.Never)]
public static readonly BindablePropertyKey IsFocusedPropertyKey = BindableProperty.CreateReadOnly("IsFocused", typeof(bool), typeof(VisualElement), default(bool),
propertyChanged: OnIsFocusedPropertyChanged);
public static readonly BindablePropertyKey IsFocusedPropertyKey = BindableProperty.CreateReadOnly("IsFocused",

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 15, 2017

Member

I hate that public BP

@StephaneDelcroix

StephaneDelcroix Dec 15, 2017

Member

I hate that public BP

This comment has been minimized.

@hartez

hartez Dec 15, 2017

Member

It was like that when I got here. ¯\(ツ)

@hartez

hartez Dec 15, 2017

Member

It was like that when I got here. ¯\(ツ)

var isEnabled = (bool)newValue;
VisualStateManager.GoToState(element, isEnabled

This comment has been minimized.

@nickrandolph

nickrandolph Dec 16, 2017

Please don't call the VisualStateManager directly. There should be a method such as "UpdateVisualState" that is called whenever the VisualElement thinks the visual state should be updated. This should be avirtual method so that developers can easily sub-class and override the visual state manager behaviour.

@nickrandolph

nickrandolph Dec 16, 2017

Please don't call the VisualStateManager directly. There should be a method such as "UpdateVisualState" that is called whenever the VisualElement thinks the visual state should be updated. This should be avirtual method so that developers can easily sub-class and override the visual state manager behaviour.

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Dec 16, 2017

Member
Member

StephaneDelcroix commented Dec 16, 2017

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Dec 17, 2017

Member

if you do not define your VSM in Style, XamlG will probably try to generate multiple fields with the name "Normal".

Tested this, and you are absolutely correct.

The x:Name handling and RuntimeNamePropertyAttribute should be handled in the visitor handling x:Name and namescopes (creading a new node, skipping the x:Name setting)

So in the NameScopeVisitor, should I be creating a new namescope when I've got a VisualStateGroups element which is not part of a Style?

Member

hartez commented Dec 17, 2017

if you do not define your VSM in Style, XamlG will probably try to generate multiple fields with the name "Normal".

Tested this, and you are absolutely correct.

The x:Name handling and RuntimeNamePropertyAttribute should be handled in the visitor handling x:Name and namescopes (creading a new node, skipping the x:Name setting)

So in the NameScopeVisitor, should I be creating a new namescope when I've got a VisualStateGroups element which is not part of a Style?

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Dec 21, 2017

Member

if you do not define your VSM in Style, XamlG will probably try to generate multiple fields with the name "Normal".
This should be fixed now.

Member

hartez commented Dec 21, 2017

if you do not define your VSM in Style, XamlG will probably try to generate multiple fields with the name "Normal".
This should be fixed now.

internal VisualState GetState(string name)
{
foreach (VisualState state in States)

This comment has been minimized.

@tpetrina

tpetrina Dec 22, 2017

Contributor

If this method is called often, does it make sense to switch to for loop? Or is this premature optimisation?

@tpetrina

tpetrina Dec 22, 2017

Contributor

If this method is called often, does it make sense to switch to for loop? Or is this premature optimisation?

This comment has been minimized.

@hartez

hartez Dec 27, 2017

Member

I didn't think there'd be a significant difference, since the underlying implementation is using List. But just to be certain I wrote up some quick-and-dirty benchmarking tests to see if for would make any difference vs foreach here.

For the first batch, I created VisualStateGroupLists with 1 group, and the group had 2 states (which I think will be a pretty common scenario). I then had the test run through 100,000 iterations of moving randomly between the states (using the GoToState method). Between the two implementations (foreach and for), the test runs took the same amount of time on average.

For the second batch, I created an uglier scenario with VisualStateGroupLists with 10 groups, each group having 10 states. Running that over 100,000 iterations, the foreach actually ended up being slightly faster.

Looking at the tests in the profiler, the bulk of the time spent in the method (in either implementation) is the string comparison. The looping method doesn't end up making much difference.

So for the time being I think we'll stick with foreach for clarity. If we start running into performance problems with this later, we can revisit it.

@hartez

hartez Dec 27, 2017

Member

I didn't think there'd be a significant difference, since the underlying implementation is using List. But just to be certain I wrote up some quick-and-dirty benchmarking tests to see if for would make any difference vs foreach here.

For the first batch, I created VisualStateGroupLists with 1 group, and the group had 2 states (which I think will be a pretty common scenario). I then had the test run through 100,000 iterations of moving randomly between the states (using the GoToState method). Between the two implementations (foreach and for), the test runs took the same amount of time on average.

For the second batch, I created an uglier scenario with VisualStateGroupLists with 10 groups, each group having 10 states. Running that over 100,000 iterations, the foreach actually ended up being slightly faster.

Looking at the tests in the profiler, the bulk of the time spent in the method (in either implementation) is the string comparison. The looping method doesn't end up making much difference.

So for the time being I think we'll stick with foreach for clarity. If we start running into performance problems with this later, we can revisit it.

This comment has been minimized.

@jassmith

jassmith Jan 4, 2018

Member

The difference isn't in performance but in allocation. foreach generates an iterator which adds some GC pressure. It probably doesn't matter much though.

@jassmith

jassmith Jan 4, 2018

Member

The difference isn't in performance but in allocation. foreach generates an iterator which adds some GC pressure. It probably doesn't matter much though.

This comment has been minimized.

@hartez

hartez Jan 4, 2018

Member

The enumerator for List is a struct, so it's pretty low-impact.

@hartez

hartez Jan 4, 2018

Member

The enumerator for List is a struct, so it's pretty low-impact.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Jan 2, 2018

Member

failed test not related

Member

rmarinho commented Jan 2, 2018

failed test not related

hartez added some commits Dec 2, 2017

Remove irrelevant samples from the EntryDisabledStatesGallery
Make Background color work on UWP Entry with VSM

hartez added some commits Dec 15, 2017

@rmarinho

It's good stuff, i wonder if we should make useLegacy color a Flag instead of PS ..

@@ -6,19 +6,19 @@
namespace Xamarin.Forms.Controls
{
[Preserve (AllMembers=true)]
[Issue (IssueTracker.None, 0, "Default colors toggle test", PlatformAffected.All)]
[Issue (IssueTracker.None, 9906753, "Default colors toggle test", PlatformAffected.All)]

This comment has been minimized.

@rmarinho

rmarinho Jan 4, 2018

Member

you can drop the 990 now

@rmarinho

rmarinho Jan 4, 2018

Member

you can drop the 990 now

This comment has been minimized.

@hartez

hartez Jan 4, 2018

Member

No, I like the number 9906753.

@hartez

hartez Jan 4, 2018

Member

No, I like the number 9906753.

@rmarinho rmarinho merged commit 28948d7 into master Jan 5, 2018

13 checks passed

VSTS: Android API19 Validation Fast Renderers UITests Finished
Details
VSTS: Android API19 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API23 Validation Fast Renderers UITests Finished
Details
VSTS: Android API23 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API25 Validation Fast Renderers UITests Finished
Details
VSTS: Android API25 Validation Legacy Renderers UITests Finished
Details
VSTS: Xamarin Forms (PR Builds) Succeeded PR process
Details
VSTS: Xamarin Forms OSX PR-1405 - (1264648) succeeded
Details
VSTS: Xamarin Forms Windows VS2017 PR-1405 - (1264576) succeeded
Details
VSTS: iOS10 Validation UITests Finished
Details
VSTS: iOS11 Validation UITests Finished
Details
VSTS: iOS9 Validation UITests Finished
Details
license/cla All CLA requirements met.
Details

v3.1.0 automation moved this from In Review to Done Jan 5, 2018

@hartez hartez deleted the vsm2 branch Jan 24, 2018

@samhouts samhouts added the API-change label Apr 3, 2018

@samhouts samhouts added this to the 3.0.0 milestone May 5, 2018

@samhouts samhouts removed this from Done in v3.1.0 May 7, 2018

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