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

View / ViewModel Activation #503

Merged
merged 20 commits into from
Jan 28, 2014
Merged

View / ViewModel Activation #503

merged 20 commits into from
Jan 28, 2014

Conversation

anaisbetts
Copy link
Member

Consider the following ViewModel constructor:

public MyBrokenViewModel()
{
    UserError.RegisterHandler(x => {
        // NB: Stuff
    });
}

This is broken because every time we create MyBrokenViewModel, we create another error handler. What Do? What we really want for certain global things like UserError, is for UserError to be subscribed only when the View associated with the ViewModel is visible. However, that information isn't available to ViewModels, and even if it was, it's not super obvious. Let's fix it

How does this work:

Activation allows you, for both Views and ViewModels, to set up the things that should be active when the View is visible. Here's how you do it for ViewModels:

public class MyWorkingViewModel : ReactiveObject, ISupportsActivation
{
    public ViewModelActivator Activator { get; protected set; }

    public ActivatingViewModel()
    {
        Activator = this.WhenActivated(d => {
            // d() registers a Disposable to be cleaned up when
            // the View is deactivated / removed
            d(UserError.RegisterHandler(x => {
                // NB: Stuff
            }));
        });
    }
}

Here's how it works for Views:

public class MyWorkingView : UserControl, IViewFor<MyWorkingViewModel>
{
    public ActivatingView()
    {
        this.WhenActivated(d => {
            Console.WriteLine("Helloooooo Nurse!")
            d(Disposable.Create(() => Console.WriteLine("Goodbye, Cruel World")));
        });
    }
}

Note that calling WhenActivated in a View automatically means that the associated ViewModel gets notified for activated / deactivated changes (and in fact, you must call WhenActivated in the View to get the ViewModel to be notified).

TODO:

@haacked
Copy link
Contributor

haacked commented Jan 24, 2014

The design of this seems like it might be lacking in discoverability. Maybe having d be an object with useful methods might make things more discoverable. Then you could do things like:

Activator = this.WhenActivated(d => {
            // d() registers a Disposable to be cleaned up when
            // the View is deactivated / removed
            d.DisposeWhenDeactivated(UserError.RegisterHandler(x => {
                // NB: Stuff
            }));
        });

And

this.WhenActivated(d => {
            Console.WriteLine("Helloooooo Nurse!")
            d.DisposeWhenDeactived(() => Console.WriteLine("Goodbye, Cruel World")));
        });

That's a little more readable and the nice thing is we can have overloads of DisposeWhenDeactivated that do the right thing. In the second example, it's calling Disposable.Create and adding it to the underlying d.

@jen20 jen20 mentioned this pull request Jan 27, 2014
4 tasks
@anaisbetts
Copy link
Member Author

Alright, calling all API designers - here's how this API looks now:

public class ActivatingViewModel : ReactiveObject, ISupportsActivation
{
    public ViewModelActivator Activator { get; protected set; }

    public int IsActiveCount { get; protected set; }

    public ActivatingViewModel()
    {
        Activator = new ViewModelActivator();

        this.WhenActivated(d => {
            IsActiveCount++;
            d(Disposable.Create(() => IsActiveCount--));
        });
    }
}

What could this look like?

/cc @reactiveui/owners

@jlaanstra
Copy link
Member

I actually like the suggestion by @haacked. I also think there is value in an WhenInitialized that is called only the first time the object gets activated.

Also why do I need the ViewModelActivator? Seems like it is unused now. Maybe it makes sense to have the WhenActivated method on the ViewModelActivator?

@jen20
Copy link
Contributor

jen20 commented Jan 27, 2014

To be honest, it seems quite reasonable as it is. Making it an object with a method would make the declarations of things which need disposing a lot more verbose, made more important since you have to do use this construct to dispose of bindings.

At least with a lambda you get to choose your level of verbosity.

@half-ogre
Copy link

. At least with a lambda you get to choose your level of verbosity.

This is why I favor just the action and not an object with a method as well. Between mousing over the value and help via IntelliSense, I think it's plenty discoverable. You can also make d more verbose in docs and samples, something like this.WhenActivated(disposeWhenDeactivated => { ....

@haacked
Copy link
Contributor

haacked commented Jan 27, 2014

Hmm, this whole discussion started because of error handling registration. It seems to me the real problem is we're calling global error registration in a ViewModel ctor. Maybe the solution is to change the way we register error handlers to take into account the view model. For example, what if this was more like?

public static void RegisterErrorHandler(this ISupportsActivation activation) {
  // This does it the better way
  ...
}

public static void RegisterErrorHandler(this ReactiveObject reactiveObject) {
  var activatable = reactiveObject as ISupportsActivation;
  if (activatable != null) {
    activatable.RegisterErrorHandler();
    return;
  }
 // Do it the old way.
  UserError.RegisterHandler(...);
}

Then in a view model:

public class MyFixedViewModel : ReactiveObject, ISupportsActivation
{
  public MyBrokenViewModel()
  {
    this.RegisterErrorHandler(x => {
        // NB: Stuff
    });
  }
}

The idea here is you always call this.RegisterErrorHandler but if your view model implements ISupportsActivation shit's handled for you.

@jen20
Copy link
Contributor

jen20 commented Jan 28, 2014

@haacked That works for the ViewModel and registering error handlers, but you also have to use the same for bindings in the view:

public class MyView : IViewFor<MyViewModel>
{
    public MyView() {
        InitializeComponent();

        this.WhenActivated(d => {
            d(this.OneWayBind(ViewModel, vm => vm.SomeValue, v => v.SomeTextBlock.Text));
            d(this.OneWayBind(ViewModel, vm => vm.SomeOtherValue, v => v.SomeOtherTextBlock.Text));
        });
    }
    //Implementation of gunk for VM dependency property here
}

It would be a bit weird if there was a different syntax for activation in the View to the ViewModel (though I agree it might be a good idea to specialise for registering the error handler which likely most VMs will want to do).

@haacked
Copy link
Contributor

haacked commented Jan 28, 2014

but you also have to use the same for bindings in the view:

@jen20 yeah, that should be made clear in the issue.

It would be a bit weird if there was a different syntax for activation in the View to the ViewModel

I'm not suggesting that. Let's not push the method of activation to the consumer. Let's focus on providing an easy way to register things that is automatically handled when activation is supported.

Underneath the hood, it could be the same approach. But for end users, it's much simpler.

For example, what if we did this for all the bind methods (pseudocode)?

public static void Bind(this whateverItIs, ....) {
   if whateveveritIs is ISupportsActivation then
      Bind this in a manner that doesn't leak
}

@jen20
Copy link
Contributor

jen20 commented Jan 28, 2014

@haacked That's actually a pretty nice idea, would silently fix people's binding code :-)

@anaisbetts
Copy link
Member Author

Actually one of the neat side effects of 5618a7a is that you can have more than one WhenActivated block in the ctor, so these kinds of binding tricks can totally be done.

Let's stick with the current design for WhenActivated for now, but I really want to steal @haacked's ideas for Binds and Error Registration stuff.

@ramonahosu
Copy link

Hello,

What do you recommend to use for activating the view models only when controls are made visible? (other than subscribe on VisibleChanged)

By checking the implementations of IactivationForViewFetcher for both winforms and wpf, I can see that activation can happen even if a control does not become visible.
Eg. For Winforms, when having a user control that is not visible by default, the activation happens on HandleCreated..
Eg. For WPF activation will happen for user controls that are not visible due to activation on Load.

Regards

@ghuntley ghuntley deleted the activation branch August 14, 2017 09:06
@lock lock bot locked and limited conversation to collaborators Jun 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants