views:

906

answers:

5

I have a list of items in a WPF ListBox. I want to allow the user to select several of these items and click a Remove button to eliminate these items from the list.

Using the MVVM RelayCommand pattern, I've created a command with the following signature:

public RelayCommand<IList> RemoveTagsCommand { get; private set; }

In my View, I wire up my RemoveTagsCommand like this:

<DockPanel>
<Button DockPanel.Dock="Right" Command="{Binding RemoveTagsCommand}" CommandParameter="{Binding ElementName=TagList, Path=SelectedItems}">Remove tags</Button>
<ListBox x:Name="TagList" ItemsSource="{Binding Tags}" SelectionMode="Extended">
    <ListBox.ItemsPanel>
        <ItemsPanelTemplate>
            <StackPanel Orientation="Horizontal"/>
        </ItemsPanelTemplate>
    </ListBox.ItemsPanel>
    <ListBox.Resources>
        <DataTemplate DataType="{x:Type Model:Tag}">
            ...
        </DataTemplate>
    </ListBox.Resources>
</ListBox>
</DockPanel>

My ViewModel constructor sets up an instance of the command:

RemoveTagsCommand = new RelayCommand<IList>(RemoveTags, CanRemoveTags);

My current implementation of RemoveTags feels clunky, with casts and copying. Is there a better way to implement this?

    public void RemoveTags(IList toRemove)
    {
        var collection = toRemove.Cast<Tag>();
        List<Tag> copy = new List<Tag>(collection);

        foreach (Tag tag in copy)
        {
            Tags.Remove(tag);
        }
    }
A: 

Why don't you specify the type argument of the RelayCommand to be a List<Tag>, since that is what you are going to get anyway? There is no point specifying a more generic type than that because the executed handler is hard-coded to work with a list of Tag objects. Since you've already made the dependency there, you might as well make it on the type argument, as well. Then your executed handler won't need any cast or copying.

Charlie
I don't believe SelectedItems will be List<Tag>, even though you've only added Tag instances.
Dan Bryant
@Dan is right; I can't use a List<Tag> here. I think the SelectedItems property returns a non-generic SelectedItemsCollection object.
dthrasher
+1  A: 

This looks fairly clean to me, though you might be able to bind SelectedItems to a property on your VM using Mode=OneWayToSource and then use the bound collection property from RemoveTags. I'm not entirely sure, but you may be able to use a strongly-typed IList collection in this case.

Dan Bryant
Unfortunately, I can't use a strongly typed IList. I think this is a limitation of the SelectedItemCollection returned by WPF, but it could also be due to the particular flavor of RelayCommand I use.
dthrasher
Looking at this again, I see the real concern is that the SelectedItems list may change as a result of the removal, which is why you create the copy. If that's the case, I think this code is about as clean as you'll get. The casting is unavoidable, unless you want to make your RelayCommand support the casting internally (something like `RelayListCommand<Tag>` with an implicit `Cast<T>` adapting `IList` to `IList<T>`).
Dan Bryant
Thanks, Dan. You're right -- the copying and casting feels a bit clunky, but it gets the job done.
dthrasher
A: 

I'd use the ItemContainerStyle on the ListBox to bind the items' IsSelected property to a flag in the view model, e.g.:

 <ListBox.ItemContainerStyle> 
    <Style TargetType="{x:Type ListBoxItem}">  
      <Setter Property="IsSelected" Value="{Binding Path=IsSelected, Mode=TwoWay}"/>  
    </Style> 
 </ListBox.ItemContainerStyle>

Then you don't need to worry about what argument you're passing in to your command. Also, in my experience when it's simple for an object in a view model to know that the user selected it, you find other uses for that information.

The code in the command would be something like:

foreach (Tag t in Tags.Where(x => x.IsSelected).ToList())
{
   Tags.Remove(t);
}
Robert Rossney
Hmmm. That's a very interesting approach. I'm not sure about letting UI state leak into my ViewModel, though.
dthrasher
That doesn't make sense to me. The view model is the model of the view. It seems entirely appropriate for it to know about UI state. If it exposes a command to remove selected items, surely it should know what the selected items are. You might restyle your view so that it uses a `ListView` instead of a `ListBox`, but you're not going to restyle it so that it doesn't let the user select the items to remove.
Robert Rossney
A: 

1.) Bind your Remove button a Command in your ViewModel.

2.) When you set up your binding, use a CommandParameter to take the Selecteditems from your ListBox by giving your ListBox a name and using ElementName=NameOfListBox, Path=SelectedItems

3.) Make sure your Command in your ViewModel passes along the arguments. You'll get an object that you can cast as an IList.

Below is a simple example, this should help you set up your structure.

In the View:

<Button Command="{Binding CommandInViewModelForRemove}"
        CommandParameter="{Binding ElementName=blah,Path=SelectedItems}"

<ListBox x:Name="blah" .... />

In the ViewModel:

public ViewModel(){
    RemoveCommand = new RelayCommand<object>(Remove, CanRemove);
}

private void Remove(object selectedItems){
   var list = (IList)selectedItems;
   //do some work, cast to view models that represent list items, etc
}

Hope this helps!

Jimmy Lyke
I was actually doing the things you suggest already. My question was about the "do some work" bits in your sample. Is there a cleaner, better way to remove items from the list without casting each item and copying the selectedItems to a new list?
dthrasher
Wow, I completely missed that -- sorry about that! Let me head back to the drawing board and see what I come up with. Cheers!
Jimmy Lyke
A: 

Not trying to hijack this thread, but my SelectedItems.Count always is ZERO.

I found this thread from Laurent (at the end of the thread): http://geekswithblogs.net/lbugnion/archive/2010/05/19/handling-datagrid.selecteditems-in-an-mvvm-friendly-manner.aspx

thanks, voss.

Voss
@Voss - welcome to Stack Overflow. there are no threads to hijack - only answers, which this is not.
McDowell