reactiveui / ReactiveUI

An advanced, composable, functional reactive model-view-viewmodel framework for all .NET platforms that is inspired by functional reactive programming. ReactiveUI allows you to abstract mutable state away from your user interfaces, express the idea around a feature in one readable place and improve the testability of your application.
https://www.reactiveui.net
MIT License
8.05k stars 1.12k forks source link

ViewModel never calls deactivation #2490

Closed wakuflair closed 3 years ago

wakuflair commented 4 years ago

Describe the bug

My ViewModel's Deactivation method never get called.

Steps To Reproduce

I followed this When Activated doc to implement a viewmodel that supports deactivation. The code is almost same with the sample in the doc:

    public class MainWindowViewModel : ReactiveObject, IActivatableViewModel
    {
        public MainWindowViewModel()
        {
            Activator = new ViewModelActivator();
            this.WhenActivated(disposables =>
            {
                InitializeSystem();

                Disposable
                    .Create(HandleDeactivation)
                    .DisposeWith(disposables);
            });
        }

        private void HandleDeactivation()
        {
            // Never called
        }

       ... 
}

I can see the InitializeSystem is called as expected. However HandleDeactivation is not.

Expected behavior

Method HandleDeactivation get alled.

Screenshots

Environment

Additional context

This is a WPF project. Nuget packages versions:

glennawatson commented 4 years ago

Are you calling WhenActivated() in the View as well as the ViewModel?

wakuflair commented 4 years ago

@glennawatson

Yeah, I did like this:

 public partial class MainWindow : IViewFor<MainWindowViewModel>
    {
        public MainWindow()
        {
            InitializeComponent();
            ViewModel = new MainWindowViewModel();

            this.WhenActivated(disposables =>
            {
                // Some standard "this.Bind" stuff
            });
        }

        object IViewFor.ViewModel
        {
            get => ViewModel;
            set => ViewModel = (MainWindowViewModel)value;
        }

        public MainWindowViewModel ViewModel { get; set; }
    }
glennawatson commented 4 years ago

ViewModel would need to be observable

wakuflair commented 4 years ago

ViewModel would need to be observable

Sorry I'm new to ReactiveUI, but what's this mean? Is implementing ReactiveObject , IActivatableViewModel not enough for deactivation?

glennawatson commented 4 years ago

DependencyProperty or a INotifyPropertyChanged for the ViewModel property.

worldbeater commented 4 years ago

MainWindow

I suspect the reason for deactivation not getting called is that the view for the main view model is the window, and when the window is closed, the application is shut down, so no need to dispose subscriptions in an app instance which is no longer running. We are currently listening Loaded, IsHitTestVisibleChanged, and Unloaded events https://github.com/reactiveui/ReactiveUI/blob/a6c5dcf4f0ce82a9c0c8b05d7c78f80cd57028d1/src/ReactiveUI.Wpf/ActivationForViewFetcher.cs so probably worth inspecting if any of these events is getting called when the window is closing.

GitHub
reactiveui/ReactiveUI
An advanced, composable, functional reactive model-view-viewmodel framework for all .NET platforms that is inspired by functional reactive programming. ReactiveUI allows you to abstract mutable st...
nathanpovo commented 3 years ago

I ran into this issue with a WPF application using ReactiveUI version 13.3.2 and .NET 5.0 (the issue also occurs on .NET 4.8).

Apparently, WPF does not raise the Unloaded event if the application is shutting down. Relevant information from the documentation page:

Note that the Unloaded event is not raised after an application begins shutting down. Application shutdown occurs when the condition defined by the ShutdownMode property occurs. If you place cleanup code within a handler for the Unloaded event, such as for a Window or a UserControl, it may not be called as expected.

The ActivationForViewFetcher class uses the Unloaded event to know that when a view must be deactivated.

The issues https://github.com/dotnet/wpf/issues/1442, and https://github.com/MvvmCross/MvvmCross/issues/3481 indicate that mvvmcross also had issues with the Unloaded event. They fixed the issue by using the Closing along with the Unloaded event to determine that a viewmodel should be deactivated.

Based on this question on stackoverflow (that asks for the best way to dispose of WPF controls) I have opted to deactivate my view (which will, in turn, deactivate its viewmodel) by using the Dispatcher.ShutdownStarted event as follows:

using System;
using System.Reactive.Disposables;
using System.Windows.Threading;
using ReactiveUI;

namespace YourNamespaceHere
{
    public partial class MainWindow
    {
        private readonly IDisposable activator;

        public MainWindow()
        {
            InitializeComponent();

            activator = this.WhenActivated(disposables =>
            {
                ViewModel ??= new MainViewModel();

                Disposable
                    .Create(HandleDeactivation)
                    .DisposeWith(disposables);

                this.Dispatcher.Events()
                    .ShutdownStarted
                    .Subscribe(_ => activator?.Dispose())
                    .DisposeWith(disposables);
            });
        }

        private void HandleDeactivation() { }
    }
}

Note that this requires the ReactiveUI.Events.WPF package.

worldbeater commented 3 years ago

@nathanpovo thanks for the detailed description of the solution! Probably we should subscribe to the Closing event in WPF's IActivationForViewFetcher like we are already doing this in e.g. Avalonia. We'd be happy to accept a PR with a fix I guess

GitHub
reactiveui/ReactiveUI
An advanced, composable, functional reactive model-view-viewmodel framework for all .NET platforms that is inspired by functional reactive programming. ReactiveUI allows you to abstract mutable st...
nathanpovo commented 3 years ago

@worldbeater, I'd be happy to submit a PR to fix the issue.

glennawatson commented 3 years ago

@nathanpovo go ahead and submit a PR, I have a release going out today but can do a point release with the fix later this week.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.