Wiring CollectionView and updating itself without the need for Refresh()

Question

I have been using your expertise developing my application using parts of WPF that I have never touched before.

In this stage my View does all I need it to do, however, I am not sure if I've done my side of the job right. My guess is that it still going to need numerous changes to be called as code following "Best Practice".

One of those major changes should be the binding of my CollectionView, which at the moment depends on its Refresh() to update its content.

The second would be using code behind for the ListViewItems double click.

Please feel free to comment any other issues other than the couple I mentioned.

ViewModel

/// <summary>
/// Represents a container of EditPartViewModel objects
/// that havs support for staying synchronized with the
/// PartRespository. This class also provides information
/// related to multiple selected parts.
/// </summary>
public class AllPartsViewModel : WorkspaceViewModel
{       
    #region Fields

    readonly RelayCommand _clearSearch;
    readonly PartRepository _partRepository;
    readonly VendorRepository _vendorRepository;
    bool _errorParts;
    List<EditPartViewModel> allpvms;
    RelayCommand _exportToCsv;
    INotifyCollectionChanged notifyCollectionChanged;
    string _searchFilterString;

    #endregion // Fields

    #region Constructor

    public AllPartsViewModel(PartRepository partRepository, VendorRepository vendorRepostiory)
    {
        if (partRepository == null)
            throw new ArgumentNullException("partRepository");
        if (_clearSearch == null)
            _clearSearch = new RelayCommand(
                param => this.OnRequestCleanSearch(),
                param => this.CanClear
                );

        base.DisplayName = Strings.AllPartsViewModel_DisplayName;

        _partRepository = partRepository;
        _vendorRepository = vendorRepostiory;
        _searchFilterString = string.Empty;

        // Populates the AllParts collection with PartViewModels.
        this.CreateAllParts();
    }

    void CreateAllParts()
    {
        // GetParts() returns List<Part>. Now we can pour it's results 
        // using the following select in List<EditPartViewModel> on the 
        // fly.
        // allpvms: All Part View Models.
        allpvms = _partRepository.GetParts().Select(p => new EditPartViewModel(p, _vendorRepository)).ToList();

        foreach (EditPartViewModel cvm in allpvms)
        {
            cvm.PropertyChanged += this.OnPartViewModelPropertyChanged;
        }

        this.AllParts = new ObservableCollection<EditPartViewModel>(allpvms);
        this.AllParts.CollectionChanged += this.OnCollectionChanged;

        // We could use the ICollectionView interface rather. However it wouldn't have the list features
        // such as add/remove/count etc. 
        this.AllPartsCollectionView = (ListCollectionView)CollectionViewSource.GetDefaultView(this.AllParts);
        AllPartsCollectionView.Filter = PartsFilter;

        // ListCollectionView explicitly implements INotifyCollectionChanged.CollectionChanged
        // from CollectionView. 
        // So to be able to bind to ListCollectionView's Count property, you need to cast
        // your collection view to INotifyCollectionChanged first. Then subscribe to 
        // CollectionChanged event such as below.
        notifyCollectionChanged = (INotifyCollectionChanged)this.AllPartsCollectionView;
        notifyCollectionChanged.CollectionChanged += OnNotifyCollectionChanged;
    }

    #endregion // Constructor

    #region Public Interface

    public ObservableCollection<EditPartViewModel> AllParts { get; set; }

    public ListCollectionView AllPartsCollectionView { get; private set; }

    public int CollectionCount { get { return AllPartsCollectionView.Count; } }

    public bool ShowErrorsOnly
    {
        get
        {
            return _errorParts;
        }
        set
        {
            _errorParts = value;
            OnPropertyChanged("ShowErrorsOnly");
            AllPartsCollectionView.Refresh();
        }
    }

    public string SearchFilterString
    {
        get { return _searchFilterString; }
        set
        {
            _searchFilterString = value;
            OnPropertyChanged("SearchFilterString");
            AllPartsCollectionView.Refresh();
        }
    }

    #endregion // Public Interface

    #region Base Class Overrides

