none
ICommand.CanExecute don`t stop after window closed

    Question

  • In my app i have a menu, from which i open some child window. On that window i have button with Command property binded to viewmodel. Command is DelegateCommand instance.
    All work fine, but when i closed child window, CanExecute of that Command dont stop to check. 
    I cant find why it happens.
    Here is small sample:


    Child window markup:


    <Window x:Class="Realization.Windows.ContentDialogWindow"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml">
        <Grid>
            <Button Content="Ok" Command="{Binding OkCommand}"/>
        </Grid>
    </Window>
    


    Child window viewmodel:


        public class MobDlgVM 
        {
            public ICommand OkCommand { get; set; }
    
            public MobDlgVM()
            {
                OkCommand = new DelegateCommand(() => { }, CanExecute);
            }
    
            private bool CanExecute()
            {
                return true;
            }
        }
    


    Running from main window:


    ContentDialogWindow win = new ContentDialogWindow();
    win.DataContext = new MobDlgVM();
    win.ShowDialog();
    
    


    Can anybody help me to find solution ?






    • Edited by Sirga Tuesday, December 22, 2009 9:37 AM
    Tuesday, December 22, 2009 8:44 AM

Answers

  • Hi,

    the ViewModel that is the DataContext of the Child window is created in the Parent window. Does the Parent window or its ViewModel hold any references to the Child's ViewModel while the Child is open? If yes, this could explain the behaviour:

    The Child window registers event handlers on the DataContext in order to listen to change notifications. The delegate which is the event handler contains a reference to the Child window. When the Child window is closed, its ViewModel isn't released, since the Parent window still owns a reference. Since it isn't released, the ViewModel keeps the Child window alive through the reference contained in the event handler. If you set the DataContext to null, the change event handler on the ViewModel is removed, so the Child window is freed.

    Do you think it could be like this?
    http://wpfglue.wordpress.com
    Wednesday, December 23, 2009 5:52 PM
  • Hi,

    I think I found out what happens. The world is ok, the MVVM gurus are right (how could I doubt it), and I should have tried it out myself in the first place instead of relying on articles...

    So: I changed the code of the child window to this:

        Private Shared _Count As Integer = 0
        Private _Index As Integer
        Public Sub New()
            MyBase.New()
            InitializeComponent()
            _Count += 1
            _Index = _Count
            Debug.Print("{0} {1} Constructed", Me.GetType.Name, _Index)
        End Sub
    
        Protected Overrides Sub Finalize()
            Debug.Print("{0} {1} Finalized", Me.GetType.Name, _Index)
            MyBase.Finalize()
        End Sub
    
    
        Private Class Handler
    
            Private Shared _Count As Integer = 0
            Private _Index As Integer
            Public Sub New()
                MyBase.New()
                _Count += 1
                _Index = _Count
                Debug.Print("{0} {1} Constructed", Me.GetType.Name, _Index)
            End Sub
            Protected Overrides Sub Finalize()
                Debug.Print("{0} {1} Finalized", Me.GetType.Name, _Index)
                MyBase.Finalize()
            End Sub
    
            Public Sub OnRequerySuggested(ByVal sender As Object, ByVal e As EventArgs)
                Debug.Print("RequerySuggested fired")
            End Sub
    
            Public Function CanExecute(ByVal parameter As Object) As Boolean
                Debug.Print("{0}.CanExecute called on instance {1}.", Me.GetType.Name, _Index)
                Return True
            End Function
    
            Public Sub Execute(ByVal parameter As Object)
                Debug.Print("{0}.Execute called on instance {1}.", Me.GetType.Name, _Index)
            End Sub
        End Class
    
        Private Sub AddCommand_Click(ByVal sender As System.Object, ByVal e As System.Windows.RoutedEventArgs)
            Dim handler As Handler = New Handler
            Dim command As RelayCommand = New RelayCommand(AddressOf handler.Execute, AddressOf handler.CanExecute)
            CommandButton.Command = command
        End Sub
    
    

    I didn't set the DataContext, just made a button which adds a RelayCommand, which calls methods on a class that has instance tracking, i.e. it prints debug messages when it is constructed or finalized.

    Second, I added a button on the parent window which calls GC.Collect.

    What happened then was that if I opened the child window with the first button, added the command handler, and closed the window, the CanExecute event would keep firing until the window, the command and the handler were garbage collected. If I didn't set the DataContext, this garbage collection would happen automatically after a few seconds, if I set the DataContext to contain the handler and didn't release it before closing the window, I would have to force garbage collection with the GC button, but then CanExecute would stop firing.

    So: it looks like the CommandManager has no clue that it should stop firing CanExecute until the handler gets garbage collected (which is reasonable if it uses the WeakEvent pattern), and garbage collection seems to take longer if the DataContext contains a reference to the handler when the reference to the window is released. (maybe something about first and second generation garbage collection, or so...)
    http://wpfglue.wordpress.com
    Thursday, December 24, 2009 11:24 AM

All replies

  • I found that "Remove" section of CanExecuteCnanged event dont riched after window was closed.. But i dont understand why..


            public event EventHandler CanExecuteChanged
            {
                add
                {
                    if (!_isAutomaticRequeryDisabled)
                    {
                        CommandManager.RequerySuggested += value;
                    }
                }
                remove
                {
                    if (!_isAutomaticRequeryDisabled)
                    {
                        CommandManager.RequerySuggested -= value;
                    }
                                }
            }
    

    Tuesday, December 22, 2009 8:53 AM
  • shouldn't you change the code as follows?
    public MobDlgVM()
    {
      if(OkCommand==null)
      {
        OkCommand = new DelegateCommand(() => { }, CanExecute);
      }
    }
    Tuesday, December 22, 2009 9:39 AM
  • Why?

    How it can be not null in constructor?
    Tuesday, December 22, 2009 9:42 AM
  • Hi,

    does the child window register any other eventhandlers? It is a known issue that if the Child window registers event handlers with the parent window and doesn't unregister them, it is not disposed after being hidden.

    About the Remove section never being called: This should only happen if the command on the button is set to null explicitly. If the button is disposed and destroyed, or all of the model, the window and with it the button are garbage collected, the remove section will probably not be called.
    http://wpfglue.wordpress.com
    Tuesday, December 22, 2009 11:04 AM
  • Hi,

    does the child window register any other eventhandlers? 

    Only one - Button.Click

    Codebehind fo child window:

        public partial class ContentDialogWindow : Window
        {
            public ContentDialogWindow()
            {
                InitializeComponent();
            }
    
            private void Button_Click(object sender, RoutedEventArgs e)
            {
                DialogResult = true;
            }
        }
    





    Tuesday, December 22, 2009 11:17 AM
  • I found that if I set DataContext of child window to NULL explicitly after window was closed, the issue goes away.
    How it can be explained?
    Wednesday, December 23, 2009 7:09 AM
  • Hi,

    the ViewModel that is the DataContext of the Child window is created in the Parent window. Does the Parent window or its ViewModel hold any references to the Child's ViewModel while the Child is open? If yes, this could explain the behaviour:

    The Child window registers event handlers on the DataContext in order to listen to change notifications. The delegate which is the event handler contains a reference to the Child window. When the Child window is closed, its ViewModel isn't released, since the Parent window still owns a reference. Since it isn't released, the ViewModel keeps the Child window alive through the reference contained in the event handler. If you set the DataContext to null, the change event handler on the ViewModel is removed, so the Child window is freed.

    Do you think it could be like this?
    http://wpfglue.wordpress.com
    Wednesday, December 23, 2009 5:52 PM
  • Hi,

    the ViewModel that is the DataContext of the Child window is created in the Parent window. Does the Parent window or its ViewModel hold any references to the Child's ViewModel while the Child is open?
    No, the reference to child viewmodel doesn`t stored anywhere. Below the complete example that shows how child viewmodel was created and window was showed. As you can see, it in local method, that handles button click event. So after it finished, all local object must be destroyed. 

    private void button1_Click(object sender, RoutedEventArgs e)
    {
        ContentDialogWindow win = new ContentDialogWindow();
        win.DataContext = new MobDlgVM();
        win.ShowDialog();
    }
    

    You can make a simple project and try it yourself. 
    Wednesday, December 23, 2009 9:45 PM
  • Hi,

    you might want to look at this article:

    http://blogs.msdn.com/jgoldb/archive/2008/02/04/finding-memory-leaks-in-wpf-based-applications.aspx

    Anyway, I'll try it myself when I find the time...

    Found this, too,

    http://blogs.msdn.com/nathannesbit/archive/2009/05/29/wpf-icommandsource-implementations-leak-memory.aspx

    which seems to describe exactly the problem you experience. So, my advice would be: use a RoutedCommand.
    http://wpfglue.wordpress.com
    Thursday, December 24, 2009 8:34 AM
  • http://blogs.msdn.com/nathannesbit/archive/2009/05/29/wpf-icommandsource-implementations-leak-memory.aspx

    which seems to describe exactly the problem you experience. So, my advice would be: use a RoutedCommand.
    Actually no.. Because the DelegateCommand class uses the same approach as RoutedCommand. 

    I.e. "RoutedCommand.CanExecuteChanged implementation is to be a proxy for CommandManager.RequerySuggested"

    public event EventHandler CanExecuteChanged
    {
         add
         {
             if (!_isAutomaticRequeryDisabled)
             {
                 CommandManager.RequerySuggested += value;
             }
         }
         remove
         {
             if (!_isAutomaticRequeryDisabled)
             {
                 CommandManager.RequerySuggested -= value;
             }
         }
    }




    Thursday, December 24, 2009 9:37 AM
  • Hi again,

    I've been playing around with it a little. I'm using VB and the RelayCommand from this article:

    http://www.codeproject.com/KB/WPF/InternationalizedWizard.aspx

    I believe the implementation is the same as DelegateCommand.

    I tried to change the implementation so that the CommandManager doesn't hold on to the Button directly anymore. This stopped the CanExecute Method from being called, but now the CommandManager holds on to the RelayCommand instead, which in turn holds on to the ViewModel, so that ViewModel instances are leaking. I tried to resolve this by using IDisposable, but no luck so far. Even if I call Dispose explicitly and remove the event handlers for CommandManager.RequerySuggested, the ViewModel stays alive.

    My conclusion is that one shouldn't use RelayCommands or DelegateCommands, which is quite a shock, since I figured all the MVVM gurus who are recommending them as the standard way in MVVM should have noticed this problem a long time ago...

    By the way, I'm just working on a Commanding Framework which allows it to create and manipulate RoutedCommands easily in XAML. It also comes with different flavours of command delegation, like just calling a method on an object using a set of bindable parameters (basically wrapping ObjectDataProvider and causing it to fire on an Executed event), or to delegate to an existing ICommand implementation. You might want to check out my blog after the holidays.
    http://wpfglue.wordpress.com
    Thursday, December 24, 2009 9:50 AM
  • Hi again,

    I tried to change the implementation so that the CommandManager doesn't hold on to the Button directly anymore. 

    I know that CommandManager implements WeakEvent pattern and stores weak reference to eventhandlers, so i think it cant be the cause of issue.


    Thursday, December 24, 2009 10:03 AM
  • Hi,

    from the article I had the impression that CommandManager broke the pattern. However, I just checked it, and at least the RequerySuggested event doesn't leave any dangling references. This gets more and more interesting, I am still researching.
    http://wpfglue.wordpress.com
    Thursday, December 24, 2009 10:39 AM
  • Hi,

    I think I found out what happens. The world is ok, the MVVM gurus are right (how could I doubt it), and I should have tried it out myself in the first place instead of relying on articles...

    So: I changed the code of the child window to this:

        Private Shared _Count As Integer = 0
        Private _Index As Integer
        Public Sub New()
            MyBase.New()
            InitializeComponent()
            _Count += 1
            _Index = _Count
            Debug.Print("{0} {1} Constructed", Me.GetType.Name, _Index)
        End Sub
    
        Protected Overrides Sub Finalize()
            Debug.Print("{0} {1} Finalized", Me.GetType.Name, _Index)
            MyBase.Finalize()
        End Sub
    
    
        Private Class Handler
    
            Private Shared _Count As Integer = 0
            Private _Index As Integer
            Public Sub New()
                MyBase.New()
                _Count += 1
                _Index = _Count
                Debug.Print("{0} {1} Constructed", Me.GetType.Name, _Index)
            End Sub
            Protected Overrides Sub Finalize()
                Debug.Print("{0} {1} Finalized", Me.GetType.Name, _Index)
                MyBase.Finalize()
            End Sub
    
            Public Sub OnRequerySuggested(ByVal sender As Object, ByVal e As EventArgs)
                Debug.Print("RequerySuggested fired")
            End Sub
    
            Public Function CanExecute(ByVal parameter As Object) As Boolean
                Debug.Print("{0}.CanExecute called on instance {1}.", Me.GetType.Name, _Index)
                Return True
            End Function
    
            Public Sub Execute(ByVal parameter As Object)
                Debug.Print("{0}.Execute called on instance {1}.", Me.GetType.Name, _Index)
            End Sub
        End Class
    
        Private Sub AddCommand_Click(ByVal sender As System.Object, ByVal e As System.Windows.RoutedEventArgs)
            Dim handler As Handler = New Handler
            Dim command As RelayCommand = New RelayCommand(AddressOf handler.Execute, AddressOf handler.CanExecute)
            CommandButton.Command = command
        End Sub
    
    

    I didn't set the DataContext, just made a button which adds a RelayCommand, which calls methods on a class that has instance tracking, i.e. it prints debug messages when it is constructed or finalized.

    Second, I added a button on the parent window which calls GC.Collect.

    What happened then was that if I opened the child window with the first button, added the command handler, and closed the window, the CanExecute event would keep firing until the window, the command and the handler were garbage collected. If I didn't set the DataContext, this garbage collection would happen automatically after a few seconds, if I set the DataContext to contain the handler and didn't release it before closing the window, I would have to force garbage collection with the GC button, but then CanExecute would stop firing.

    So: it looks like the CommandManager has no clue that it should stop firing CanExecute until the handler gets garbage collected (which is reasonable if it uses the WeakEvent pattern), and garbage collection seems to take longer if the DataContext contains a reference to the handler when the reference to the window is released. (maybe something about first and second generation garbage collection, or so...)
    http://wpfglue.wordpress.com
    Thursday, December 24, 2009 11:24 AM
  • So: it looks like the CommandManager has no clue that it should stop firing CanExecute until the handler gets garbage collected (which is reasonable if it uses the WeakEvent pattern), and garbage collection seems to take longer if the DataContext contains a reference to the handler when the reference to the window is released. (maybe something about first and second generation garbage collection, or so...)
    http://wpfglue.wordpress.com

    Your investigation is interesting, but doesn`t make it obvious for me why child window and viewmodel doesn`t become garbage collected after they left their execution scope and not reachable from application anymore. And why its true only if child windows datacontext was set..

    Thursday, December 24, 2009 12:07 PM
  • Hi,

    I think they become garbage collected, it just takes longer... I could only speculate about the reasons.

    Anyway, Merry Christmas to you, in case you celebrate it.
    http://wpfglue.wordpress.com
    Friday, December 25, 2009 6:59 AM
  • Hi,

    I think they become garbage collected, it just takes longer... I could only speculate about the reasons.

    Anyway, Merry Christmas to you, in case you celebrate it.

    Thank you.
    You right, they become... but time they stay alive is unpredictable. It may be from seconds to many minutes. And until they collected their handlers works. 
    So the best idea i found is to set datacontext to null after window closed and thus unsubscribe from CanExecuteChanged.
    Friday, December 25, 2009 10:11 AM