Loading sub items of TreeView in background

I am working with a TreeView (default control from .Net Framework) that displays hierachical data. Data are bound to the view using MVVM pattern with the HierarchicalDataTemplate.

The child items are loaded automatically when the view requests the child collection. To stay responsive, the child items are loaded in background up to a specific depth:

public class TreeItemViewModel : ViewModelBase
{
    private readonly ObservableCollection myChildren = new ObservableCollection();
    private const int PRELOADING_DEPTH = 1;

    public string Name { get; set; }
    public bool IsLoaded { get; set; }
    public bool IsLoading { get; set; }

    public ObservableCollection Children
    {
        get
        {
            ThreadPool.QueueUserWorkItem(o => EnsureChildrenAreLoaded(myChildren, 0));
            return myChildren;
        }
    }

    public void EnsureChildrenAreLoaded(ObservableCollection childrenToFill, int depth)
    {
        if (!IsLoaded && !IsLoading)
        {
            IsLoading = true;
            var children = LoadChildren().ToArray();
            App.RunOnGuiThread(() =>
            {
                foreach (var child in children)
                    childrenToFill.Add(child);
                IsLoaded = true;
                IsLoading = false;
            });
        }
        if (depth  LoadChildren()
    {
        // logic for loading sub items
        return Enumerable.Empty();
    }
}

RunOnGuiThread:

public static void RunOnGuiThread(Action action)
    {
        if (Current == null || Current.Dispatcher == null)
            action();
        else
            Current.Dispatcher.Invoke(action);
    }

Is that a proper solution? Has it any disadvantages / potential for improvements?

I do not see your full UI then maybe you're using it (to display a busy indicator?) but, from what I have here, it seems that IsLoading is just a repetition for IsLoaded and as such it should be dropped. Any reason for EnsureChildrenAreLoaded() to be public and TreeItemViewModel not to be sealed?

EnsureChildrenAreLoaded() might be:

var children = LoadChildren().ToArray();
App.RunOnGuiThread(() =>
{
    foreach (var child in children)
        childrenToFill.Add(child);

    IsLoaded = true;
});

However we now have the problem to preload the children. My major concern here is that you're queuing an action in the pool each time you read the property. Not such big overhead but avoidable. Also I'd move this responsibilities to separate methods:

void LoadChildrenAndUpdateUi(ObservableCollection childrenToFill)
{
    IsLoaded = true;

    var children = LoadChildren().ToArray();
    App.RunOnGuiThread(() =>
    {
        foreach (var child in children)
            childrenToFill.Add(child);
    });
}

And:

ObservableCollection EnsureChildrenAreLoaded(
    ObservableCollection childrenToFill, int depth)
{
    if (depth >= PRELOADING_DEPTH)
        return;

    if (!IsLoaded)
        ThreadPool.QueueUserWorkItem(_ => LoadChildrenAndUpdateUi(myChildren));

    foreach (var child in myChildren)
        child.EnsureChildrenAreLoaded(child.myChildren, depth + 1);

    return myChildren;
}

Our getter will then be:

public ObservableCollection Children
    => EnsureChildrenAreLoaded(myChildren, 0);

Major difference is that we're going through the children to determine if they have to be loaded in the calling UI thread but we're queuing an action in the thread pool only when required. Note that we're using only one property to track the state (IsLoaded, which should be private). I set its value to true before effectively reading children to avoid multiple parallel requests, name is now somehow misleading and should be changed to something meaningful (_isVisited or something like that).

What next? If PRELOADING_DEPTH might ever be 0 or loading time may be really long then I'd add a dummy item to the child collection (to display the expand/collapse indicator), something like:

public TreeItemViewModel()
{
    Children.Add(new TreeItemViewModel { Name = "Loading..." });
}

Removed before you start adding new items:

children.Clear();
foreach (var child in children)
    childrenToFill.Add(child);

But, well, you already have a busy indicator then you don't need this at all!

Edit: if you're using IsLoading in your UI then, unfortunately, you can't drop it that easily. I think you have two options: replace IsLoading and IsLoaded with an enum like enum LoadingStatus { NotLoaded, Loading, Loaded } used in conjunction with a value converter to hide/show the busy indicator or use ordering (as you already doing) to avoid locks (because fortunately you have only one thread for writing):

var children = LoadChildren().ToArray();
App.RunOnGuiThread(() =>
{
    foreach (var child in children)
        childrenToFill.Add(child);

    IsLoaded = true;
    IsLoading = false;
});

Set IsLoading in the UI thread to avoid race conditions (both read and write will be then done in the same UI thread):

if (!IsLoaded && !IsLoading)
{
    IsLoading = true;
    ThreadPool.QueueUserWorkItem(_ => LoadChildrenAndUpdateUi(myChildren));
}

Final notes.

If your depth is limited to direct children then you may simplify your implementation (just invoke the lazy load method when you populate the list). You may also want to try to use a LazyAsync implementation (I saw a nice one somewhere...can't find/remember where).

WPF supports asynchronous binding with a simple IsAsync=true in the binding expression. You may want to experiment with that, see MSDN for details

StackExchange Code Review Q#127999, answer score: 3