Where to load data, in constructor or by event

[drei97]

Hello,

actually I load data which have to be shown in my view/DataGrid/ComboBox in the event handler for AttachedToVisualTree, not in the constructor of the viewmodel.

A ComboBox in my view shows via ItemsSource a list of entries which where also loaded in AttachedToVisualTree. But the items cannot be selected. It seems they are not known, even if they are shown by the ItemsPanel. In detail, SelectionChanged is not triggered, the selected item is not shown in the textpanel, even if SelectedItem is set.

The datagrid is filled correctly!

When I transfer the loading of the data into the constructor, all items are shown in the ItemsPanel and they can be selected, ... means the selected item is then shown in the textpanel, SelectionChanged is triggered.

1. Is this a bug, that the ItemsPanel of ComboBox is showing items for selection but they cannot be selected effectively? Why is SelectionChanged not triggered? This seem weird to me.

2. What is better in principle, loading (a big amount of) data in the constructor or in the handler of AttachedToVisualTree?

-> Avalonia 11.2.3

[msneijders]

The behavior you see is not normal. You should show a minimal repro project.

My pattern for doing viewmodel loading is like this:

(Based on new Avalonia project template using CommunityToolkit) and v11.2.3:

public partial class MainView : UserControl
{
    public MainView()
    {
        InitializeComponent();

        this.combobox.SelectionChanged += Combobox_SelectionChanged;
    }

    protected override void OnLoaded(RoutedEventArgs e)
    {
        base.OnLoaded(e);

        _ = (DataContext as MainViewModel)!.LoadAsync();
    }

    private void Combobox_SelectionChanged(object? sender, SelectionChangedEventArgs e)
    {
        System.Diagnostics.Debug.WriteLine($"Selection changed: {e.AddedItems[0]}");
    }
}

public partial class MainViewModel : ViewModelBase
{
    [ObservableProperty]
    string[]? _items;

    public async Task LoadAsync()
    {
        var loadedItems = await Task.Run(() =>
        {
            // Loading large amount of data from http endpoint or file, or doing calculations
            // doing it on background thread so UI will not freeze.
            return new string[] { "Item 1", "Item 2", "Item 3" };
        });

        Items = loadedItems;
    }
}

And in MainView.axaml:

<ComboBox x:Name="combobox" ItemsSource="{Binding Items}" />

The combobox then shows all items, and selecting triggers Combobox_SelectionChanged. But perhaps I am misunderstanding what you are doing.

[thevortexcloud]

The code you have provided has some issues. The main ones being:

1. You are not awaiting the task which means any exceptions will be ignored
2. You are making changes to things the view is bound to on a thread which not guaranteed to be a UI thread (tasks can return to a completely different thread than the one that started them)

Both issues are easy to fix. Just uses the Loaded event in your view with an async handler to await your call to the view model instead of overriding the method, and then update the Items property with a Dispatcher call.

Alternatively you can also use ReactiveUI which has built in features to handle this sort of thing for you.

[drei97]

update the Items property with a Dispatcher call.

This was my mistake. I now put the call of RaiseNotifyChanged for the property ItemsSource into a Dispatcher and it works.

Thnx

[msneijders]

As you see, showing some code is always thought-provoking ;p.

@thevortexcloud If you wouldn't mind: Am I wrong to assume the AvaloniaSynchronizationContext makes sure the await continuation stays on the original main thread (or maybe with an explicit ConfigureAwait(true))? Or maybe that does not work on all platforms? (would like to know, as without explicit dispatcher makes the code a bit better readable imo).

[thevortexcloud]

Honestly I am not sure about that. My knowledge of Avalonia's internals is not great. When I wrote that it was a somewhat general statement. Someone on the core team can probably answer that question though. But updating bound properties from the UI thread is required to avoid weirdness. So explicitly starting a background task and making changes will also likely cause you problems. The same also applies to WPF.

[msneijders]

ty for your honest answer, and whatever that SynchronizationContext is doing, the core thing is as you state:

But updating bound properties from the UI thread is required to avoid weirdness

And its an easy and common mistake to make.

Therefor when using a background thread for a task, as a general rule, do not directly manipulate your viewmodels; use that thread to gather data, and get it back to the main thread (dispatcher), and then use that data to change the viewmodels accordingly.

[thevortexcloud]

Are you able to share some code? It would make it a lot clearer what is happening.

