Is it better to put my logic in an event handler or in a setter for MVVM (Xamarin `Picker` `SelectedItem` quirks)

Question

SOLUTION IS IN EDIT OF THE ACCEPTED ANSWER

I have a view in which has two Pickers, I need to have it so that when the SelectedItem property in one Picker changes, the list of Items in the second Picker (ItemSource) changes as well.

Currently I have a bound the SelectedItem and SelectedIndex properties of both pickers to properties in my ViewModel. In the setter(s) for both of them, I perform the logic needed to change the list of Items in the second picker. The list of Items in the second picker changes successfully, but when I set the SelectedIndex (to make it select an Item by default), this fails if the index which I am setting it to is the same as the index which it was on in the previous list. It just shows the Title of the Picker instead, this issue seems to be related to this bug.

My Code:

• Both Pickers are bound to an Observable collection of strings FYI
• FrameType and DirectionType are Enums
• I initially used only the SelectedItem property

Relevant XAML

<Label Grid.Row="1" Grid.Column="0" VerticalTextAlignment="Center"
     Text="Direction: " />
     <Picker x:Name="PickerDirection" Grid.Row="1" Grid.Column="1"
                      Title="Select Direction"
                      ItemsSource="{Binding Directions}"
                      SelectedItem="{Binding SelectedDirection}"
                      SelectedIndex="{Binding SelectedDirectionIndex}"></Picker>

<Label Grid.Row="2" Grid.Column="0" VerticalTextAlignment="Center"
       Text="Frame: "/>
<Picker x:Name="PickerFrame" Grid.Row="2" Grid.Column="1"
                 Title="Select Frame"
                 ItemsSource="{Binding Frames}"
                 SelectedItem="{Binding SelectedFrame}"
                 SelectedIndex="{Binding SelectedFrameIndex}"></Picker>

Relevant View Model code:

public string SelectedDirection
{
    get { return _selectedDirection; }
    set 
    {
        if (Directions.Contains(value))
        {
            if (_selectedDirection != value)
            {
                if (EnumUtils.ToEnumFromString<FrameType>(SelectedFrame) == FrameType.Road &&
                            !DirectionsRoad.Contains(value))
                {
                 _selectedDirection = Directions[Directions.IndexOf(DirectionType.Right.ToString())];
                }
                else
                {
                    _selectedDirection = value;
                }
                OnPropertyChanged();
            }
        }
    }
}

public string SelectedFrame
{
    get { return _selectedFrame; }
    set
    {
        if (_selectedFrame != value)
        {
            _selectedFrame = value;
            if (EnumUtils.ToEnumFromString<FrameType>(_selectedFrame) == FrameType.Road)
            {
                Directions = DirectionsRoad;
                if (Directions.Contains(SelectedDirection))
                {
                    SelectedDirectionIndex = Directions.IndexOf(SelectedDirection);
                }
                else
                {
                     SelectedDirectionIndex = Directions.IndexOf(DirectionType.Right.ToString());
                }
              }else if (EnumUtils.ToEnumFromString<FrameType>(_selectedFrame) == FrameType.Lane)
              {
                  Directions = DirectionsAll;
                  SelectedDirectionIndex = Directions.IndexOf(SelectedDirection);
              }
               OnPropertyChanged();
           }
       }
   }
}

public int SelectedDirectionIndex
{
    get { return _selectedDirectionIndex; }
    set
    {
        if (_selectedDirectionIndex != value)
        {
            if (!(value < 0 || value >= Directions.Count))
            {
                _selectedDirectionIndex = value;
                OnPropertyChanged();
            }
        }
    }
}

public int SelectedFrameIndex
{
    get { return _selectedFrameIndex; }
    set
    {
        if (_selectedFrameIndex != value)
        {
            if (!(value < 0 || value >= Frames.Count))
            {
                _selectedFrameIndex = value;
                OnPropertyChanged();
            }
        }
    }
}

The outcome:

I expect it to never be empty since I ensure that the SelectedDirection is always set to something.

Notes:

• I initially used just the SelectedItem property, but when I encountered this bug I thought using the SelectedIndex property would help to fix it.

• I used ObservableCollection to maintain consistency with the other viewModels in the project, and to ensure that the options in the picker are updated when I make changes (based on my understanding you need to use ObservableCollection to make this possible).

• I do plan to refactor the code in the setter for SelectedFrame into smaller functions as soon as I get things to work.

