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

feat: synchronize ViewModel and BindingContext properties #1270

Merged
merged 3 commits into from Feb 19, 2017

Conversation

Projects
None yet
6 participants
@kentcb
Copy link
Contributor

kentcb commented Feb 13, 2017

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature.

What is the current behavior? (You can also link to an open issue here)

#1256

What is the new behavior (if this is a feature change)?

  1. Whenever the BindingContext changes on a reactive view, the ViewModel is updated accordingly. However, if the BindingContext is set to a non-null instance of a different type of object, the VM is not updated (allowing for ViewModel and BindingContext to deviate where desirable).
  2. Whenever the ViewModel property is assigned by RoutedViewHost or ViewModelViewHost, the BindingContext is also set to the same.

Does this PR introduce a breaking change?

No.

Please check if the PR fulfills these requirements

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 13, 2017

Coverage Status

Coverage remained the same at 65.938% when pulling 7ee30af on kentcb:Issue1256 into 8e9963f on reactiveui:develop.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 13, 2017

Coverage Status

Coverage remained the same at 65.938% when pulling 42dfbaf on kentcb:Issue1256 into 8e9963f on reactiveui:develop.

@Qonstrukt

This comment has been minimized.

Copy link
Member

Qonstrukt commented Feb 13, 2017

Any reasons you didn't do this the other way around?
IMHO, I'd expect the BindingContext to be set whenever I assign an ViewModel, because I'm working in a project with RxUI I'm more inclined to set a ViewModel than a BindingContext.

When setting up a controller in a non-Forms project I'd set up bindings, assign a ViewModel and be done with it. With this implementation I'd set a ViewModel, and nothing happens because I didn't explicitly set a BindingContext. So this should at least be very well documented.

@kentcb

This comment has been minimized.

Copy link
Contributor Author

kentcb commented Feb 13, 2017

@Qonstrukt TBH, I hoped someone would raise it so we could have the discussion :)

I was approaching this from the other side. Normally I end up with this crap in my XAML:

<ListView>
    <ListView.ItemTemplate>
        <me:SomeView ViewModel="{Binding .}"/>
    </ListView.ItemTemplate>
</ListView>

That is, the XAML runtime has assigned the BindingContext to SomeView, and I need to manually propagate it to ViewModel. I would prefer that setting either BindingContext or ViewModel propagates to the other:

public TViewModel ViewModel 
{
    get { return (TViewModel)GetValue(ViewModelProperty); }
    set
    {
        SetValue(ViewModelProperty, value);
        // would actually do this via the BindableProperty metadata so that we don't get recursion and so it works when not using the CLR property, but this is just for demonstration purposes
        this.BindingContext = value;
    }
}

protected override void OnBindingContextChanged()
{
    base.OnBindingContextChanged();
    this.ViewModel = this.BindingContext as TViewModel;
}

However, the drawback is that it's potentially breaking. If there is existing code out there that relies on having distinct values for ViewModel and BindingContext, that code will break. Probably a good thing in my opinion, since it seems an awful practice, but there you go.

To avoid the breaking change, we could use the code I've already committed as well as the change to the ViewModel property above, but have it check that the BindingContext is not already set to some other object of a type other than TViewModel. Frankly, it gets a bit messy and somewhat confusing for users I think. I'd rather just say BindingContext propagates to ViewModel and vice-versa.

If we can all agree this is the most desirable approach, I'm all for it.

@Qonstrukt

This comment has been minimized.

Copy link
Member

Qonstrukt commented Feb 13, 2017

Fair points 😊

I agree that having the BindingContext and ViewModel properties be equal to each other for most of cases should be preferred, whichever way you set them.

The only case I can imagine is your ViewModel having a property which defines the BindingContext for your View. That's why I'd prefer to have the ViewModel set the BindingContext and not the other way around.

It'd be interesting to hear what others like @escamoteur expect who use Forms a lot more than I do.

@kentcb

This comment has been minimized.

Copy link
Contributor Author

kentcb commented Feb 13, 2017

Agreed - further input appreciated.

I was also just thinking that we should probably look to the WPF/UWP RxUI implementations for guidance here, as they have exactly the same problem. I'll look into the implementation tomorrow.

