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

Simplify TemplateBindings #1695

Merged
merged 5 commits into from
Jul 3, 2018
Merged

Simplify TemplateBindings #1695

merged 5 commits into from
Jul 3, 2018

Conversation

grokys
Copy link
Member

@grokys grokys commented Jun 25, 2018

Previously a TemplateBinding just created a normal Binding with a RelativeSourceMode.TemplatedParent. This PR changes TemplateBinding to a simple binding class in its own right, with a Property instead of a Path (note that this is also how TemplateBinding works in WPF and UWP).

Implementing TemplateBinding like this means that for the common case of binding to an AvaloniaProperty on the template parent, no allocations need to be made other than subscribing to a couple of PropertyChanged events: the TemplateBinding class itself is reused for the instantiated binding in the common case that it is bound to a single property.

In WPF and UWP, two-way bindings are not supported by TemplateBinding. Here they are: there was no downside to allowing this and it makes it more broadly usable.

Breaking Changes

  • Existing TemplateBindings which specify Path= will stop working, as the property is now called Property. Simply remove the Path= qualifier
  • Existing TemplateBindings which don't simply bind to a property on the templated parent will need to be changed to regular Bindings with RelativeSourceMode.TemplatedParent

Memory Usage

Memory usage was measured via the VS2017 diagnostic tools by running ControlCatalog in Release mode, opening all pages and then taking a memory snapshot:

Note that this PR depends on #1694:

On #1694:

Objects Heap Size (KB)
1,381,936 76,310

This PR:

Objects Heap Size (KB)
1,086,578 63,557

Depends on #1694

grokys added 2 commits June 25, 2018 09:53
Rather than just use a standard `Binding`, make `TemplateBinding` a lightweight binding in the case where the binding is simply to a property on the templated parent.
Can just use TemplateBinding directly.
@grokys grokys requested review from jkoritzinsky and a team June 25, 2018 08:09
@@ -139,7 +139,7 @@ public object Load(Uri uri, Uri baseUri = null, object rootInstance = null)
{
uriString = new Uri(baseUri, uri).AbsoluteUri;
}
throw new XamlLoadException("Error loading xaml at " + uriString, e);
throw new XamlLoadException("Error loading xaml at " + uriString + ": " + e.Message, e);
Copy link
Member Author

Choose a reason for hiding this comment

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

Also fixed the annoying problem where the XAML exception message didn't actually include the description of what went wrong.

@@ -72,8 +72,7 @@
<Style Selector="Slider /template/ Track#PART_Track">
<Setter Property="Minimum" Value="{TemplateBinding Minimum}"/>
<Setter Property="Maximum" Value="{TemplateBinding Maximum}"/>
<Setter Property="Value" Value="{TemplateBinding Path=Value, Mode=TwoWay}"/>
<Setter Property="Orientation" Value="{TemplateBinding Orientation}"/>
<Setter Property="Value" Value="{TemplateBinding Value, Mode=TwoWay}"/>
Copy link
Member Author

@grokys grokys Jun 25, 2018

Choose a reason for hiding this comment

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

This setter was both unnecessary and caused problems in that it was binding to Track.Orientation instead of Slider.Orientation which are actually different AvaloniaPropertys (#1696). This was resulting in the Slider getting messed up.

public class AvaloniaPropertyTypeConverter : TypeConverter
{
private static readonly Regex regex = new Regex(@"^\(?(\w*)\.(\w*)\)?|(.*)$");
Copy link
Member Author

Choose a reason for hiding this comment

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

This allows attached properties to be optionally specified with braces surrounding the property, as in a standard Binding.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way we can do this without using regular expressions? Regex uses lot of memory for us just checking if the string starts and ends with parentheses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah definitely! I looked at using StringTokenizer for this, but it wasn't suitable, and I didn't want to introduce another string tokenizer in this PR. Wasn't sure if we should make StringTokenizer more general purpose, or just manually parse the string here, so I just used Regex for the time being.

This shouldn't happen normally as `InpcPropertyAcessorPlugin` matches everything.
Copy link
Collaborator

@wieslawsoltes wieslawsoltes left a comment

Choose a reason for hiding this comment

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

As this is part of #1703 and it was tested so this PR is also tested 👍

@jkoritzinsky
Copy link
Collaborator

Other than the regex, this looks good! I'll merge it in!

@jkoritzinsky jkoritzinsky merged commit 7a6b3e3 into master Jul 3, 2018
@jkoritzinsky jkoritzinsky deleted the templatebinding branch July 3, 2018 19:19
@grokys grokys added this to the 0.7.0 milestone Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants