Skip to content

Commit

Permalink
Unset Parent on Conductors' Items.Clear()
Browse files Browse the repository at this point in the history
ObservableCollection<T> is designed so
NotifyCollectionChangedEventArgs.OldItems is not populated when .Clear()
is called on the collection. Because of this, Caliburn conductors can't
unset the Parent property on all conducted IChild.

There are multiple solutions to this issue that have been discussed, but
I feel like adding a new CollectionCleared event to BindableCollection
is the best compromise considering it doesn't break any existing code
and only extends existing ObservableCollection functionality.
  • Loading branch information
adamgauthier committed Nov 14, 2019
1 parent 8eacda6 commit d3e33ed
Show file tree
Hide file tree
Showing 7 changed files with 76 additions and 7 deletions.
21 changes: 21 additions & 0 deletions src/Caliburn.Micro.Core.Tests/BindableCollectionTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
using Xunit;

namespace Caliburn.Micro.Core.Tests
{
public class BindableCollectionTests
{
[Fact]
public void Clear_ThenCollectionClearedIsRaisedWithAllItems()
{
var objects = new[] { new object(), new object(), new object() };
var bindableCollection = new BindableCollection<object>(objects);

var collectionClearedEvent = Assert.Raises<CollectionClearedEventArgs<object>>(
h => bindableCollection.CollectionCleared += h, h => bindableCollection.CollectionCleared -= h,
() => bindableCollection.Clear()
);

Assert.Equal(objects, collectionClearedEvent.Arguments.ClearedItems);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
using Xunit;

namespace Caliburn.Micro.Core.Tests
{
public class ConductorCollectionAllActiveTests
{
[Fact]
public void ParentItemIsUnsetOnClear()
{
var conductor = new Conductor<IScreen>.Collection.AllActive();
var conducted = new[] { new Screen(), new Screen() };
conductor.Items.AddRange(conducted);

conductor.Items.Clear();

Assert.All(conducted, screen => Assert.NotEqual(conductor, screen.Parent));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public async Task CanCloseIsTrueWhenItemsAreClosable()
conductor.Items.Add(conducted);

await ((IActivate)conductor).ActivateAsync(CancellationToken.None);
var canClose =await conductor.CanCloseAsync(CancellationToken.None);
var canClose = await conductor.CanCloseAsync(CancellationToken.None);

Assert.True(canClose);
Assert.False(conducted.IsClosed);
Expand All @@ -62,7 +62,7 @@ public async Task CanCloseIsTrueWhenItemsAreNotClosableAndCloseStrategyCloses()
IsClosable = true
};
conductor.Items.Add(conducted);
await((IActivate)conductor).ActivateAsync(CancellationToken.None);
await ((IActivate)conductor).ActivateAsync(CancellationToken.None);
var canClose = await conductor.CanCloseAsync(CancellationToken.None);

Assert.True(canClose);
Expand Down Expand Up @@ -119,7 +119,7 @@ public void ParentItemIsSetOnReplacedConductedItem()
Assert.Equal(conductor, newConducted.Parent);
}

[Fact(Skip = "This is not possible as we don't get the removed items in the event handler.")]
[Fact]
public void ParentItemIsUnsetOnClear()
{
var conductor = new Conductor<IScreen>.Collection.OneActive();
Expand Down
7 changes: 7 additions & 0 deletions src/Caliburn.Micro.Core/BindableCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ public BindableCollection(IEnumerable<T> collection)
/// </summary>
public bool IsNotifying { get; set; }

/// <summary>
/// Occurs when the collection has been cleared.
/// </summary>
public event EventHandler<CollectionClearedEventArgs<T>> CollectionCleared;

/// <summary>
/// Notifies subscribers of the property change.
/// </summary>
Expand Down Expand Up @@ -180,7 +185,9 @@ protected override sealed void ClearItems()
/// </remarks>
protected virtual void ClearItemsBase()
{
var clearedItems = new List<T>(collection: this);
base.ClearItems();
CollectionCleared?.Invoke(this, new CollectionClearedEventArgs<T>(clearedItems));
}

/// <summary>
Expand Down
15 changes: 15 additions & 0 deletions src/Caliburn.Micro.Core/CollectionClearedEventArgs.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
using System;
using System.Collections.Generic;

namespace Caliburn.Micro
{
public class CollectionClearedEventArgs<T> : EventArgs
{
public CollectionClearedEventArgs(IReadOnlyCollection<T> clearedItems)
{
ClearedItems = clearedItems;
}

public IReadOnlyCollection<T> ClearedItems { get; }
}
}
9 changes: 6 additions & 3 deletions src/Caliburn.Micro.Core/ConductorWithCollectionAllActive.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System;
using System.Collections.Generic;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.Linq;
using System.Reflection;
Expand Down Expand Up @@ -38,6 +37,10 @@ public AllActive(bool openPublicItems)
/// </summary>
public AllActive()
{
_items.CollectionCleared += (s, e) =>
{
e.ClearedItems.OfType<IChild>().Apply(x => x.Parent = null);
};
_items.CollectionChanged += (s, e) =>
{
switch (e.Action)
Expand Down Expand Up @@ -80,7 +83,7 @@ protected override Task OnActivateAsync(CancellationToken cancellationToken)
/// <returns>A task that represents the asynchronous operation.</returns>
protected override async Task OnDeactivateAsync(bool close, CancellationToken cancellationToken)
{
foreach(var deactivate in _items.OfType<IDeactivate>())
foreach (var deactivate in _items.OfType<IDeactivate>())
{
await deactivate.DeactivateAsync(close, cancellationToken);
}
Expand Down
6 changes: 5 additions & 1 deletion src/Caliburn.Micro.Core/ConductorWithCollectionOneActive.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ public class OneActive : ConductorBaseWithActiveItem<T>
/// </summary>
public OneActive()
{
_items.CollectionCleared += (s, e) =>
{
e.ClearedItems.OfType<IChild>().Apply(x => x.Parent = null);
};
_items.CollectionChanged += (s, e) =>
{
switch (e.Action)
Expand Down Expand Up @@ -172,7 +176,7 @@ public override async Task<bool> CanCloseAsync(CancellationToken cancellationTok
closable = stillToClose;
}

foreach(var deactivate in closable.OfType<IDeactivate>())
foreach (var deactivate in closable.OfType<IDeactivate>())
{
await deactivate.DeactivateAsync(true, cancellationToken);
}
Expand Down

0 comments on commit d3e33ed

Please sign in to comment.