3

I'm implementing a ComboBox that can be refreshed by users using a button. I'm trying to make it so that the previously selected item is automatically reselected if still present inside the ComboBox after a refresh.

MainWindow.xaml:

<ComboBox Canvas.Left="10" Canvas.Top="10" DisplayMemberPath="Name" IsEnabled="{Binding Path=Enabled}" ItemsSource="{Binding Path=Items}" SelectedItem="{Binding Mode=TwoWay, Path=SelectedItem}" Width="379"/>
<Button Content="{x:Static p:Resources.TextRefresh}" Canvas.Right="10" Canvas.Top="10" Click="OnClickButtonRefresh" Width="75"/>

MainWindow.xaml.cs:

public MainWindow()
{
    InitializeComponent();
    DataContext = m_BrowserInstances = new BrowserInstancesViewModel();
}

private void OnClickButtonRefresh(Object sender, RoutedEventArgs e)
{
    m_BrowserInstances.Populate();
}

[EDITED TO CURRENT VERSION] BrowserInstancesViewModel.cs:

public sealed class BrowserInstancesViewModel : ViewModel
{
    private Boolean m_Enabled;
    public Boolean Enabled
    {
        get { return m_Enabled; }
    }

    private BrowserInstance m_SelectedItem;
    public BrowserInstance SelectedItem
    {
        get { return m_SelectedItem; }
        set
        {
            if (m_SelectedItem != value)
            {
                m_SelectedItem = value;
                NotifyPropertyChanged("SelectedItem");
            }
        }
    }

    private ObservableCollection<BrowserInstance> m_Items;
    public ObservableCollection<BrowserInstance> Items
    {
        get { return m_Items; }
    }

    public BrowserInstancesViewModel()
    {
        Populate();
    }

    private static Func<BrowserInstance, Boolean> Recover(BrowserInstance selectedItem)
    {
        return x =>
        {
            Process currentProcess = x.Process;
            Process selectedProcess = selectedItem.Process;

            if (currentProcess.Id != selectedProcess.Id)
                return false;

            if (currentProcess.MainModule.BaseAddress != selectedProcess.MainModule.BaseAddress)
                return false;

            if (currentProcess.MainWindowTitle != selectedProcess.MainWindowTitle)
                return false;

            return true;
        };
    }

    public void Populate()
    {
        BrowserInstance item = m_SelectedItem;
        List<BrowserInstance> items = new List<BrowserInstance>();

        foreach (Process process in Process.GetProcessesByName("chrome"))
            items.Add(new BrowserInstance(process));

        if (items.Count > 0)
        {
            m_Enabled = true;

            m_Items = new ObservableCollection<BrowserInstance>(items.OrderBy(x => x.Process.Id));

            if (item != null)
                m_SelectedItem = m_Items.SingleOrDefault(Recover(item));

            if (m_SelectedItem == null)
                m_SelectedItem = m_Items[0];
        }
        else
        {
            m_Enabled = false;

            m_Items = new ObservableCollection<BrowserInstance>();
            m_Items.Add(new BrowserInstance());

            m_SelectedItem = m_Items[0];
        }

        NotifyPropertyChanged("Enabled");
        NotifyPropertyChanged("Items");
        NotifyPropertyChanged("SelectedItem");
    }
}

I can get back the previously selected item, but only sometimes. Looks like the code is not working properly when I need to select a default value (Index 0) if the previous selected item cannot be recovered.

Tommaso Belluzzo
  • 23,232
  • 8
  • 74
  • 98

1 Answers1

3

You need to set m_SelectedItem to the item found by SingleOrDefault(Recover(...)).

Currently, you are setting it to the old instance. That instance no longer exists in the list and apparently your BrowserInstance class doesn't implement any equality members.

Correct code based on your current code:

if(selectedItem != null)
    m_SelectedItem = m_Items.SingleOrDefault(Recover(selectedItem));
if(m_SelectedItem == null)
    m_SelectedItem = m_Items[0];

Update:

The code you uploaded has two problems.

  1. The value of the Process property of the default BrowserInstance object that you add if there is no process is null. This leads to a NullReferenceException in the comparison code used by SingleOrDefault.
    Fix it by changing the preceding if to

    if(selectedItem != null && selectedItem.Process != null)
    
  2. At the end of the Populate method you raise the PropertyChanged event for Items - to update the values in the combobox - and for SelectedItem - to set the selected item to the one the user had previously selected.
    The problem here is that WPF will update SelectedItem with null when PropertyChanged is raised for Items as it doesn't find the previously selected item in the new item list. This effectively overwrites the new selected item you computed in the Populate method.
    Fix it by not assigning the new selected item to m_SelectedItem but to selectedItem and assign that value to SelectedItem after the PropertyChanged event for Items was raised:

    public void Populate()
    {
        BrowserInstance selectedItem = m_SelectedItem;
        List<BrowserInstance> items = new List<BrowserInstance>();
    
        foreach (Process process in Process.GetProcessesByName("chrome"))
            items.Add(new BrowserInstance(process));
    
        if (items.Count > 0)
        {
            m_Enabled = true;
    
            m_Items = new ObservableCollection<BrowserInstance>(items.OrderBy(x => x.Process.Id));
    
            if (selectedItem != null && selectedItem.Process != null)
                selectedItem = m_Items.SingleOrDefault(x => (x.Process.Id == selectedItem.Process.Id) && (x.Process.MainModule.BaseAddress == selectedItem.Process.MainModule.BaseAddress));
    
            if (selectedItem == null)
                selectedItem = m_Items[0];
        }
        else
        {
            m_Enabled = false;
    
            m_Items = new ObservableCollection<BrowserInstance>();
            m_Items.Add(new BrowserInstance());
    
            selectedItem = m_Items[0];
        }
    
        NotifyPropertyChanged("Enabled");
        NotifyPropertyChanged("Items");
        SelectedItem = selectedItem;
    }
    

