Fixing a years spanning UI bug

I'm maintaining a WPF Windows application that shows the user a list of items, each item having several action buttons that can be enabled or disabled depending on the state of the item.

The implementation is rather straightforward, but with a peculiarity: the commands (ICommand properties) are not on the ItemViewModel but on the parent view model (the one containing the collection of ItemViewModel).

Each command is given the ItemViewModel calling the command as a parameter:

<ListView ItemsSource="{Binding Items}" x:Name="Root">
  <ListView.ItemTemplate>
    <DataTemplate>
      <Button Command="{Binding DataContext.Command, ElementName=Root}"
              CommandParameter="{Binding}" />
    </DataTemplate>
  </ListView.ItemTemplate>
</ListView>

public class MyCommand : ICommand
{
  // [...]
  public bool CanExecute(object parameter)
  {
    var item = (ItemViewModel)parameter;
    return item.IsEnabled;
  }
  // [...]
}

It has worked like that for years without much problems, except that in some rare cases the action buttons are not properly enabled or disabled when the corresponding ItemViewModel changes.

Since it rarely occurs, and clicking anywhere in the view fixes it, nobody has bothered trying to fix it...

The magic that held it together

Fast forward to a few months ago.

We had been doing a lot of changes to our code base to make it cross-platform (and use the same code base for a Windows WPF and a Xamarin.Mac application).

We managed to reuse most of our View-Models, but one of the things we had to change was the way commands are refreshed.

The common way to refresh the CanExecute property of ICommand instances is to use the CommandManager, and specifically its RequerySuggested event.

This event is magically called whenever "the CommandManager detects conditions that might change the ability of a command to execute".

Most of the time, it works well. Sometimes, manually calling InvalidateRequerySuggested() is necessary.

To fix our refresh bug, we had already tried spreading around calls to InvalidateRequerySuggested(), but without much success...

Anyway, the CommandManager is not portable, so we had to get rid of it.

We replaced it with explicit raising of the ICommand.CanExecuteChanged event whenever something would require CanExecute to be re-evaluated.

Magic is bad

We were expecting our long lasting refresh bug to be solved by the removal of the CommandManager, but instead, it got worse: the bug was still happening, and this time, buttons would get stuck indefinitely. Clicking anywhere in the view was no longer doing anything.

The magic was gone, but at least now the bug was explicit and more deterministic.

We noticed that it would always happen when the list was scrolled: items being brought into view would have their buttons stuck disabled, while the ones that were previously visible would not have the problem.

The cause of the bug was now clear: UI virtualization was messing with our commands CanExecute().

Fixing the bug

Our first instinct was to add more code, and try to tie ICommand.CanExecuteChanged with the scrolling events.

Since UI virtualization is not supposed to be extended by user code, it did not go well. After a few wasted hours adding more and more hacks to try and access the collection of visible items, we stopped looking that way.

The fix is actually simple for our specific case: move the ICommand properties from the parent View Model into the ItemViewModel object.

The result makes more sense, and WPF knows how to make it work with UI virtualization!

<ListView ItemsSource="{Binding Items}">
  <ListView.ItemTemplate>
    <DataTemplate>
      <Button Command="{Binding Command}" />
    </DataTemplate>
  </ListView.ItemTemplate>
</ListView>

Appendix

Here is a complete demo of the bug, and its fix. Diffing both versions is encouraged.

TL;DR: when displaying a list of items in Xaml, especially one that might be virtualized, prefer declaring ICommand properties on the item using them rather than on the object holding the item collection. Otherwise, visual elements bound to CanExecute might not always refresh properly.