4

If, in a XAML file, I bind a Button to "Command" from the following class, then clicking the Button does not cause DoIt to be executed:

class Thing()
{
  public Thing(Foo p1)
  {
    Command = new RelayCommand(() => DoIt(p1));
  }

  private DoIt(Foo p)
  {
    p.DoSomething();
  }

  public ICommand Command { get; private set; }
}

However, it does work if I initialize a field from p1 and pass the field as a parameter to the method call inside the lambda:

class Thing()
{
  private Foo field;
  public Thing(Foo p1)
  {
    field = p1;
    Command = new RelayCommand(() => DoIt(field));
  }

  private DoIt(Foo p)
  {
    p.DoSomething();
  }

  public ICommand Command { get; private set; }
}

Why does the former fail, but the latter work as expected?

Probably relevant: How do closures work behind the scenes? (C#)

EDIT: To clarify, the following would also work for me. However, I would still like to know why the second example did what I expected, but the first one did not.

class Thing()
{
  private Foo field;
  public Thing(Foo p1)
  {
    field = p1;
    Command = new RelayCommand(DoIt);
    //Command = new RelayCommand(() => DoIt()); Equivalent?
  }

  private DoIt()
  {
    field.DoSomething();
  }

  public ICommand Command { get; private set; }
}
Community
  • 1
  • 1
theguy
  • 861
  • 9
  • 19

2 Answers2

4

It's an old question but I recently stumbled upon this topic and it's worth answering.

The reason for this strange behavior originates from the MVVM Light implementation of RelayCommand. The execute and canexecute handlers are stored as WeakAction _execute and WeakFunc<bool> _canExecute in the relay command. The WeakAction is an attempt to allow the GC cleanup of viewmodels when the command is still referenced by the UI for some reason.

Skipping some details, the bottom line is: assigning a viewmodel method as handler works great, because the WeakAction will stay alive as long as the viewmodel stays alive. For a dynamically created Action, the situation is different. If the only reference to that action is inside the RelayCommand, only a weak reference exists and GC can collect the action at any time, turning the whole RelayCommand into a dead brick.

Ok, time for the details. The implementation of WeakAction is not blindly storing a weak reference to the action - this would lead to many disappearing references. Instead, a combination of a weak Delegate.Target reference and an Delegate.MethodInfo is stored. For a static method, the method will be stored by strong reference.

Now, this leads to three categories of lambda:

  1. static method: () => I_dont_access_anything_nonstatic() will be stored as a strong reference
  2. closure on member variables: () => DoIt(field) the closure method will be created in the viewmodel class, the action target is the viewmodel and will stay alive as long as the viewmodel stays alive.
  3. closure on local variables: () => DoIt(p1) the closure will create a separate class instance to store the captured variables. This separate instance will be the action target and there won't be any strong reference to it - GC cleans up at some point

Important: as far as I can tell, this behavior might change with Roslyn: Delegate caching behavior changes in Roslyn so there is a chance that todays working code with case (2) turns into non-working code with Roslyn. However, I didn't test this assumption, it might work out completely different.

grek40
  • 13,113
  • 1
  • 24
  • 50
0

your Problem is that calling the Method DoIt is inside another anonymous Method created by the lamda expression. Your expression

() => DoIt(p1);

creates a anonymous Method without parameters (seen as there are no variables provided in the first braces).

I would recommend you to use the generic constructor from mvvm-light for creating the Command:

class Thing
{
    public Thing()
    {
       Command = new GalaSoft.MvvmLight.Command.RelayCommand<bool>(DoIt);
    }

    private void DoIt(bool p)
    {
       p.DoSomething(p);
    }

    public System.Windows.Input.ICommand Command { get; private set; }
}

Then just bind the Button to the "Command".

Alexx01
  • 13
  • 1
  • I don't want to accept parameters from the binding. That is, I want Thing to be given a constructor parameter, and I want the value of that parameter to be used inside DoIt. I could make DoIt parameterless, assign the constructor parameter p1 to a field, and use the field inside DoIt (rather than pass the value of the field as a parameter to DoIt), but I would like to understand exactly what is happening here. – theguy Sep 16 '15 at 13:54
  • @theguy I see. The problem is that creating the lamda expression creates a anonymous inner method which is then called. You can see it like: `public Thing(Foo p1) { Command = new RelayCommand(AnonymousMeth()); } private void AnonymousMeth() { DoIt(p1); }` therefore the method generated by the lamda expression doesn't know what to do with p1. You can (as correctly described) avoid this problem by using a private field. – Alexx01 Sep 16 '15 at 18:11