    protected override void OnDispose()
    {
        // TODO: Added '.ToList()' to the end of AllParts is a change
        //      made to fix the 'Collection was modified; enumeration 
        //      operation may not execute' error. Investigate the 
        //      effects of this change.
        foreach (EditPartViewModel partVM in this.AllParts.ToList())
        {
            partVM.Dispose();

            this.AllParts.Clear();
            this.AllParts.CollectionChanged -= this.OnCollectionChanged;
        }
        // To release the handler delegated to this event.
        this.notifyCollectionChanged.CollectionChanged -= this.OnNotifyCollectionChanged;
    }

    #endregion // Base Class Overrides

    #region Event Handling Methods

    private bool PartsFilter(object item)
    {
        var editPartViewModel = item as EditPartViewModel;
        return PartsSearchFilter(item) && PartsWithErrorFilter(item);
    }

    private bool PartsSearchFilter(object item)
    {
        var editPartViewModel = item as EditPartViewModel;
        return editPartViewModel.PartName.ToUpper().Contains(_searchFilterString.ToUpper());

    }

    private bool PartsWithErrorFilter(object item)
    {
        var editPartViewModel = item as EditPartViewModel;
        return _errorParts ? editPartViewModel.HasErrors : true;

    }

    /// <summary>
    /// This is for the AllPartsCollectionView.
    /// </summary>
    /// <param name="sender"></param>
    /// <param name="e"></param>
    private void OnNotifyCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        OnPropertyChanged("CollectionCount");
    }

    /// <summary>
    /// This is for AllParts.
    /// </summary>
    /// <param name="sender"></param>
    /// <param name="e"></param>
    void OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        if (e.NewItems != null && e.NewItems.Count != 0)
            foreach (EditPartViewModel partVM in e.NewItems)
                partVM.PropertyChanged += this.OnPartViewModelPropertyChanged;

        if (e.OldItems != null && e.OldItems.Count != 0)
            foreach (EditPartViewModel partVM in e.OldItems)
                partVM.PropertyChanged -= this.OnPartViewModelPropertyChanged;
    }

    void OnPartViewModelPropertyChanged(object sender, PropertyChangedEventArgs e)
    {
        string IsSelected = "IsSelected";

        // Makes sure that the property name we're referencing is valid.
        // This is a debugging technique, and doesn't execute in a release build.
        (sender as EditPartViewModel).VerifyPropertyName(IsSelected);
    }

    #endregion // Event Handling Methods

    #region Event Handlers

    // Command bound to 'Export the List' button in the view.
    public ICommand ExportToCsv
    {
        get
        {
            if (_exportToCsv == null)
            {
                _exportToCsv = new RelayCommand(
                    param => this.OnRequestExportToCsv());
            }
            return _exportToCsv;
        }
    }

    public ICommand ClearSearch { get { return _clearSearch; } }

    #endregion // Event Handlers

    #region Private Helpers

    private void OnRequestCleanSearch()
    {
        _searchFilterString = string.Empty;
        OnPropertyChanged("SearchFilterString");
        AllPartsCollectionView.Refresh();
    }

    private void OnRequestExportToCsv()
    {
        var timeStamp = DateTime.Now.ToString("ddMMMyy hhttmm");
        SaveFileDialog saveFileDialog = new SaveFileDialog();
        saveFileDialog.FileName = string.Format("PriceGenerator_Output_{0}", timeStamp);
        saveFileDialog.DefaultExt = ".csv";
        saveFileDialog.Filter = "CSV (Comma delimited) (*.csv)|*.csv";

        var allPartsInTheView = AllPartsCollectionView.OfType<EditPartViewModel>().ToList();
        var finalCsvString = ListToCsvConverter.ConvertWithComma(allPartsInTheView);

        if (saveFileDialog.ShowDialog() == DialogResult.OK)
        {
            string csvFilePath = saveFileDialog.FileName;
            try
            {
                var csvFile = File.CreateText(csvFilePath);
                csvFile.WriteLine(finalCsvString);
                csvFile.Close();
            }
            catch (Exception)
            {
                System.Windows.Forms.MessageBox.Show("Error: Could not read file from disk.");
            }
        }
    }

    /// <summary>
    /// returns true if the searchFilterString isn't empty.
    /// </summary>
    bool CanClear
    {
        get
        {
            return !string.IsNullOrEmpty(this._searchFilterString);
        }
    }

    #endregion // Private Helpers
}

