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

Relative source syntax sugar #1209

Merged
merged 18 commits into from Dec 2, 2017

Conversation

Projects
None yet
3 participants
@jkoritzinsky
Member

jkoritzinsky commented Oct 12, 2017

Syntax sugar for RelativeSource bindings. Fixes #590.

You can now use the following shorthand for RelativeSource bindings:

Shorthand Implementation
$self Mode = Self
$parent Mode = FindAncestor; AncestorLevel = 1
$parent[Level] Mode = FindAncestor; AncestorLevel = Level +1
$parent[ns:Type] Mode = FindAncestor; AncestorType = ns:Type
$parent[ns:Type; Level] Mode = FindAncestor; AncestorType = ns:Type; AncestorLevel = Level + 1

@jkoritzinsky jkoritzinsky requested a review from grokys Oct 12, 2017

@grokys

Left a few comments. If you agree, I wonder if maybe we should do this in a couple of separate PRs:

  • Make Binding a markup extension
  • Add a Tree=Logical parameter to RelativeSource

Aside: We can probably simplify things a bit in our code here because any class can be used as a markup extension in XAML (cwensley/Portable.Xaml#65) so we can probably merge a few classes...

}
else if(relativeSourceMode == "parent")
{
relativeSource.Mode = RelativeSourceMode.FindAncestor;

This comment has been minimized.

@grokys

grokys Oct 13, 2017

Member

This probably isn't what we want here, RelativeSource works on the visual tree, so:

<ContentControl>
  <TextBlock Text="$parent.Foo"/>
<ContentControl>

Will bind to the ContentPresenter in the ContentControl rather than the ContentControl. This is because we follow WPF's semantics for RelativeSource and that works on the visual rather than logical tree. This is OK in WPF because you have to specify the type of the ancestor, but here one would intuitively expect it to work on the logical tree.

private static Type LookupAncestorType(string ancestorTypeName)
{
//TODO: What is our syntax for type lookup here?
throw new NotImplementedException();

This comment has been minimized.

@grokys

grokys Oct 13, 2017

Member

Would simply $parent[ns:ControlType] work here? To do this I think we'd need to make Binding a MarkupExtension so we have access to the XAML context. This is how it is in WPF but we couldn't do it previously because of how OmniXAML worked(OmniGUI/OmniXAML#74) but we should be able to do it now we're using Portable.Xaml.

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Oct 15, 2017

I'm ok with making Binding a markup extension,. My main curiosity right now with that is the separation between Binding and XamlBinding. would we combine those two classes as well?

@grokys

This comment has been minimized.

Member

grokys commented Oct 15, 2017

Yeah I think those 2 classes could be merged. I did something similar when I went from StyleResource to DynamicResource/StaticResource.

jkoritzinsky added some commits Oct 15, 2017

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Oct 15, 2017

@grokys I've moved the path shorthand parsing to the BindingExtension class.

I also changed the FindAncestor lookup when AncestorType == null to lookup on the logical tree instead, as well as fix some bugs in ControlLocator.

@jkoritzinsky jkoritzinsky changed the title from WIP: Relative source syntax sugar to Relative source syntax sugar Oct 15, 2017

jkoritzinsky added some commits Oct 15, 2017

@grokys

This is great @jkoritzinsky. Not had chance for a full review yet, but found something that doesn't look right.

var detached = Observable.FromEventPattern<LogicalTreeAttachmentEventArgs>(
x => relativeTo.DetachedFromLogicalTree += x,
x => relativeTo.DetachedFromLogicalTree += x)

This comment has been minimized.

@grokys

grokys Oct 18, 2017

Member

I think this should be -=.

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Oct 18, 2017

Member

Good catch! I'll fix that.

@grokys grokys requested review from wieslawsoltes and danwalmsley Oct 18, 2017

jkoritzinsky and others added some commits Oct 18, 2017

Added more tests.
One failing.
@grokys

This comment has been minimized.

Member

grokys commented Oct 18, 2017

I'm a little unsure about switching between logical and visual trees depending on whether a type is specified. For example, I'd expect these two snippets to do the same:

<ContentControl>
  <TextBlock Tag="{Binding $parent[0]}"/>
</ContentControl>
<ContentControl>
  <TextBlock Tag="{Binding $parent[Control;0]}"/>
</ContentControl>

But currently they won't: the first will bind to ContentControl and the second to ContentPresenter. This strikes me as a potential source of confusion...

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Oct 23, 2017

So do we want to have the shorthand use the logical tree and the non-shorthand use the visual? Or do we want to just use the visual tree and change the code so it can support an AncestorType of null? Or should we default it to IControl?

@grokys

This comment has been minimized.

Member

grokys commented Oct 25, 2017

Well for the longhand I think we're bound to follow WPF's syntax. The WPF syntax isn't confusing because you always have to supply the control type. But if for the shorthand we're allowing the user to not supply a control type then I think it'd be confusing (and also not that useful) to use the visual tree.

So if that's accepted, then we need to support looking up in both the logical and visual tree.

If we need to support both then still my preferred solution is to supply a public TreeType Tree { get; set; } property on RelativeSource. The WPF longhand syntax would then default to TreeType.Visual and the shorthand syntax would use TreeType.Logical.

What do you think?

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Oct 25, 2017

Sounds good to me! I'll implement it that way.

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Oct 26, 2017

I've updated the implementation as per your comments. Can you take another look?

@danwalmsley

This comment has been minimized.

Member

danwalmsley commented Oct 29, 2017

I know its a bit late, however allow me to suggest what I think could be a minor improvement for the syntax...

instead of
$parent[ns:Type; Level]

types could be declared in <> brackets like a generic class in c#?
so this would become

$parent<ns:Type>[Level]

essentially [] is reserved for indexing, and <> is reserved for type selection, might be more memorable than [type;level] could easily forget the order?

Looks good though will give this a test hopefully mon or tuesday

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Oct 29, 2017

We can't do angle brackets because that gets funky when the XAML is being parsed. And I don't want our users to have to escape the angle brackets. It would ruin the cleanness of the shorthand.

@danwalmsley

This comment has been minimized.

Member

danwalmsley commented Oct 29, 2017

ah of course! :)

@grokys

@jkoritzinsky - really sorry for the delay on reviewing this. I think we're just about there, but just one thing I'm not convinced about. I'm willing to be persuaded though :)

@@ -78,6 +78,87 @@ public void Binding_To_First_Ancestor_Works()
}
[Fact]
public void Binding_To_First_Ancestor_Without_AncestorType_Uses_LogicalTree()

This comment has been minimized.

@grokys

grokys Nov 10, 2017

Member

Hmm, I still think that this implicit switching of trees depending on whether AncestorType is specified or not could be confusing. I'd prefer to require AncestorType when Tree=Visual (following WPF) and require Tree=Logical to switch over to the logical tree where AncestorType doesn't need to be specified. What do you think?

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Nov 12, 2017

Member

I'll make AncestorType be required when Tree=Visual. Sounds reasonable to me.

The automatic switch isn't happening any more. I just forgot to remove the test.

window.ApplyTemplate();
// This isn't needed in `Binding_To_First_Ancestor_Without_AncestorType_Uses_LogicalTree` -

This comment has been minimized.

@grokys

grokys Nov 10, 2017

Member

I think we can remove this block.

string path)
{
Contract.Requires<ArgumentNullException>(target != null);
return new ExpressionObserver(
ControlLocator.Track(target, RelativeSource.AncestorType, RelativeSource.AncestorLevel -1),
ControlLocator.Track(target, relativeSource.AncestorType != null ? relativeSource.Tree : TreeType.Logical, relativeSource.AncestorLevel - 1, relativeSource.AncestorType),

This comment has been minimized.

@grokys

grokys Nov 16, 2017

Member

One final thing: is relativeSource.AncestorType != null ? relativeSource.Tree : TreeType.Logical still needed here now that the visual tree can't be searched with no AncestorType?

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Nov 16, 2017

Member

Good catch! I'll get rid of that.

jkoritzinsky added some commits Nov 19, 2017

@grokys

grokys approved these changes Dec 1, 2017

@grokys grokys merged commit 7826506 into AvaloniaUI:master Dec 2, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grokys

This comment has been minimized.

Member

grokys commented Dec 2, 2017

Merged! Thanks so much for this Jeremy!

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