3
\$\begingroup\$

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.

<HierarchicalDataTemplate DataType="{x:Type models:TreeItemViewModel}" ItemsSource="{Binding Children, Mode=OneWay}">
    <TextBlock Text="{Binding Name}" />
</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<TreeItemViewModel> myChildren = new ObservableCollection<TreeItemViewModel>();
    private const int PRELOADING_DEPTH = 1;

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

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

    public void EnsureChildrenAreLoaded(ObservableCollection<TreeItemViewModel> 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 < PRELOADING_DEPTH)
            foreach (var child in myChildren)
                child.EnsureChildrenAreLoaded(child.myChildren, depth + 1);
    }

    protected virtual IEnumerable<TreeItemViewModel> LoadChildren()
    {
        // logic for loading sub items
        return Enumerable.Empty<TreeItemViewModel>();
    }
}

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?

\$\endgroup\$
  • \$\begingroup\$ A single edit after a year and you already got two answers ;-) \$\endgroup\$ – t3chb0t Aug 14 '17 at 7:47
  • 2
    \$\begingroup\$ Monday morning seems to be a good time for asking questions ;) \$\endgroup\$ – JanDotNet Aug 14 '17 at 7:51
3
\$\begingroup\$

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<TreeItemViewModel> childrenToFill)
{
    IsLoaded = true;

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

And:

ObservableCollection<TreeItemViewModel> EnsureChildrenAreLoaded(
    ObservableCollection<TreeItemViewModel> 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<TreeItemViewModel> 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<T> 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

\$\endgroup\$
  • \$\begingroup\$ Thanks for your review. 1) Unfortunately, IsLoading is used to updated a busy indicator (as you correctly guessed ;)). Therefore it is not possible to drop the property. \$\endgroup\$ – JanDotNet Aug 14 '17 at 8:11
  • \$\begingroup\$ 2) I like the way you restructured the reloading logic. It seems to be more reasonable. However, The condition should be depth > PRELOADING_DEPTH, isn't? \$\endgroup\$ – JanDotNet Aug 14 '17 at 8:15
  • 1
    \$\begingroup\$ I am worry wheather "as soon as possible" is fast enough to AVOID race conditions :). What do you think about adding another instance variable of type int and use Interlocked.CompareExchange to set a loading flag as atomic operation and avoid locking!? \$\endgroup\$ – JanDotNet Aug 14 '17 at 8:37
  • 2
    \$\begingroup\$ @JanDotNet quite! As long as there is only one thread modifying the values (which you don't seem to be enforcing at the moment; an Interlocked flag would work well for such exclusion) and you don't really care about when the updates reach the UI thread then you don't need to lock anything. You might consider throwing in a memory barrier after LoadChildren() and before the foreach to ensure uptodateness (though I'd be amazed if Invoke didn't imply this already). If you do want to lock something, stick to lock (unless performance is critical, or it is impossible). enum is a good idea. \$\endgroup\$ – VisualMelon Aug 14 '17 at 8:46
  • 1
    \$\begingroup\$ @JanDotNet I knew it wasn't clear (that's why italic) but I didn't find a better way to express the concept succinctly. I mean: it must be done before effectively starting the operation. Note that in this way read/write are in the same (UI) thread then there isn't any race with that flag, you do not need any synchronization primitive in this case (and code shouldn't be reordered in any way for currently supported .NET architectures, if in doubt...I think VisualMelon is right and a memory barrier is all you need) \$\endgroup\$ – Adriano Repetti Aug 14 '17 at 8:52
2
\$\begingroup\$

I am really unsure why you have a public access modifier for the EnsureChildrenAreLoaded method. I don't see any reason from the code why you would need it, and I think it should be at most protected, but preferably private.

For the IsLoading and IsLoaded members, I don't see why they should be members, and why you should be able to assign a value from outside your class. I think the should be private fields inside your class, cause nobody except the current TreeItemViewModel would care if they are loaded or not (and if they would, even then, I wouldn't allow it to be set from the outside)

To the outside, I would tend to give out only IList<TreeItemViewModel>, and keep the implementation of it hidden from the users. WPF will detected the ICollectionChanged interface, and will update on any changes (as long as you don't reassign)

I am also unsure if the Name of the TreeItemViewModel supports updates to the UI, if it does, it seems a part of the INotifyPropertyChanged is not yet implemented for it.

\$\endgroup\$
  • \$\begingroup\$ AFAIK an observable collection (instead of IList is required to support automatic update when the collection is updated. Lock isn't necessary at all for a boolean property (per .NET memory model). Name isn't never updated after item is created then it doesn't need to be observable. \$\endgroup\$ – Adriano Repetti Aug 14 '17 at 7:45
  • \$\begingroup\$ @AdrianoRepetti Actually, IList is enough, as long as you define the children as ObservableCollection<T>, wpf will detect the ICollectionChanged interface upon the binding. For the boolean property you are right. For the name, as I mentioned, I wasn't sure if you wanted it to updated ;) \$\endgroup\$ – Icepickle Aug 14 '17 at 7:54
  • \$\begingroup\$ Thx, agree that EnsureChildrenAreLoaded and setter of IsLoaded / IsLoading should be private and properties should support change notification. Actually IsLoading does it in the current implementation ;). \$\endgroup\$ – JanDotNet Aug 14 '17 at 7:56
  • \$\begingroup\$ @AdrianoRepetti: Lock isn't necessary at all for a boolean property (per .NET memory model): Could you explain that? After reviewing it again, reading IsLoading and writing IsLoading are 2 distinct steps and therefore it is "critical section", isn't it? \$\endgroup\$ – JanDotNet Aug 14 '17 at 8:01
  • 2
    \$\begingroup\$ @Icepickle because it's a code review, if something in code might be simplified then I think it should be (well...or at least we should talk about it). In this case IMO there isn't the need to have them both. \$\endgroup\$ – Adriano Repetti Aug 14 '17 at 8:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.