[drei97]

                            <ComboBox Grid.Column="1" Grid.Row="3"
                                      Name="SelectCategory"
                                      ItemsSource="{Binding Kategorien}"
                                      SelectionChanged="SelectCategory_OnSelectionChanged" />

    private async void Visual_OnAttachedToVisualTree(object sender, VisualTreeAttachmentEventArgs e)
    {
        await ViewModel.LoadAsync();
    }

    public async Task LoadAsync()
    {
        await Task.Run(() => Load());
    }

    public void Load()
    {
        Data = new ObservableCollection<DocumentsDocumentEntity>(_dokumente!.Dokumente.OrderBy(xx => xx.Date));
        SelectedItem = Data.LastOrDefault();
        if (SelectedItem != null)
            Dispatcher.UIThread.InvokeAsync(() =>
                View!.DataGrid.ScrollIntoView(SelectedItem, null));
        
        Kategorien.AddRange(Data.Select(xx => xx.Category!).Distinct().OrderBy(xx => xx));
        Dispatcher.UIThread.InvokeAsync(() =>
        {
            this.RaisePropertyChanged("Kategorien");
        });
    }

[thevortexcloud]

This code also has some issues for a couple reasons:

1. It violates MVVM. Viewmodels must not be aware of the view or anything on it. Meaning the view model can't directly access the datagrid. You can instead do your scroll logic in your code behind after the data has loaded.
2. You probably (although this is not necessarily wrong) don't want to recreate the observable collection each time. Instead you want to clear it and then add to it. This is because an observable collection raises a change event each time it changes which ensures the view gets updated properly. When you call RaisePropertyChanged you are actually saying that Kategorien itself has changed, rather than its contents.
3. Also as a minor nitpick, use nameof when using RaisePropertyChanged to make it a strongly typed reference which will generate compile errors if the property name ever changes/the property gets removed.

[drei97]

Sry, I have to reopen the thread. I had not removed all temporary code and this had "effects" I initially didn't recognize and therefore I was fooled by the code. It was a fake success!

I still have the problem that if I am feeding the ItemsSource property of my ComboBox in the event handler of AttachtoVisualTree the ComboBox is not working properly. As mentioned above, by clicking on the down symbol, the ItemsPanel is opened, I can hover and the items are highlighted, I can click on any listed item, ... but nothing happens then. No SelectionChanged is triggered, SelectedItem is not altered.

Moving it into the constructor of the ViewModel, everything is OK.