@PureWeen

This comment has been minimized.

Copy link
Contributor

PureWeen commented Feb 13, 2017

On all my Xamarin Forms Views I set it both places
VM => BindingContext when VM is set
BindingContext => VM when OnBindingContextChanged occurs

I found if I try to be aware of the scenarios of when to do each I just mess it all up or forget to wire the BindingContext back to the VM in cases like a ListView. It's easier going in just knowing those are going to be set.

Plus I think when you start implementing RxUI one just assumes that's how it's going to work and the BindingContext will be set. Especially during cases of VM navigation. The detachment of the BindingContext from the VM is a little confusing first and will probably be a little more confusing if it works one way and not the other

The ViewModel is really just there to be a typed BindingContext so you get a FluentApi over the bindings right? It doesn't really offer any additional behavior/purpose does it? So it seems like the two should mirror each other.

@escamoteur

This comment has been minimized.

Copy link

escamoteur commented Feb 13, 2017

I agree with Shane 100% I always see ViewModel as the same as the Forms BindingContext and so far never had the need to use the two inf different ways. That's one reason that we added an XamvvmRxUI to xamvvm that autoassigns the ViewModel to the ViewModel instead of the BindingContext property duringpage creation and VM navigation.

If RxUI fpr Forms woudl do this automatically (maybe by use Fody to automatically make ViewModel asn alias of BindingContext would be amazing.

Another important pint here is that in Forms the BindingContext automatically get's forwarded to the childviews. That's something ViewModel should also mirror.

@GiusepeCasagrande

This comment has been minimized.

Copy link

GiusepeCasagrande commented Feb 13, 2017

I see no reason to make ViewModel differ from BindContext and I believe that we should discourage this.
As I see this, if you need a ViewModel other then what is set in the BindingContext (and the other way around), something is pretty wrong with you design/code.

So I'm all in for the "breaking" solution.

@escamoteur

This comment has been minimized.

Copy link

escamoteur commented Feb 13, 2017

Agreed actually this would prevent people from doing crazy stuff

@kentcb

This comment has been minimized.

Copy link
Contributor Author

kentcb commented Feb 14, 2017

OK, I've just pushed an implementation as discussed. If people could try it out on their projects, that would be much appreciated.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 14, 2017

Coverage Status

Coverage remained the same at 65.938% when pulling 0e8fd36 on kentcb:Issue1256 into 8e9963f on reactiveui:develop.

@escamoteur

This comment has been minimized.

Copy link

escamoteur commented Feb 15, 2017

Just had a look at the code. Important for this to work is that if someone overrides the OnBindindContextChanged he has to call the base implementation. Is it possible to issue a Warning if an overriding Method does not call the base?

@escamoteur

This comment has been minimized.

Copy link

escamoteur commented Feb 15, 2017

Another thought. The way Kent solved this problem now only works when you derive from one of this base classes if you just use IViewFor<> you get no support.
Would there be another solution for this? Bringing Fody into play again? Or require to implement somethink like The ViewModelActivator to do the assigning. As BindingContext itself is a BindableProperty it should be possible to register for changes or not?

I have no real solution right now but just changing the base classes seems not the right solution for me.

@kentcb

This comment has been minimized.

Copy link
Contributor Author

kentcb commented Feb 15, 2017

Is it possible to issue a Warning if an overriding Method does not call the base?

Not to my knowledge, but it's generally considered poor practice to override a method and neglect to call the base implementation. And the IDE helps out here by filling in the base call for you when you override.

As BindingContext itself is a BindableProperty it should be possible to register for changes or not?

WPF includes the DependencyPropertyDescriptor class, which allows you to hook into a dependency property's change notifications externally from the implementation class itself. As best I can tell, XF has no equivalent. Thus, the only option is for the change handler to be triggered from within the implementation class itself.

Bringing Fody into play again?

I don't know that Fody is an option as a dependency for RxUI itself. Certainly I wouldn't be too keen on having the IL rewritten.

@PureWeen

This comment has been minimized.

Copy link
Contributor

PureWeen commented Feb 16, 2017

AFAIK all Bindable Property changes flow through the OnPropertyChanged events

https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Core/BindableObject.cs#L588

So in theory I think one could subscribe to PropertyChanged in the constructor and then look for "ViewModel" or "BindingContext"

Though I think I would also lean on this perspective

Not to my knowledge, but it's generally considered poor practice to override a method and neglect to call the base implementation. And the IDE helps out here by filling in the base call for you when you override.

If you override something and don't call the base implementation and things start acting weird then that's on you :-)