XAML (partial)

<UserControl.Resources>
...
    <DataTemplate DataType="{x:Type vm:EditPartViewModel}">
    <DataTemplate.Resources>
    ...
    </DataTemplate.Resources>     
    </DataTemplate>
...
</UserControl.Resources>

<ListView
AlternationCount="2"
d:DataContext="{d:DesignInstance Type=vm:MockViewModel, IsDesignTimeCreatable=True}"
ItemsSource="{Binding AllPartsCollectionView}"
>
<ListView.ItemContainerStyle>
    <Style TargetType="{x:Type ListViewItem}">
    <EventSetter Event="MouseDoubleClick" Handler="ListViewItem_MouseDoubleClick" />
    <Setter Property="HorizontalContentAlignment" Value="Stretch"/>
    </Style>
</ListView.ItemContainerStyle>
</ListView>

And in case if you are curious, this is the View I am producing filled with my MockViewModel rather than the actual one.

UI

Answer 1

One thing that jumped out at me is this:

private bool PartsFilter(object item)
{
    var editPartViewModel = item as EditPartViewModel;
    return PartsSearchFilter(item) && PartsWithErrorFilter(item);
}

private bool PartsSearchFilter(object item)
{
    var editPartViewModel = item as EditPartViewModel;
    return editPartViewModel.PartName.ToUpper().Contains(_searchFilterString.ToUpper());
}

private bool PartsWithErrorFilter(object item)
{
    var editPartViewModel = item as EditPartViewModel;
    return _errorParts ? editPartViewModel.HasErrors : true;
}

You are sacrificing C#'s strongly typed model by allowing everything to be passed in. Afterwards you cast it to the expected type but you use as which means the value will be null if it happened to be the wrong type. This will cause NullReferenceExceptions on the next lines which call methods on this object.

The solution is easy: make each method accept an object of type EditPartViewModel and embrace the strongly typed nature of C#.

---

Notice you don't use editPartViewModel in your first method (although this is caught by doing that cast in every method). Still, nasty.

---

Compare case-sensitive strings using string.Equals(a, b, StringComparison.InvariantCulture). Imagine if that method is called 500.000 times with strings over 5000 characters long: each .ToUpper() call would create a new string object.

---

Always put brackets ({}), even if it's for oneliners. Save yourself the inevitable headaches of having to find an error only to find out it's because it wasn't in the scope of an if or for block.

---

Use [CallerMemberName] instead of using weakly-typed strings inside OnPropertyChanged. Example here: http://grenangen.se/node/75

---

Since AllPartsCollectionView is accessible from public, it isn't needed to specifically define

public int CollectionCount { get { return AllPartsCollectionView.Count; } }

If you need this property in your xaml, you can always used the nested notation.

---

I don't know what _errorParts means. Keep boolean naming convention in mind and make it descriptive.

---

Inherit from INotifyxxx instead of using composition. They define a trait of your type and should be used as such.

Answer 2

There's a lot going on here, but a few quick things:

There's duplication between OnRequestCleanSearch and SearchFilterString:

private void OnRequestCleanSearch()
{
    _searchFilterString = string.Empty;
    OnPropertyChanged("SearchFilterString");
    AllPartsCollectionView.Refresh();
}

public string SearchFilterString
{
    get { return _searchFilterString; }
    set
    {
        _searchFilterString = value;
        OnPropertyChanged("SearchFilterString");
        AllPartsCollectionView.Refresh();
    }
}

You can just write

private void OnRequestCleanSearch()
{
    SearchFilterString = string.Empty;
}

---

This test in the constructor is redundant

if (_clearSearch == null)

---

Since CreateAllParts is only called from the constructor, you can just move it into the constructor.

---

System.Windows.Forms.MessageBox.Show("Error: Could not read file from disk.");

The error message talks about reading the file, but the code above is trying to write to the file. It's poor form to catch Exception -- catch the more specific IOException here. Also use the using pattern:

using (var csvFile = File.CreateText(csvFilePath))
{
    csvFile.WriteLine(finalCsvString);
}