Damir's Corner.

Dangers of a hidden PropertyChanged event

I recently spent a considerable amount of time looking for a reason why the data binding in one of the Xamarin.Forms pages was not working. When I found the cause, it made perfect sense. But it was not easy to identify it.

Here are the (simplified) relevant parts of the view model code:

public class MainPageViewModel : BaseViewModel
{
  private List<string> items = new List<string>();

  public List<string> Items
  {
    get { return items; }
    set
    {
      items = value;
      OnPropertyChanged();
    }
  }

  public MainPageViewModel()
  {
    LoadData();
  }

  async void LoadData()
  {
    await Task.Delay(500);
    Items = new List<string>
    {
      "Item 1",
      "Item 2",
      "Item 3",
      "Item 4",
      "Item 5"
    };
  }

  public event PropertyChangedEventHandler PropertyChanged;

  private void OnPropertyChanged([CallerMemberName] string propertyName = null)
  {
    PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
  }
}

And here is the markup for the page:

<ContentPage.BindingContext>
  <viewmodels:MainPageViewModel />
</ContentPage.BindingContext>

<StackLayout>
  <ListView ItemsSource="{Binding Items}" />
</StackLayout>

Basically, it's just a data-bound property whose value is set when the data is loaded asynchronously. Can you guess why this data was never rendered on the page?

It was due to the incorrect implementation of INotifyPropertyChanged. Incorrect how?

The following warnings should give you a clue:

CS0108: MainPageViewModel.PropertyChanged hides inherited member BaseViewModel.PropertyChanged. Use the new keyword if hiding was intended. CS0108: MainPageViewModel.OnPropertyChanged(string) hides inherited member BaseViewModel.OnPropertyChanged(string). Use the new keyword if hiding was intended.

The PropertyChanged event invoked by the OnPropertyChanged method in this class is not the event that implements the INotifyPropertyChanged interface, and therefore is not handled by the user interface. Thus, if the OnPropertyChanged method is called by the Items property after the new value is set, the triggered event never reaches the user interface and the list view is not re-rendered.

This is exactly what the warnings say. These two members are not related to the base class members of the same name. They are only accessible from the derived class and break the polymorphism.

Using the new keyword, as suggested, also does not fix the problem. It just makes the warning go away because we are telling the compiler that the behavior I just described is what we wanted to achieve.

But that's not true. We wanted to invoke the PropertyChanged event, which implements the INotifyPropertyChanged interface as defined in the BaseViewModel base class:

public class BaseViewModel : INotifyPropertyChanged
{
  public event PropertyChangedEventHandler PropertyChanged;

  protected void OnPropertyChanged([CallerMemberName] string propertyName = null)
  {
    PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
  }
}

The best way to do that is to delete the PropertyChanged event and the OnPropertyChanged method in our derived MainPageViewModel class. They are just duplicates of the members in the base class anyway. And after we delete them, the code in the derived class will correctly call the members of the base class. And the loaded data will be displayed on the page as intended.

An interesting question is why these duplicate members would be implemented in the derived class in the first place? Of course, this is a programmer's mistake. But such a mistake can happen quickly during refactoring if we are not careful enough.

For example, it could be that the BaseViewModel was not in the project at first and the MainPageViewModel had to implement the INotifyPropertyChanged interface itself, so the PropertyChanged event had to be defined in the class. Only later was the BaseViewModel introduced to get rid of the same code in each view model. At that point, the MainPageViewModel could be derived from BaseViewModel instead of implementing the INotifyPropertyChanged interface. And since the PropertyChanged and OnPropertyChanged members were not deleted at the same time, we ended up with broken code.

And then there's the matter of warnings. If these were noticed immediately, the problem would be easier to identify. But new warnings are only easy to spot in a project if there are no other warnings. However, if there are already dozens or even hundreds of warnings in the project or even in the same class, it is much more difficult. This is another reason why all warnings in a project should be fixed one way or another as soon as possible.

I have created an example project on this topic in my GitHub repository. The individual commits show the working code before the BaseViewModel was added, the broken code after it was mistakenly added, and finally the fixed working code again.

Programming errors happen. And while this error can be noticed and explained with sufficient understanding of C#, it is even more important to keep the project code in high quality. This includes fixing any compiler warnings when they first appear. The best way to force yourself and other developers on the team to do this is to enable the compiler flag Treat warnings as errors.