If you would properly implement equality for BrowserInstance you could make use of the WPF feature that keeps the currently selected item.
The code of Populate can be simplified like this:

public void Populate()
{
    BrowserInstance selectedItem = m_SelectedItem;
    List<BrowserInstance> items = new List<BrowserInstance>();

    foreach (Process process in Process.GetProcessesByName("chrome"))
        items.Add(new BrowserInstance(process));

    m_Enabled = items.Any();
    m_Items = new ObservableCollection<BrowserInstance>(items.OrderBy(x => x.Process.Id));
    if(!m_Enabled)
        m_Items.Add(new BrowserInstance());

    NotifyPropertyChanged("Enabled");
    NotifyPropertyChanged("Items");
    if (SelectedItem == null)
        SelectedItem = m_Items[0];
}

The equality implementation of BrowserInstance looks like this:

public sealed class BrowserInstance : IEquatable<BrowserInstance>
{

    // ...

    public bool Equals(BrowserInstance other)
    {
        if (ReferenceEquals(null, other))
            return false;
        if (ReferenceEquals(this, other))
            return true;
        if (m_Process == null)
        {
            if (other.m_Process == null)
                return true;
            return false;
        }

        if (other.m_Process == null)
            return false;

        return m_Process.Id == other.m_Process.Id && m_Process.MainModule.BaseAddress == other.m_Process.MainModule.BaseAddress;
    }

    public override bool Equals(object obj)
    {
        return Equals(obj as BrowserInstance);
    }

    public override int GetHashCode()
    {
        unchecked
        {
            return m_Process != null ? ((m_Process.Id.GetHashCode() * 397) ^ m_Process.MainModule.BaseAddress.GetHashCode()) : 0;
        }
    }
}
Daniel Hilgarth
  • 171,043
  • 40
  • 335
  • 443
  • Wow thanks! What is the best way to implement equality members on a custom class? My BrowserInstance class has only two fields: `System.Diagnostics.Process` Process and `System.String Name`... all I would have to to is compare the processes, but I can't find a way to properly check if two processes are identical. – Tommaso Belluzzo Apr 23 '13 at 13:11
  • @Zarathos: You would need to override `Object.Equals` and `Object.GetHashCode`. How exactly you perform the comparison is up to you and your program. It depends on how you define equality for a BrowserInstance. For example, you could use the logic from `Recover`. – Daniel Hilgarth Apr 23 '13 at 13:15
  • Mhhhh... I'm testing my code and I still see some problems. If I keep on pushing the Refresh button, for an unknown reason, the ComboBox selected item is blinking (sometimes it's empty, sometimes not). I can't understand the reason. I edited the question with my edited code following your suggestions. – Tommaso Belluzzo Apr 23 '13 at 15:21
  • @Zarathos: You didn't edit it the way I showed. Please note the difference between `selectedItem` and `m_SelectedItem` in my code. `selectedItem` has been taken from the previous version of your code and contains the item that has been selected before the user clicked "Refresh". – Daniel Hilgarth Apr 23 '13 at 15:24
  • Ahem sorry. Ok I modified it correctly now. But there is still something wrong that happens when I have a selected item and I click on the Refresh button very quickly. It should always keep the current one... but in fact sometimes SelectedItem is null and no value is selected from the ComboBox. It looks like after SingleOrDefault something bad happens and I can't select the first item in list or the selected item that I get from it is wrong. – Tommaso Belluzzo Apr 23 '13 at 15:44
  • Please add `if(m_SelectedItem == null) throw new InvalidOperationException("SelectedItem should never be null");` right before the calls to `NotifyPropertyChanged` in `Populate`. Please check if that exceptions gets thrown or not. – Daniel Hilgarth Apr 23 '13 at 15:47
  • @Zarathos: If you can upload a small sample that demonstrates this behavior I can more easily find the cause. – Daniel Hilgarth Apr 23 '13 at 15:48
  • Euh... can you just explain me why you used `m_Process.Id.GetHashCode() * 397` in `BrowserInstance.GetHashCode()`? – Tommaso Belluzzo Apr 23 '13 at 19:19
  • @Zarathos: Check this: http://stackoverflow.com/questions/102742/why-is-397-used-for-resharper-gethashcode-override – Daniel Hilgarth Apr 23 '13 at 19:21
  • Hi Daniel! Thank you very much again for your help. Testing my code, I found something really weird and hard to debug. Just try this: open Google Chrome with 3 tabs (each one pointing to a different website) and, from the ComboBox, select the only entry which have a window title... then hit Refresh: nothing happens as expected. Now, in Chrome, activate / bring to front another tab, hit Refresh and open the ComboBox dropdown: you will notice that the selected entry in the list has a different title. WTF?! Do you have any idea? http://www.filedropper.com/hackse – Tommaso Belluzzo Apr 24 '13 at 00:23
  • @Zarathos: The reason is simple: You only compare the process ID and the base address but you don't compare the name. You need to add that, too. – Daniel Hilgarth Apr 24 '13 at 08:10