Due to this It seems that using the SelectedIndexChanged event of the Picker would be the only way to fix this. However the comment by ottermatic in this question says that events are unreliable. Hence I felt is was better to perform this logic in the setter.

If someone could comment on what I may be doing wrong in my code which is causing this issue and also comment on the pros/cons and/or whether or not I should use the eventHandler or have the logic in my setter. Thanks

Answer 1

I don't really see why you are using both SelectedItem and SelectedIndex, but I think what you are trying to achieve can be achieved easier.

First of all, I don't think you need ObservableCollection types at all in your example, since you are setting the directions anyway and not modifying the collection. More importantly, fiddling around with the indices is completely unnecessary, as far as I can tell. You are using strings anyway and even though String is not a value type, but a reference type, you cannot practically distinguish two String instances that have the same content, hence assinging the respective values to SelectedDirection and SelectedFrame is sufficient.

The following checks seem redundant to me

if (Directions.Contains(value))


if (EnumUtils.ToEnumFromString<FrameType>(SelectedFrame) == FrameType.Road &&
    !DirectionsRoad.Contains(value))

since Directions are set to DirectionsRoad anyway if SelectedFrame has been set to "Road". Hence I'd assume that the second condition won't evaluate to true in any case. Hence the SelectedDirection can be simplified:

public string SelectedDirection
{
    get => _selectedDirection;
    set 
    {
        if (_selectedDirection != value && Directions.Contains(value))
        {
            _selectedDirection = value;
            OnPropertyChanged();
        }
    }
}

Within the setter of SelectedFrame there are many things going on, which I'd refactor to methods on it's own right, to improve clarity.

public string SelectedFrame
{
    get => _selectedFrame;
    set
    {
        if (_selectedFrame != value)
        {
            _selectedFrame = value;
            UpdateAvailableDirections();
            OnPropertyChanged();
        }
    }
}

private void UpdateAvailableDirections()
{
    // store the selected direction
    var previouslySelectedDirection = SelectedDirection;

    Directions = GetValidDirectionsByFrameType(EnumUtils.ToEnumFromString<FrameType>(SelectedFrame));
    SelectedDirection = GetSelectedDirection(previoslySelectedDirection, Directions);
}

private string[] GetValidDirectionsByFrameType(FrameType frameType)
{
    return frameType == FrameType.Road ? DirectionsRoad : DirectionsAll; 
}

private string GetSelectedDirection(string previouslySelectedDirection, string[] validDirections)
{
    return validDirections.Contains(previouslySelectedDirection) ? previouslySelectedDirection : DefaultDirection;
}

By setting the SelectedItem instead of fiddling with the indices, the correct values shall be displayed.

Concerning your question whether this logic may be better suited in an event handler or in the setter depends on your requirements. If all you need is the index, the event SelectedIndexChanged may work out for you, but if the value is needed in several places and methods that are not called by the event handler, the presented solution may be more viable.

Edit

You were correct, it has got nothing to do with the usage of SelectedIndex and SelectedItem. The issue is a bit more subtle.

I build a quick proof-of-concept and found the following:

• Assuming SelectedDirection is "Right" (and the index is set accordingly)
• When Directions is set, the SelectedItem on the picker seems to be reset
• SelectedDirection is set to null
• this.Directions.Contains(value) evaluates to false, hence _selectedDirection is not set (this hold true for SelectedDirectionIndex, since the value -1 is filtered by if(!value < 0 || value >= this.Directions.Count))
• When SelectedDirection is set afterwards, the value is still "Right", hence OnPropertyChanged is not called (since the values are the same) and the SelectedItem is not set

This way there is a mismatch between the value the Picker actually holds and the property in the viewmodel, which leads to unexpected behavior.

How to mitigate the issue?

I'd still stick with the code without the indices (unless you really need them) and use the string values.

There are other possibilities, but I'd change the setter of SelectedDirection. When you allowed the value to be set to null, PropertyChanged will be raised properly when the value is set to Right afterwards. If you really need to filter what the value is set to, you should still raise OnPropertyChanged, to inform the Picker that the value has changed (preventing a mismatch between the actual Pickers value and the viewmodel)

public string SelectedDirection
{
    get => _selectedDirection;
    set 
    {
        if (_selectedDirection != value)
        {
            if(Directions.Contains(value))
            {
                _selectedDirection = value;
            }
            else
            {
                _selectedDirection = DirectionTypes.Right.ToString();
            }

            OnPropertyChanged();
        }
    }
}