c# - Messagebox inside Lambda expression

Question

Well I have as the title describes: a Lambda expression with a Messagebox inside it that doesn't work awfully well.

My project is WPF, using C# and MVVM all in Visual Studio 2010.

This starts with a context menu, which is as follows:

<ContextMenu x:Key="ChatNodeMenu" >
            <MenuItem Header="Remove ChatNode" Command="{Binding RelativeSource={RelativeSource FindAncestor, AncestorType={x:Type ContextMenu}}, Path=PlacementTarget.Tag.DataContext.RemoveChatNodeCommand}" />

            <Separator/>
            <MenuItem Header="Add branching for mission complete:" ItemsSource="{Binding ChatNodeListViewModel.DraggableNodeAddMissionList, Source={StaticResource Locator}}">
                <MenuItem.ItemContainerStyle>
                    <Style>
                        <Setter Property="MenuItem.Header" Value="{Binding DisplayName}"/>
                        <Setter Property="MenuItem.Command" Value="{Binding ContextMenuCommand}"/>
                    </Style>
                </MenuItem.ItemContainerStyle>
            </MenuItem>
        </ContextMenu>

The detail is really about the second item, which gets its information from a list, called DraggableNodeAddMissionList, which is an ObservableCollection of a type which holds a String and a RelayCommand.

When said list is populated, it runs an event which is as follows:

DraggableNodeAddMissionList.Clear();
                foreach(Mission m in Database.Instance.Missions)
                {
                    Mission m2 = m;

                    DraggableNodeAddMissionList.Add(new ContextMenuVM()
                    {
                        DisplayName = m2.MissionName,
                        ContextMenuCommand = new RelayCommand(
                            () =>
                            {
                                MessageBox.Show("You clicked!" + m2.MissionName);
                            })
                    });
                }

So, as you can see, there is a String (DisplayName) and the RelayCommand (ContextMenuCommand).

It works fine and the context menu is populated as I would expect from the list. You can click on each item.

Now to the circumstances of the error: if I simply have a Messagebox which just shows "You Clicked" (without the addition of the Mission Name), it works every time.

When I add in the addition of the Mission Name to the string, it works the first time only.

I had thought this to be something to do with variable capture or 'loop variable closure', which is why I have the 'Mission m2 = m;' line. This does have an effect, in that I now get the correct string in the messagebox, but it only runs once. I can open that menu a hundred times and click, and only on the first time will I get anything. Before, when it was giving me only the final string from the list, it also only ran once (although at least now it is the correct string that is shown).

I have put a break point within that event handler to see when it fires. In the first circumstance (just 'You Clicked'), it fires every time I click one of the menu items. When I add that additional Mission Name, it only fires once.

I hope that confers enough information to be of use. Thanks for reading.

Edit: I am using Galasoft MVVMLight, in case that makes a difference.

Edit 2: I have also tried the most common suggestion, which is to copy the MissionName to a separate string within the loop and then use that withing the MessageBox.Show method. This makes no change - I mentioned that I'd already accounted for (as well as I know) the loop closure problem by my copying of the Mission object 'm' to a temporary object 'm2'. Even so, I still tried the suggestion to copy just the string and it did not make a difference.

Edit 3: I wanted to take the MessageBox out of the situation, so I created a List within the class which contains all this stuff (my View Model class) and tried adding to that the Mission ID. I did take closure into account before I did this and created a temporary int which I then used when I added that to the List. Just as before, it I told it a number to add (let's say '0') it would run every time I clicked a menu item. If I used something from within the loop (even a closure safe copy), it ran just once.

Edit 4: The event is triggered by a database load of these missions. They are loaded from a file when the program starts, and when that loading is complete, and event is fired. Two view models subscribe to this event. I have tried commenting out that other event, but it made no difference. Also there are eight (8) missions in the file and this loop of the missions runs eight (8) times.

Edit 5: I tried to change a few of the conditions for a test. I took out the access to the database, and I also took the entire thing out of the event handler. So now, in the constructor of the view model in question, I have the following code:

Mission m1 = new Mission() { MissionID = 1001, MissionName = "Mission 1001", IsCompleted = false };
        Mission m2 = new Mission() { MissionID = 1002, MissionName = "Mission 1002", IsCompleted = false };
        Mission m3 = new Mission() { MissionID = 1003, MissionName = "Mission 1003", IsCompleted = false };
        Mission m4 = new Mission() { MissionID = 1004, MissionName = "Mission 1004", IsCompleted = false };

        TestMissions.Add(m1);
        TestMissions.Add(m2);
        TestMissions.Add(m3);
        TestMissions.Add(m4);

        foreach (Mission m in this.TestMissions)
        {
            String sName = m.MissionName;

            DraggableNodeAddMissionList.Add(new ContextMenuVM()
            {
                DisplayName = sName,
                ContextMenuCommand = new RelayCommand(
                    () =>
                    {
                        MessageBox.Show("You clicked!" + sName);
                    })
            });
        }

So what happpens? Well, regarding the context menu, it populates as I would expect - four entries with the names of the missions. The commands are another matter. As before, if I remove 'sName' from the Messagebox.Show, I can click on any of the menu items (and as many times as I like) and I will get a result. I will see the messagebox and all is well. But there is a difference from before in the case where I do include sName (as in the code above). This time I can't even get one result (i.e. clicking on the menu items does nothing). Before I got one and then nothing after that. This time I just get none. And sName used for the DisplayName worked fine - the context menu items were as expected.

Edit 6: I repeated all the same steps only this time with something called 'Item' instead of 'Mission'. I loaded some 'items' from a file in the database, then added an event handler in my view model to populate the ObservableCollection for the menu item.

It now looks like this:

<ContextMenu x:Key="ChatNodeMenu" >
            <MenuItem Header="Remove ChatNode" Command="{Binding RelativeSource={RelativeSource FindAncestor, AncestorType={x:Type ContextMenu}}, Path=PlacementTarget.Tag.DataContext.RemoveChatNodeCommand}" />
            <Separator/>
            <MenuItem Header="Add branching for mission complete:" ItemsSource="{Binding ChatNodeListViewModel.DraggableNodeAddMissionList, Source={StaticResource Locator}}">
                <MenuItem.ItemContainerStyle>
                    <Style>
                        <Setter Property="MenuItem.Header" Value="{Binding DisplayName}"/>
                        <Setter Property="MenuItem.Command" Value="{Binding ContextMenuCommand}"/>
                    </Style>
                </MenuItem.ItemContainerStyle>
            </MenuItem>
            <MenuItem Header="Add direction for item:" ItemsSource="{Binding ChatNodeListViewModel.DraggableNodeAddItemDirectorsList, Source={StaticResource Locator}}">
                <MenuItem.ItemContainerStyle>
                    <Style>
                        <Setter Property="MenuItem.Header" Value="{Binding DisplayName}"/>
                        <Setter Property="MenuItem.Command" Value="{Binding ContextMenuCommand}"/>
                    </Style>
                </MenuItem.ItemContainerStyle>
            </MenuItem>
        </ContextMenu>

I then did the same steps to populate it - looping through all the Items within the database and, for each, adding an item to, in this case, DraggableNodeAddItemDirectorsList. I also took the same precautions for loop closure, by saving the name of the Item in a temporary string. It was pretty much a copy paste, in fact, but with 'Item' instead of 'Mission'. The problem was exactly the same (as perhaps should be expected).

Then I followed the steps in edit five (5) and separated out the event handler and access to the database and just used some test Items. It behaves exactly the same.

Edit 7: some success! I started a new project from scratch attempting to recreate the problem with the simplest project I could. I was indeed able to recreate the problem. Then, since my project was not going to be ruined, I played around with a few things. You see, the code I borrowed to create this ContextMenu stuff in the first place was in another of my projects. It worked in that project just fine, which is why I assumed it would work fine here. So what was the difference? The project in which it worked was using MicroMvvm and this project used MvvmLight (Galasoft). So I imported the MicroMvvm reference into my simple test project and then made all the RelayCommand items of the MicroMvvm type rather than of the MvvmLight type. And it worked!

So, for now, the solution is to import MicroMvvm into my main project to use the working RelayCommand from that.

It seems kinda weird that this doesn't work in MvvmLight but... that's definitely the difference for me. As to how I get MvvmLight and how up to date it is, I use the NuGet manager in Visual Studio 2010. My test project, which I created today, used the latest version on there.

Answer 1

The MVVM Light implementation of RelayCommand uses a WeakAction reference to the handler functions. This works nice in cases, where the action maps to a viewmodel function and the viewmodel lifetime defines the command / handler lifetime. This implementation doesn't really work with dynamically created Action handlers, where the command is supposed to hold the only reference to that action. Even though the command is working for lambdas that can be translated to a static function, I'd actually suggest not to use lambdas at all with this RelayCommand implementation.

Possible solutions:

• A RelayCommand implementation that supports strong action references
• Keeping a reference to the created action

Reference list example:

ConditionalWeakTable<ContextMenuVM, Action> ActionHolder = new ConditionalWeakTable<ContextMenuVM, Action>();
// keep action references alive, ActionHolder needs to have some
// appropriate scope so it doesn't disappear as long as
// ContextMenuVM is alive

...

    foreach(Mission m in Database.Instance.Missions)
    {
        var item = new ContextMenuVM()
        {
            DisplayName = m.MissionName,
        };
        Action a = () =>
            {
                MessageBox.Show("You clicked!" + item.DisplayName);
            };
        item.ContextMenuCommand = new RelayCommand(a);
        DraggableNodeAddMissionList.Add(item);
        ActionHolder.Add(item, a); // keep a strong action reference for the lifetime of item
    }