0

I have a ListView with an ObservableCollection. The class User has a List with Groups. I have another ListView which shows the Groups of the selected User. Now i try to add/delete groups, but they don't refresh in realtime. If i select another user and back to my user, i see the updated list.

XAML:

    <ListView ItemsSource="{Binding ListUsers}" SelectedItem="{Binding SelectedUser}">
        <ListView.ItemTemplate>
            <DataTemplate>
                <TextBlock Text="{Binding Name}"/>
            </DataTemplate>
        </ListView.ItemTemplate>
    </ListView>

    [...]

    <ListView ItemsSource="{Binding SelectedUser.Groups}" SelectedItem="{Binding SelectedGroup}">
        <ListView.ItemTemplate>
            <DataTemplate>
                <TextBlock Text="{Binding}"/>
            </DataTemplate>
        </ListView.ItemTemplate>
    </ListView>

Class:

public class User : ObservableObject
{
    private string name;

    public string Name
    {
        get => name;
        set => SetProperty(ref name, value);
    }

    private ObservableCollection<string> groups;
    public ObservableCollection<string> Groups
    {
        get => groups;
        set => SetProperty(ref groups, value);
    }
}

ViewModel:

private ObservableCollection<User> listusers;
public ObservableCollection<User> ListUsers
{
    get => listusers;
    private set => SetProperty(ref listusers, value);
}

private User _selectedUser;
public User SelectedUser
{
    get => _selectedUser;
    set => SetProperty(ref _selectedUser, value);
}

private string _selectedGroup;
public string SelectedGroup
{
    get => _selectedGroup;
    set => SetProperty(ref _selectedGroup, value);
}

public async Task DelGroup()
{
    await Task.Run(() =>
    {
        //SelectedUser.Groups.Remove(SelectedGroup);

        for (int i = 0; i < ListUsers.Count; i++)
        {
            if (SelectedUser.SAMAccountName == ListUsers[i].SAMAccountName)
            {
                ListUsers[i].Groups.Remove(SelectedGroup);
            }
        }
    });
}
DerKiLLa
  • 135
  • 12

3 Answers3

1

Using a background thread only to move the work to the Dispatcher results in bad performance and should be avoided. At best, the performance remains the same as it would be when deleting the group directly on the UI thread using your code. This means no improvement.

You will get the best performance if you create and maintain a lookup table where the SAMAccountName is the key and the value a collection of User items of that SAMAccountName (in case your table doesn't allow duplicate keys):

public ObservableCollection<User> ListUsers { get; }
private Dictionary<string, IList<User>> UserTable { get; }

public ViewModel()
{
  this.ListUsers = new ObservableCollection<User>();
  this.ListUsers.ColectionChanged += OnUsersChanged;
}

public void DelGroup()
{
  if (!this.UserTable.TryGetValue(this.SelectedUser.SAMAccountName, out IList<User>> usersWithEqualName)
  {
    return;
  }
  foreach (var user in usersWithEqualName)
  {
    user.Group.Remove(this.SelectedGroup);
  }
}

private void OnUsersChanged(object? sender, NotifyCollectionChangedEventArgs e)
{
  switch (e.Action)
  {
    case NotifyCollectionChangedAction.Add:
      foreach (var newUser in e.NewItems.Cast<User>())
      {
        AddUserToLookupTable(newUser);
      }
      break;
    case NotifyCollectionChangedAction.Remove:
    case NotifyCollectionChangedAction.Replace:
      foreach (var oldUser in e.OldItems.Cast<User>())
      {
        RemoveUserFromLookupTable(oldUser);
      }
      break;
    case NotifyCollectionChangedAction.Reset:
      this.UserTable.Clear();
      break;
  }
}

private void AddUserToLookupTable(User user)
{
  if (!this.UserTable.TryGetValue(user.SAMAccountName, out IList<User>> usersWithEqualName)
  {
    usersWithEqualName = new List<User>();
    this.UserTable.Add(user.SAMAccountName, usersWithEqualName);
  }
  usersWithEqualName.Add(user);
}

private void RemoveUserFromLookupTable(User user)
{
  if (!this.UserTable.TryGetValue(user.SAMAccountName, out IList<User>> usersWithEqualName)
  {
    return;
  }
  usersWithEqualName.Remove(user);
  if (!usersWithEqualName.Any())
  {
    this.UserTable.Remove(user.SAMAccountName);
  }
}

A more simple solution would be to ensure that User.Group is the same instance for all User instances. This way, removing a group from one User.Group collection, will remove it from all. This would eliminate the above solution and even perform better. But this solution is only feasible if all User instances have the same groups.

BionicCode
  • 1
  • 4
  • 28
  • 44
-1

Found the answer:

Application.Current.Dispatcher.Invoke(() => SelectedUser.Groups.Remove(SelectedGroup));
DerKiLLa
  • 135
  • 12
  • Please don't do this. This results in very bad performance. Don't create a background thread just to execute all the work on the Dispatcher. The complete DelGroup method as it is must be executed on the UI thread. Instead optimize the loop and try to collect or track the items to remove instead of searching them. Creating a lookup table can help. If the list is very big and you don't know how to improve finding the items to delete, then filter and collect on the background thread. Then delete the collected items on the UI thread. – BionicCode May 12 '22 at 12:58
  • You can also use LINQ to filter the collection. LINQ allows deferred execution... – BionicCode May 12 '22 at 13:02
  • You will get the best performance if you create a lookup table where the SAMAccountName is the key and the value a collection (in case your table doesn't allow duplicate keys). – BionicCode May 12 '22 at 13:08
-1

You need to invoke the action through the dispatcher because you start it from a ThreadPool thread - using Task.Run.
You could drop the Dispatcher invokation and still benefit from a non blocking asyc call, if you rewrite, so that the modifying UIElement access happens on the Main thread.
For example like:

public async Task DelGroup()
{
    foreach (int index in await Task.Run(GetRemoveIndices).ConfigureAwait(true)) {
        ListUsers[index].Groups.Remove(SelectedGroup);
    };

    IEnumerable<int> GetRemoveIndices() {
        for (int i = ListUsers.Count-1; i >= 0; i--) {
            if (SelectedUser.SAMAccountName == ListUsers[i].SAMAccountName) {
                 yield return i;
            }
        }
    }    
}
lidqy
  • 1,891
  • 1
  • 9
  • 11
  • `ConfigureAwait(true)` is the default. Your solution creates a nested loop which results in a bad performance and which is totally not necessary. Also the yield return inside the thread won't do what you expect it to do. The Task will return after the complete GetRemoveIndices has returned. Filter the collection using LINQ instead. If you make sure to return an IEnumerable, then the LINQ expresion will be evaluated in the foreach (deferred), saving you an extra iteration. – BionicCode May 12 '22 at 13:05
  • You will get the best performance if you create a lookup table where the SAMAccountName is the key and the value a collection (in case your table doesn't allow duplicate keys). – BionicCode May 12 '22 at 13:08
  • `ConfigureAwait(true)` is default and most coding guidelines and tools recommend its explicit usage. Did you not know? What a pitty, lol. – lidqy May 12 '22 at 13:19