AvaloniaUI / Avalonia

Develop Desktop, Embedded, Mobile and WebAssembly apps with C# and XAML. The most popular .NET UI client technology
https://avaloniaui.net
MIT License
25.62k stars 2.22k forks source link

The initialisation sequence of XAML bindings and WhenActivated has changed #2845

Open aguahombre opened 5 years ago

aguahombre commented 5 years ago

Using build 0.8.999-cibuild0003548-beta. The creation of the view's XAML bindings now occurs before the WhenActivated call in the view model. In v0.8.1, WhenActivated was called before the XAML bindings were made.

The original sequence feels correct to me as you can use the WhenActivated call to setup view model properties that can then be bound in XAML without implementing INotifyPropertyChanged. This is how DynamicData (ReactiveUI for collections) with ReadOnlyObservableCollection works, requiring no INotifyPropertyChanged code.

This sequence change causes failure to display the items generated using DynamicData Bind when called from WhenActivated. This previously worked in v0.8.1. Here is an example of a view / view model that worked in v0.8.1 but now creates the ListBox bound to null instead of the generated list:

<UserControl xmlns="https://github.com/avaloniaui"
             xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
             xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
             xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
             mc:Ignorable="d" d:DesignWidth="800" d:DesignHeight="450"
             x:Class="AvaloniaApplication8.Views.MyView">
  <StackPanel>
    <TextBlock Text="Here is my list of 3 items:" />
    <ListBox Items="{Binding Items}"/>
  </StackPanel>
</UserControl>
    public class MyViewModel : ReactiveObject, IRoutableViewModel, ISupportsActivation
    {
        public string UrlPathSegment => "MyView";

        public IScreen HostScreen { get; set; }

        public ViewModelActivator Activator { get; } = new ViewModelActivator();
        private ReadOnlyObservableCollection<string> _items;
        public ReadOnlyObservableCollection<string> Items => _items;

        public MyViewModel(IScreen screen, MyModel model)
        {
            HostScreen = screen;

            this.WhenActivated(disposables =>
            {
                model.Values.ToObservableChangeSet()
                    .ObserveOn(RxApp.MainThreadScheduler)
                    .Bind(out _items)
                    .Subscribe()
                    .DisposeWith(disposables);

                //this.RaisePropertyChanged(nameof(Items));
            });
        }
    }

Here is the full repo example. App8.zip

Adding the this.RaisePropertyChanged(nameof(Items)); line fixes the problem in build 0.8.999-cibuild0003548-beta.

So my question is whether : a) the initialisation sequence behaviour is undefined and I should not rely on the initialisation sequence being in any particular order? or b) is this a breaking change? or c) is it a bug?

kekekeks commented 5 years ago

@worldbeater

kekekeks commented 5 years ago

The change is probably caused by the switch to top-down initialization order

x2bool commented 5 years ago

I would also appreciate defined initialization sequence. This change is pretty big.

worldbeater commented 5 years ago

We use AttachedToVisualTree and DetachedFromVisualTree events for IVisuals and Opened and Closed events for Windows to handle activation and deactivation, and that shouldn't have changed since 0.8.1 https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.ReactiveUI/AvaloniaActivationForViewFetcher.cs#L61 So looks like the behaviour of visual tree events changed recently.

Worth investigating how activation works on WPF etc. but generally a call to RaisePropertyChanged looks reasonable, while on most platforms activation happens after view model initialization, and the _items property is left unassigned when the initialization completes

aguahombre commented 5 years ago

The workaround using RaisePropertyChanged gets quite messy when you want to bind an initial SelectedItem to the ItemsControl.

<ListBox Items="{Binding Items}" SelectedItem="{Binding MySelection}"/>

As the Items binding returns null when the SelectedItem binding is made, the ItemsControl sets the SelectedItem back to null as it doesn't match any item in the Items.

When WhenActivated is called and the Items gets updated by RaisePropertyChanged, the SelectedItem is now null and the ItemsControl selects the first item in the Items. This is not the required behaviour.

My current workaround involves saving the current selection and restoring it after RaisePropertyChanged

                var selected = MySelection;
                this.RaisePropertyChanged(nameof(Items));
                MySelection= selected ;

I suspect this will not be the only messy side affect of this change.

worldbeater commented 5 years ago

@aguahombre another solution is to avoid using imperative code inside the WhenActivated block and to prefer handling e.g. the initial selection change event as such:

list.OnItemAdded(x => Selection = Items.First())
    .OnItemRemoved(x => Selection = Items.FirstOrDefault())
    .Subscribe();

Yeah this feels a bit hacky and full of side-effects, so probably worth using the WhenAny-based approach:

this.WhenAnyValue(x => x.Items)
    .Where(items => items != null)
    .Select(items => items.FirstOrDefault())
    // Use .Take(1) to handle initial load only
    .Subscribe(item => Selection = item);

If you are operating on a mutable data set with nested collections, then another option is to implement IDisposable manually and to use the .DisposeMany() DynamicData operator to ensure all elements are disposed when they get removed from the source collection https://github.com/RolandPheasant/Dynamic.Trader/blob/master/Trader.Client/Views/PositionsViewer.cs#L19 This option doesn't require a call to WhenActivated, so the related issues should go away.

Also head over to ReactiveUI Slack where you can ask DynamicData maintainers (#dynamicdata channel) about how they implement the desired behavior on e.g. WPF or UWP, where activation also happens later then initialization. Probably additional operators or best practices exist that could help (in case if the behaviour of Avalonia AttachedToVisualTree events won't change back to the behaviour we had in 0.8.1 and below)

kekekeks commented 5 years ago

@grokys it seems that described SelectingItemsControl behavior with resetting SelectedItem is a bug in SelectingItemsControl.

RomanSoloweow commented 4 years ago

WhenActivated on child elements fires before WhenActivated on parent elements. In WPF vice versa