diff --git a/src/Perspex.Base/PerspexObject.cs b/src/Perspex.Base/PerspexObject.cs index 7c1317a7bc..337fbf185d 100644 --- a/src/Perspex.Base/PerspexObject.cs +++ b/src/Perspex.Base/PerspexObject.cs @@ -807,18 +807,33 @@ namespace Perspex newValue, priority); - OnPropertyChanged(e); - property.NotifyChanged(e); - - if (PropertyChanged != null) + if (property.Notifying != null) { - PropertyChanged(this, e); + property.Notifying(this, true); } - if (_inpcChanged != null) + try { - PropertyChangedEventArgs e2 = new PropertyChangedEventArgs(property.Name); - _inpcChanged(this, e2); + OnPropertyChanged(e); + property.NotifyChanged(e); + + if (PropertyChanged != null) + { + PropertyChanged(this, e); + } + + if (_inpcChanged != null) + { + PropertyChangedEventArgs e2 = new PropertyChangedEventArgs(property.Name); + _inpcChanged(this, e2); + } + } + finally + { + if (property.Notifying != null) + { + property.Notifying(this, false); + } } } diff --git a/src/Perspex.Base/PerspexProperty.cs b/src/Perspex.Base/PerspexProperty.cs index 3bda662038..e44284b570 100644 --- a/src/Perspex.Base/PerspexProperty.cs +++ b/src/Perspex.Base/PerspexProperty.cs @@ -63,6 +63,11 @@ namespace Perspex /// Whether the property inherits its value. /// The default binding mode for the property. /// A validation function. + /// + /// A method that gets called before and after the property starts being notified on an + /// object; the bool argument will be true before and false afterwards. This callback is + /// intended to support IsDataContextChanging. + /// /// Whether the property is an attached property. public PerspexProperty( string name, @@ -72,6 +77,7 @@ namespace Perspex bool inherits = false, BindingMode defaultBindingMode = BindingMode.Default, Func validate = null, + Action notifying = null, bool isAttached = false) { Contract.Requires(name != null); @@ -90,6 +96,7 @@ namespace Perspex Inherits = inherits; DefaultBindingMode = defaultBindingMode; IsAttached = isAttached; + Notifying = notifying; _id = s_nextId++; if (validate != null) @@ -240,6 +247,16 @@ namespace Perspex /// public IObservable Changed => _changed; + /// + /// The notifying callback. + /// + /// + /// This is a method that gets called before and after the property starts being notified + /// on an object; the bool argument will be true before and false afterwards. This + /// callback is intended to support IsDataContextChanging. + /// + public Action Notifying { get; } + /// /// Provides access to a property's binding via the /// indexer. @@ -323,13 +340,19 @@ namespace Perspex /// Whether the property inherits its value. /// The default binding mode for the property. /// A validation function. + /// + /// A method that gets called before and after the property starts being notified on an + /// object; the bool argument will be true before and false afterwards. This callback is + /// intended to support IsDataContextChanging. + /// /// A public static PerspexProperty Register( string name, TValue defaultValue = default(TValue), bool inherits = false, BindingMode defaultBindingMode = BindingMode.OneWay, - Func validate = null) + Func validate = null, + Action notifying = null) where TOwner : PerspexObject { Contract.Requires(name != null); @@ -341,6 +364,7 @@ namespace Perspex inherits, defaultBindingMode, Cast(validate), + notifying, false); PerspexObject.Register(typeof(TOwner), result); @@ -404,6 +428,7 @@ namespace Perspex inherits, defaultBindingMode, validate, + null, true); PerspexObject.Register(typeof(THost), result); @@ -440,6 +465,7 @@ namespace Perspex inherits, defaultBindingMode, validate, + null, true); PerspexObject.Register(typeof(THost), result); diff --git a/src/Perspex.Base/PerspexProperty`1.cs b/src/Perspex.Base/PerspexProperty`1.cs index 3c81b5e91b..73ac84e102 100644 --- a/src/Perspex.Base/PerspexProperty`1.cs +++ b/src/Perspex.Base/PerspexProperty`1.cs @@ -20,6 +20,11 @@ namespace Perspex /// Whether the property inherits its value. /// The default binding mode for the property. /// A validation function. + /// + /// A method that gets called before and after the property starts being notified on an + /// object; the bool argument will be true before and false afterwards. This callback is + /// intended to support IsDataContextChanging. + /// /// Whether the property is an attached property. public PerspexProperty( string name, @@ -28,6 +33,7 @@ namespace Perspex bool inherits = false, BindingMode defaultBindingMode = BindingMode.Default, Func validate = null, + Action notifying = null, bool isAttached = false) : base( name, @@ -37,6 +43,7 @@ namespace Perspex inherits, defaultBindingMode, Cast(validate), + notifying, isAttached) { } diff --git a/src/Perspex.Controls/Control.cs b/src/Perspex.Controls/Control.cs index b48b4afca6..e41be6ccc3 100644 --- a/src/Perspex.Controls/Control.cs +++ b/src/Perspex.Controls/Control.cs @@ -32,7 +32,10 @@ namespace Perspex.Controls /// Defines the property. /// public static readonly PerspexProperty DataContextProperty = - PerspexProperty.Register(nameof(DataContext), inherits: true); + PerspexProperty.Register( + nameof(DataContext), + inherits: true, + notifying: DataContextNotifying); /// /// Defines the property. @@ -262,6 +265,16 @@ namespace Perspex.Controls /// Type IStyleable.StyleKey => GetType(); + /// + /// Gets a value which indicates whether a change to the is in + /// the process of being notified. + /// + protected bool IsDataContextChanging + { + get; + private set; + } + /// /// Gets the control's logical children. /// @@ -396,6 +409,14 @@ namespace Perspex.Controls styler.ApplyStyles(this); } + /// + /// Called when the is changed and all subscribers to that change + /// have been notified. + /// + protected virtual void OnDataContextFinishedChanging() + { + } + /// /// Makes the control use a different control's logical children as its own. /// @@ -404,5 +425,20 @@ namespace Perspex.Controls { _logicalChildren = collection; } + + /// + /// Called when the property begins and ends being notified. + /// + /// The object on which the DataContext is changing. + /// Whether the notifcation is beginning or ending. + private static void DataContextNotifying(PerspexObject o, bool notifying) + { + var control = o as Control; + + if (control != null) + { + control.IsDataContextChanging = notifying; + } + } } } diff --git a/src/Perspex.Controls/Primitives/SelectingItemsControl.cs b/src/Perspex.Controls/Primitives/SelectingItemsControl.cs index 8671be4a47..86f1603a6e 100644 --- a/src/Perspex.Controls/Primitives/SelectingItemsControl.cs +++ b/src/Perspex.Controls/Primitives/SelectingItemsControl.cs @@ -82,6 +82,7 @@ namespace Perspex.Controls.Primitives private object _selectedItem; private IList _selectedItems; private bool _ignoreContainerSelectionChanged; + private IList _clearSelectedItemsAfterDataContextChanged; /// /// Initializes static members of the class. @@ -155,7 +156,21 @@ namespace Perspex.Controls.Primitives } else if (SelectedItems.Count > 0) { - SelectedItems.Clear(); + if (!IsDataContextChanging) + { + SelectedItems.Clear(); + } + else + { + // The DataContext is changing, and it's quite possible that our + // selection is being cleared because both Items and SelectedItems + // are bound to something on the DataContext. However, if we clear + // the collection now, we may be clearing a the SelectedItems from + // the DataContext which is being unbound, so do it after DataContext + // has notified all interested parties, in + // the OnDataContextFinishedChanging method. + _clearSelectedItemsAfterDataContextChanged = SelectedItems; + } } } } @@ -179,12 +194,9 @@ namespace Perspex.Controls.Primitives set { - if (value != null) - { - UnsubscribeFromSelectedItems(); - _selectedItems = value; - SubscribeToSelectedItems(); - } + UnsubscribeFromSelectedItems(); + _selectedItems = value ?? new PerspexList(); + SubscribeToSelectedItems(); } } @@ -271,6 +283,16 @@ namespace Perspex.Controls.Primitives } } + protected override void OnDataContextFinishedChanging() + { + if (_clearSelectedItemsAfterDataContextChanged == SelectedItems) + { + _clearSelectedItemsAfterDataContextChanged.Clear(); + } + + _clearSelectedItemsAfterDataContextChanged = null; + } + /// /// Updates the selection for an item based on user interaction. /// diff --git a/tests/Perspex.Controls.UnitTests/Perspex.Controls.UnitTests.csproj b/tests/Perspex.Controls.UnitTests/Perspex.Controls.UnitTests.csproj index ac644d8aba..63876a822a 100644 --- a/tests/Perspex.Controls.UnitTests/Perspex.Controls.UnitTests.csproj +++ b/tests/Perspex.Controls.UnitTests/Perspex.Controls.UnitTests.csproj @@ -120,6 +120,10 @@ + + {3e53a01a-b331-47f3-b828-4a5717e77a24} + Perspex.Markup.Xaml + {D211E587-D8BC-45B9-95A4-F297C8FA5200} Perspex.Animation diff --git a/tests/Perspex.Controls.UnitTests/Primitives/SelectingItemsControlTests_Multiple.cs b/tests/Perspex.Controls.UnitTests/Primitives/SelectingItemsControlTests_Multiple.cs index 8a2d0f72f0..85f275ad80 100644 --- a/tests/Perspex.Controls.UnitTests/Primitives/SelectingItemsControlTests_Multiple.cs +++ b/tests/Perspex.Controls.UnitTests/Primitives/SelectingItemsControlTests_Multiple.cs @@ -8,6 +8,7 @@ using Perspex.Collections; using Perspex.Controls.Presenters; using Perspex.Controls.Primitives; using Perspex.Controls.Templates; +using Perspex.Markup.Xaml.Binding; using Xunit; namespace Perspex.Controls.UnitTests.Primitives @@ -354,8 +355,74 @@ namespace Perspex.Controls.UnitTests.Primitives Assert.Equal(new[] { "baz", "qux", "qiz" }, target.SelectedItems.Cast().ToList()); } + /// + /// Tests a problem discovered with ListBox with selection. + /// + /// + /// - Items is bound to DataContext first, followed by say SelectedIndex + /// - When the ListBox is removed from the visual tree, DataContext becomes null (as it's + /// inherited) + /// - This changes Items to null, which changes SelectedIndex to null as there are no + /// longer any items + /// - However, the news that DataContext is now null hasn't yet reached the SelectedItems + /// binding and so the unselection is sent back to the ViewModel + /// + /// This is a similar problem to that tested by XamlBindingTest.Should_Not_Write_To_Old_DataContext. + /// However, that tests a general property binding problem: here we are writing directly + /// to the SelectedItems collection - not via a binding - so it's something that the + /// binding system cannot solve. Instead we solve it by not clearing SelectedItems when + /// DataContext is in the process of changing. + /// + [Fact] + public void Should_Not_Write_To_Old_DataContext() + { + var vm = new OldDataContextViewModel(); + var target = new TestSelector(); + + var itemsBinding = new XamlBinding + { + SourcePropertyPath = "Items", + BindingMode = BindingMode.OneWay, + }; + + var selectedItemsBinding = new XamlBinding + { + SourcePropertyPath = "SelectedItems", + BindingMode = BindingMode.OneWay, + }; + + // Bind Items and SelectedItems to the VM. + itemsBinding.Bind(target, TestSelector.ItemsProperty); + selectedItemsBinding.Bind(target, TestSelector.SelectedItemsProperty); + + // Set DataContext and SelectedIndex + target.DataContext = vm; + target.SelectedIndex = 1; + + // Make sure SelectedItems are written back to VM. + Assert.Equal(new[] { "bar" }, vm.SelectedItems); + + // Clear DataContext and ensure that SelectedItems is still set in the VM. + target.DataContext = null; + Assert.Equal(new[] { "bar" }, vm.SelectedItems); + } + + private ControlTemplate Template() + { + return new ControlTemplate(control => + new ItemsPresenter + { + Name = "itemsPresenter", + [~ItemsPresenter.ItemsProperty] = control[~ItemsControl.ItemsProperty], + [~ItemsPresenter.ItemsPanelProperty] = control[~ItemsControl.ItemsPanelProperty], + }); + } + private class TestSelector : SelectingItemsControl { + public static readonly new PerspexProperty SelectedItemsProperty = + SelectingItemsControl.SelectedItemsProperty; + public new IList SelectedItems { get { return base.SelectedItems; } @@ -374,15 +441,16 @@ namespace Perspex.Controls.UnitTests.Primitives } } - private ControlTemplate Template() + private class OldDataContextViewModel { - return new ControlTemplate(control => - new ItemsPresenter - { - Name = "itemsPresenter", - [~ItemsPresenter.ItemsProperty] = control[~ItemsControl.ItemsProperty], - [~ItemsPresenter.ItemsPanelProperty] = control[~ItemsControl.ItemsPanelProperty], - }); + public OldDataContextViewModel() + { + Items = new List { "foo", "bar" }; + SelectedItems = new List(); + } + + public List Items { get; } + public List SelectedItems { get; } } } }