Here my current code (mostly identical, some changes according to the suggestions here, to my first post. But maybe you have some ideas if you see it again.

View XAML:

<ComboBox Grid.Column="1" Grid.Row="3"
          Name="SelectCategory"
          ItemsSource="{Binding Categories}"
          SelectedItem="{Binding SelectedCategory}"
          SelectionChanged="SelectCategory_OnSelectionChanged" />

View CS:

    private async void Visual_OnAttachedToVisualTree(object sender, VisualTreeAttachmentEventArgs e)
    {
        await ViewModel.LoadAsync();
        if (ViewModel.SelectedItem is not null)
            DataGrid.ScrollIntoView(ViewModel.SelectedItem, null);
    }

    private void SelectCategory_OnSelectionChanged(object? sender, SelectionChangedEventArgs e)
    {
        Logger?.Debug(this, "Selection Changed");
        if (sender is ComboBox cb)
        {
            Logger?.Debug(this, $"SelectedItem {cb.SelectedItem}");
            if (cb.SelectedItem?.ToString() == DocumentsViewModel.NewItemIdentifier)
            {
                ViewModel.NewCategory();
            }
        }
    }

ViewModel:

    public async Task LoadAsync()
    {
        await Task.Run(() => Load());
    }

    public async void Load()
    {
        Data = new ObservableCollection<DocumentsDocumentEntity>(_documents!.Dokumente.OrderBy(xx => xx.Date));
        SelectedItem = Data.LastOrDefault();
        Categories.Clear();
        Categories.Add(NewItemIdentifier);
        Categories.AddRange(Data.Select(xx => xx.Category!).Distinct().OrderBy(xx => xx));
        await Dispatcher.UIThread.InvokeAsync(() => { this.RaisePropertyChanged(nameof(Categories)); });
    }

I am not sure if these awaits and asyncs are all necessary. I am still waiting to get a better understanding of this asynchronous stuff.

Nevertheless, I have no idea, why this issue happens.

[drei97]

Your view model is a bit unclear from this snippet. (what is property Data and SelectedItem?)

I see! The view contains a DataGrid which is filled by the Data property. SelectedItem is also from the DataGrid. Beside the DataGrid there are also some TextBoxes and this ComboBox under construction. The ComboBox is to select "Categories".

I would not set any viewmodel properties inside the Task.Run, but let it return your data (i.e. the part Data.Select(xx => xx.Category!).Distinct().OrderBy(xx => xx))

this part makes no trouble. The DataGrid is filled correctly.

-- and then set Categories outside of the task.run (or dispatched to the uithread).

Putting all about the ComboBox into a Dispatcher in the Load method makes no difference!

I am wondering why such a common task, to get a ComboBox working, makes such trouble. My code is, obviously, not that wrong that it is seen by one eye what the issue is. I don't want to offend you, but your suggestions, sry to say that, sounds like guessing. Try this, try that. I am playing around since two days now and no solution for such a standard situation. curious.

I don't want to waste more time on this. Since loading all data in the constructor of the view is doing the job I will take that as solution, even if it is maybe against some good practice.

I am very interested in understanding what is wrong here, but it seems we all have no real clue and we are stuck now.

Any new idea is still welcome.

Thanks for your patience.

[stevemonaco]

Guessing is the best any member can do. There are at least 3 major components in play (XAML, code-behind, and VM) plus a number of footguns externally. No repro and the fractured source shown means anybody helping you will need to make some assumptions.

You're using Task.Run which offloads work to another threadpool thread, but then you set VM properties which we can only assume implement standard PropertyChanged practices that are binded to the View. This will trigger synchronizations while off the UI thread and will fail. Silently because you're using async void. That should only ever be used on event handlers such as your async void Visual_OnAttachedToVisualTree where there's no possibility to return Task or Task<T>. This is a lot of trouble for you because you're using async and don't understand it, especially in the context of UI. It's something that takes practice to understand. I'd recommend reading some of Stephen Toub's written articles or the video "Deep .NET: Writing async/await from scratch in C# with Stephen Toub and Scott Hanselman".

[thevortexcloud]

Adding to this: Async (and by extension mutithreading) is in can be very complicated to get right. It's fairly easy to get things like deadlocks if you don't know what you are doing.

https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

Since loading all data in the constructor of the view is doing the job I will take that as

I assume you are not using async at all if you are loading things in the constructor? If you are then then the fact it's working is probably a coincidence/race condition. If it's not a race condition then it's probably working because the view model has to be constructed before the view can bind to it, so all the data is available by the time it tries to bind to it. Anyway, unless you have many items/those items are slow to generate (EG they need to be fetched from a server) you are probably not benefiting in any way from using async.

The following code works fine for me:

View:

      <StackPanel Orientation="Vertical">
            <ComboBox ItemsSource="{CompiledBinding ComboItems}" />
            <Button Command="{CompiledBinding AddItemCommand}" Content="Add item"/>
        </StackPanel>

View Model:

 public ObservableCollection<string> ComboItems { get; } = new ObservableCollection<string>();
    public ICommand AddItemCommand { get; } 

    private void AddItem() {
        this.ComboItems.Add(Random.Shared.Next(0, 100).ToString());
    }

    public MainWindowViewModel() {
        AddItemCommand  = ReactiveCommand.Create(AddItem);
        
        this.ComboItems.Add("Test");
        this.ComboItems.Add("Test2");
    }

It also works if I make it do things on loaded and async:

View model

public Task AddItemsAsync() {
       return Task.Run(() => {
           for (int i = 0; i < 100; i++) {
               this.ComboItems.Add(i.ToString());
           }
       });
    }

View:

 public MainWindow() {
        InitializeComponent();
        this.Loaded += OnLoaded;
    }

   private async void OnLoaded(object? sender, RoutedEventArgs e) {
        await ((MainWindowViewModel)this.DataContext).AddItemsAsync();
    }

However if I use the attached to visual tree event I can also clearly see the event never fires and as a result it never calls my method to load data. Which sort of makes sense as the window can't attach to itself. As such what is your view? A Window? A user control?

[drei97]

Guessing is the best any member can do.

I know

You're using Task.Run ... This is a lot of trouble for you because you're using async and don't understand it, especially in the context of UI.

That's a big point.

It's something that takes practice to understand. I'd recommend reading some of Stephen Toub's written articles or the video "Deep .NET: Writing async/await from scratch in C# with Stephen Toub and Scott Hanselman".

Thanks you for the hints. I will have a look.

Since loading all data in the constructor of the view is doing the job I will take that as

I assume you are not using async at all if you are loading things in the constructor?

No, it is not done asynchronously

...

public ObservableCollection<string> ComboItems { get; } = new ObservableCollection<string>();

It seems I have found my mistake. It has nothing to do with threading, async or await. It is much more simple. The ItemsSource of my ComboBox was List<string>. Changing to ObservableCollection<string> was the trick. The line above was the key!

Oh dear, what a pain.

Thank you all.

[thevortexcloud]

Changing to ObservableCollection was the trick. The line above was the key!

That would do it. You can bind to a list but any changes you make to it wont be synced back to the UI. Which effectively makes them a one time binding. Which would explain why your constructor worked. If you bind to a INotifyCollectionChanged (such as an ObservableCollection), Avalonia will subscribe to change notifications from it and automatically update the UI when the collection changes.

Thank you all.

Glad I could help.