@escamoteur

This comment has been minimized.

Copy link

escamoteur commented Feb 16, 2017

OK that's the one point, the more important one for me that this will require to use one of the base classes. So if I have already a forms base class I'm f*****
I don't think Fody would be so bad. Realm for instance realizes it's whole c# binding this way

@Qonstrukt

This comment has been minimized.

Copy link
Member

Qonstrukt commented Feb 17, 2017

To be honest I think that's a never ending fight. If your base classes extend from the RxUI base classes you're ok. But if you're using base classes of some other framework, it's the choice you made, and you should implement your own combination.

I'm currently looking into combining functionality of MvvmCross and RxUI Activities and ViewControllers, and no-doubt I'll have to make some compromises there as well. Luckily @aritchie made a start with this already here: https://github.com/aritchie/mvxgoodies. But at some point you'll still have to make a choice for which implementation is more important to you.

Stuff like Fody is a nice bonus, there's no reason why there couldn't be a base class and a Fody attribute which produces the same end-result, but that belongs in a separate package IMHO.

@escamoteur

This comment has been minimized.

Copy link

escamoteur commented Feb 17, 2017

I don not completely agree and I thing Forms is special in that it already provides a Binding Infrastructur only that it uses another propterty for Bindingcontextes as RxUI so IMHO to have an automatic mechanism included in the Forms Verison of RxUI would be great.

@Qonstrukt

This comment has been minimized.

Copy link
Member

Qonstrukt commented Feb 17, 2017

But isn't that what this PR exactly accomplishes? 😉
But only if you use the Reactive*Page/View classes.

@escamoteur

This comment has been minimized.

Copy link

escamoteur commented Feb 17, 2017

yes and in that I see a problem because if you get a new Page or View base class from some other Library like PopupPages or SlideOverkit you have to care about it yourself. In making adoption of RxUIForms easy it would be great to have other solutions as well. Still thinking if Foody wouldn't be the best solution. You won't even need an Attribute because it could look for the IViewFor attribute

Unfortunately writing Foody is way above my knowledge

@kentcb

This comment has been minimized.

Copy link
Contributor Author

kentcb commented Feb 18, 2017

Fody has its own ecosystem of add-ins. Such an add-oin is outside the scope of this PR, and doesn't even belong in the RxUI code base. If you wanted to PR something, I suspect the best place to do so would be here.

As @Qonstrukt mentioned, if you step away from what the framework provides, then it's up to you to fill in any gaps.

@kentcb

This comment has been minimized.

Copy link
Contributor Author

kentcb commented Feb 18, 2017

OK, I've tried this locally and it all looks good. If there are no objections, I'll merge this tomorrow.

@kentcb kentcb changed the title Auto-assign VM from BindingContext, and vice-versa feat: synchronize ViewModel and BindingContext properties Feb 19, 2017

@kentcb kentcb added this to the vNext milestone Feb 19, 2017

@kentcb kentcb merged commit eb7a36c into reactiveui:develop Feb 19, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage remained the same at 65.938%
Details

GiusepeCasagrande pushed a commit that referenced this pull request Oct 10, 2017

fix: remove ViewModel="{Binding .}" from Cinephile
Since this was fixed #1270 we don't need to relly on a ViewModel="{Binding .}" anymore.

ghuntley added a commit that referenced this pull request Oct 10, 2017

fix: remove ViewModel="{Binding .}" from Cinephile (#1509)
Since this was fixed #1270 we don't need to relly on a ViewModel="{Binding .}" anymore.

ghuntley added a commit that referenced this pull request Nov 6, 2017

fix: remove ViewModel="{Binding .}" from Cinephile (#1509)
Since this was fixed #1270 we don't need to relly on a ViewModel="{Binding .}" anymore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment