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

Added the ability to Target a bindable object for Setters #5034

Closed

Conversation

brainoffline
Copy link
Contributor

@brainoffline brainoffline commented Jan 22, 2019

Description of Change

Adds the ability to target a child object for a Setter for visual state management or Styles.
See VisualStateManagerTests.xaml for an example.

	<VisualStateManager.VisualStateGroups>
		<VisualStateGroup Name="ColorStates">
			<VisualState Name="Red">
				<VisualState.Setters>
					<Setter TargetName="TargetLabel1" Property="Label.BackgroundColor" Value="#340002" />
					<Setter TargetName="TargetLabel1" Property="Label.TextColor" Value="#920e0c" />
					<Setter TargetName="TargetLabel1" Property="Label.Text" Value="Red" />
				</VisualState.Setters>
			</VisualState>

			<VisualState Name="Blue">
				<VisualState.Setters>
					<Setter TargetName="TargetLabel1" Property="Label.BackgroundColor" Value="#4a707a" />
					<Setter TargetName="TargetLabel1" Property="Label.TextColor" Value="#94b0b7" />
					<Setter TargetName="TargetLabel1" Property="Label.Text" Value="Blue" />
				</VisualState.Setters>
			</VisualState>
		</VisualStateGroup>
	</VisualStateManager.VisualStateGroups>

Issues Resolved

API Changes

  • Core/XAML (all platforms)

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

Include unit and xaml unit tests
corrected naming to prevent issues
Updating xamlc is still outstanding
Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

I love this! A small, but powerful PR!

I just had a question on the changes from x:Name="" to Name="" - was that intentional?

@mattleibow
Copy link
Contributor

mattleibow commented Jan 22, 2019

I have another question... How does this work with styles? One of the things that I used this part of the VSM is to apply a style to all instances of a custom control. Then, I would change the state, and the manager would update sub views. Similar thing with templates - we don't have access to the actual instance at the time of creation.

Not sure what this all means, but is a lookup for the element by name better than a reference? If this all works, then this is fine.

An example case is say I have a control that I am using - either local or third party:

<!-- LabelledEntry -->
<ContentView>
  <StackLayout>
    <Label Text="Enter text here:" />
    <Entry Name="TextEntry" />
  </StackLayout>
</ContentView>

Then, on my page (or in the app) resources, I create a style:

<Style TargetType="LabelledEntry">
  <Setter Property="VisualStateManager.VisualStateGroups">
    <VisualStateGroupList>
      <VisualStateGroup x:Name="CommonStates">
        <VisualState x:Name="Normal" />
        <VisualState x:Name="Invalid">
          <VisualState.Setters>
            <Setter Target="TextEntry" Property="Entry.BorderColor" Value="Red" />
          </VisualState.Setters>
        </VisualState>
      </VisualStateGroup>
    </VisualStateGroupList>
  </Setter>
</Style>

Now, if I have this xaml:

<ContentPage>
  <LinearLayout x:Name="myControls">

    <!-- our custom control where we injected our error state -->
    <LabelledEntry Text="{Binding TheText}" />

    <!-- a control that already supports error states  -->
    <ErrorSupportedEntry Text="{Binding TheText}" />

    <!-- summary -->
    <ErrorList IsVisible="false">
      <VisualStateManager.VisualStateGroups>
        <VisualStateGroupList>
          <VisualStateGroup x:Name="CommonStates">
            <VisualState x:Name="Normal">
              <VisualState.Setters>
                <Setter Property="IsVisible" Value="Normal" />
              </VisualState.Setters>
            </VisualState>
            <VisualState x:Name="Invalid" />
          </VisualStateGroup>
        </VisualStateGroupList>
      </VisualStateManager.VisualStateGroups>
    </ErrorList>

  </LinearLayout>
</ContentPage>

In my code, I can just go to states:

foreach (var entry in myControls.Children) {
  VisualStateManager.GoToState(entry, isError ? "Invalid" : "Normal");
}

@brainoffline
Copy link
Contributor Author

@mattleibow That is not the separation of concerns that I would normally take.
My approach would be that the LabelledEntry would be self-contained and expose maybe a HasError property. Based on that property, it would be the responsibility of the LabelledEntry control to change it's own visual state.
Therefor, there is no problem with referring to an internal control (x:Name="TextEntry") from the page's code-behind. It will also enable binding to the HasError property.

@mattleibow
Copy link
Contributor

How does this work with the VSM added via Style? The same issue occurs, you don't have a reference to any objects - but you want to add a state.

You could add the state to the actual control, but you loose the ability to have different state styles.

The case I actually noticed this was with different screen sizes/orientation. When I was in a wide screen, I moved some elements around (which can use references), but then changed the layout of items in a list (which was added via data binding/templates). I could have created a control to wrap the items and their layout, but that means code. The whole point of XAML is to not write code for visual things.

For example, here is a sample xaml file that defines some control and uses the visual state manager and styles for a certain look. When the screen goes wide, it changes:
https://gist.github.com/mattleibow/2f2f33c353be0b33e31bf9007095b105

This example is a little weird in that I use n AdaptiveTrigger which is related to the window size, but that is just for demo purposes. It could very well be a custom trigger that just decides based on device type (phone / desktop / tv). Using this forn of styling, You can define your UI once and then just move things around based on screen size or app type. There are alternatives to actually re-create the entire template for each style, but that is a lot of duplication of XAML.

@brainoffline
Copy link
Contributor Author

Great feedback. Thank you @mattleibow
I have added additional flexibility when selecting the target to apply the values to.

<Setter Target="{x:Reference TargetLabel1}" Property="Label.BackgroundColor" Value="#4a707a" />
<Setter TargetName="TargetLabel1" Property="Label.TextColor" Value="#94b0b7" />

To select which object to update, the search starts where the VSM is defined. i.e. Page level, control level, ...
If Target is present, then that object will be selected.
If TargetName is present, it will search under the currently selected object (VSM parent or Target) to find the correct BindableObject to select.
Values are then applied to the Property of the selected object.

This may be a little more complex to document, but it should work for more cases.

Copy link
Member

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

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

Setters are also used in Styles, how does the Target and TargetName properties behave in that case ?

Xamarin.Forms.Core/Setter.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Core/Setter.cs Outdated Show resolved Hide resolved
@StephaneDelcroix StephaneDelcroix added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Jan 23, 2019
@StephaneDelcroix
Copy link
Member

needs to be targeted to master

@brainoffline brainoffline changed the base branch from 3.5.0 to master January 23, 2019 09:22
@brainoffline
Copy link
Contributor Author

brainoffline commented Jan 23, 2019

Just slapped it with the Simple stick. Less complexity = less problems.

I have removed Target from the Setter.
This leaves TargetName as a way of targeting child elements. This is the same model used by WPF.

If TargetName is present, then it will search the NameScope of the applied bindable object (basically child elements).
This technique applies to both Style Setters as well as VSM Setters. They are essentially the same thing.

@mattleibow I think this was your original preference

@mattleibow
Copy link
Contributor

I also liked the fact that a reference could be set... That might have been a performance boost - not requiring a search through the views.

But then things get complicated for the user...

But, we can also be extra clever and make the type object... If it is a string, we do a lookup. If it is a bind able object, we use that directly. Or, we can go the route of creating a new type that implicitly converts from both:

class BindableTargetObject {
    public string TargetName;
    public BindableObject Target;
    public BindableObject GetTarget(BindableObject scope) =>
        Target ?? scope?.FindObject(TargetName) ?? scope;
} 

A bit more complex, but actually quite simple - especially since this works with Styles.

@brainoffline
Copy link
Contributor Author

More awesome feedback.
I am not sure how introducing a BindableTargetObject would look in the Xaml. I have gone with the simpler approach of using an object.
I have changed the property name back to Target and the developer can either using a x:Reference or name of object. Reference is quicker and name has more flexibility around templates and stuff.

Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

This is looking really good!

I was just thinking of OnPlatform. And then we have this big chunk of XAML that can totally be fixed:
https://github.com/xamarin/Xamarin.Forms/blob/bd31e1e9fc8b2f9ad94cc99e0c7ab058174821f3/Xamarin.Forms.Controls/GalleryPages/VisualStateManagerGalleries/ValidationExample.xaml

You probably can fit in a string, reference, style and OnPlatform (on the Target) into that one. I think those should cover the main thrust of the PR and we will have a visual confirmation.

BindableObject FindTargetObject(BindableObject scope, object target)
{
if (scope == null || target == null)
return scope;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? I think the next 2 if statements will catch any nulls. The only case that looks to change is if scope is null, but target is BindableObject. But, we are returning the object directly.

@samhouts samhouts removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Jan 29, 2019
@jfversluis jfversluis removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Jul 26, 2019
@jfversluis
Copy link
Member

Hey @brainoffline thanks for your awesome work! We would like to get this in soon. Since you haven’t responded in some time, I’m going to assume that you do not have the possibility to finish it.

@andreinitescu you’ve offered. Are you able to make some progress on this? Else we will take it ourselves

@andreinitescu
Copy link
Contributor

Yes, I did offer, but I got no reply if I could actually start the work on it.
But, since you seem eager to work on it, sure, please go ahead, I don't want to slow releasing new features to the framework...

@andreinitescu
Copy link
Contributor

andreinitescu commented Aug 1, 2019

Most of the work implementing string TargetName was done already by @brainoffline, like it was suggested above, it would be good to support object Target like in UWP.

@samhouts samhouts changed the base branch from master to 4.3.0 August 29, 2019 23:48
@samhouts samhouts added the Core label Sep 5, 2019
@samhouts
Copy link
Member

@andreinitescu It looks like you may be able to get to this before us! Still willing? Thanks!

@samhouts samhouts changed the base branch from 4.3.0 to master October 18, 2019 17:40
@jfversluis jfversluis mentioned this pull request Oct 22, 2019
2 tasks
@bezysoftware
Copy link

bezysoftware commented Oct 29, 2019

Was this pushed to v4.4.0? https://github.com/xamarin/Xamarin.Forms/wiki/Feature-Roadmap still indicates v4.3.0

@jfversluis
Copy link
Member

Thanks for asking @bezysoftware!

The thing I can say for sure is that it won't be in 4.3 unfortunately. I will do my absolute best to get it into 4.4. The PR is mostly ready, so I think that should be possible :)

@samhouts samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Nov 2, 2019
@samhouts samhouts added the blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. label Nov 21, 2019
@samhouts
Copy link
Member

closing in favor of #8144

@samhouts samhouts closed this Nov 22, 2019
samhouts pushed a commit that referenced this pull request Dec 6, 2019
* Added the ability to Target a bindable object for Setters

Include unit and xaml unit tests
corrected naming to prevent issues
Updating xamlc is still outstanding

* Resolve xamlc compiler with new Target property

* Added ability to find target element via Name

* Remove Target for simplicity

* Additional target type for flexibility while not introducing complexity

* [Core] Remove inline comment

* Added gallery page

* Last bits

* Fixed unit tests

* Update Setter.cs
fixes #4924
closes #5034
closes #5622
@samhouts samhouts added this to the 4.5.0 milestone Jan 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/VSM a/Xaml </> blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. Core DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. F100 inactive Issue is older than 6 months and needs to be retested retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. roadmap t/enhancement ➕
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CollectionView SelectedItem Color [F100] Add "Target" support to VisualStateManager
8 participants