Merge pull request #4138 from icsharpcode/fix/assembly-list-sort-move

Reorder the tree rows when the assembly list is sorted

ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs

@@ -201,6 +201,15 @@ namespace ICSharpCode.ILSpyX.TreeView
 
 		public virtual void OnChildrenChanged(NotifyCollectionChangedEventArgs e)
 		{
+			if (e.Action == NotifyCollectionChangedAction.Move)
+			{
+				// A move keeps the node and its parent, so none of the attach/detach work below
+				// applies: only the node's position in the flat list changes, and it takes its
+				// visible descendants with it as one run.
+				MoveChild((SharpTreeNode)e.OldItems![0]!, e.NewStartingIndex);
+				RaiseIsLastChangedIfNeeded(e);
+				return;
+			}
 			if (e.OldItems != null)
 			{
 				foreach (SharpTreeNode node in e.OldItems)
@@ -270,6 +279,31 @@ namespace ICSharpCode.ILSpyX.TreeView
 			RaisePropertyChanged(nameof(ShowExpander));
 			RaiseIsLastChangedIfNeeded(e);
 		}
+
+		void MoveChild(SharpTreeNode node, int newIndex)
+		{
+			Debug.Assert(node.modelParent == this);
+			if (!node.isVisible)
+			{
+				// Not part of the flat list, so the reorder of modelChildren is all there is to do.
+				return;
+			}
+			int oldVisibleIndex = GetVisibleIndexForNode(node);
+			List<SharpTreeNode> movedNodes = node.VisibleDescendantsAndSelf().ToList();
+			SharpTreeNode moveEnd = node;
+			while (moveEnd.modelChildren != null && moveEnd.modelChildren.Count > 0)
+				moveEnd = moveEnd.modelChildren.Last();
+			RemoveNodes(node, moveEnd);
+
+			// Same rule as insertion: the node goes after its predecessor's last descendant, or
+			// directly after this parent when it becomes the first child.
+			SharpTreeNode? insertionPos = newIndex == 0 ? null : modelChildren?[newIndex - 1];
+			while (insertionPos != null && insertionPos.modelChildren != null && insertionPos.modelChildren.Count > 0)
+				insertionPos = insertionPos.modelChildren.Last();
+			InsertNodeAfter(insertionPos ?? this, node);
+
+			GetListRoot().treeFlattener?.NodesMoved(oldVisibleIndex, GetVisibleIndexForNode(node), movedNodes);
+		}
 		#endregion
 
 		#region Expanding / LazyLoading

ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs

@@ -146,6 +146,28 @@ namespace ICSharpCode.ILSpyX.TreeView
 			OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, newNodes, index));
 		}
 
+		/// <summary>
+		/// Moves the node at <paramref name="oldIndex"/> to <paramref name="newIndex"/>, where
+		/// <paramref name="newIndex"/> is the position the node ends up at in the reordered
+		/// collection. Raised as a single <see cref="NotifyCollectionChangedAction.Move"/>, so the
+		/// node keeps its identity: a consumer that would throw away per-item state on a
+		/// remove/insert pair - selection, an expanded subtree, a container - keeps it.
+		/// </summary>
+		public void Move(int oldIndex, int newIndex)
+		{
+			ThrowOnReentrancy();
+			if ((uint)oldIndex >= (uint)list.Count)
+				throw new ArgumentOutOfRangeException(nameof(oldIndex));
+			if ((uint)newIndex >= (uint)list.Count)
+				throw new ArgumentOutOfRangeException(nameof(newIndex));
+			if (oldIndex == newIndex)
+				return;
+			var node = list[oldIndex];
+			list.RemoveAt(oldIndex);
+			list.Insert(newIndex, node);
+			OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Move, node, newIndex, oldIndex));
+		}
+
 		public void RemoveAt(int index)
 		{
 			ThrowOnReentrancy();

ICSharpCode.ILSpyX/TreeView/TreeFlattener.cs

@@ -76,6 +76,29 @@ namespace ICSharpCode.ILSpyX.TreeView
 				RaiseCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, list, index));
 		}
 
+		// The moved node's run keeps its identity here as well: one ranged Move instead of a
+		// Remove/Add pair, so a consumer that tracks items rather than indices (selection, realized
+		// containers) survives a reorder. Both indices are positions in the list as it stands before
+		// the move, which is what NotifyCollectionChangedEventArgs specifies for a move.
+		public void NodesMoved(int oldIndex, int newIndex, IEnumerable<SharpTreeNode> nodes)
+		{
+			if (!includeRoot)
+			{
+				oldIndex--;
+				newIndex--;
+			}
+			IList list = nodes as IList ?? new List<SharpTreeNode>(nodes);
+			if (list.Count == 0 || oldIndex == newIndex)
+				return;
+			// A forward move reports where the run ENDS up, not where it starts. The consumer
+			// (Avalonia's VirtualizingStackPanel) applies a ranged move by removing OldItems.Count
+			// rows at OldStartingIndex and re-inserting them at NewStartingIndex - (Count - 1), so a
+			// run reported by its final start index lands short by its own length. For a single row -
+			// a collapsed node, and every move the assembly list makes - the two readings coincide.
+			int reportedNewIndex = newIndex > oldIndex ? newIndex + list.Count - 1 : newIndex;
+			RaiseCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Move, list, reportedNewIndex, oldIndex));
+		}
+
 		public void Stop()
 		{
 			Debug.Assert(root.treeFlattener == this);

ILSpy.Tests/Controls/FlatListTreeNodeTests.cs

@@ -17,6 +17,9 @@
 // DEALINGS IN THE SOFTWARE.
 
 using System;
+using System.Collections.Generic;
+using System.Collections.Specialized;
+using System.Linq;
 
 using AwesomeAssertions;
 
@@ -69,4 +72,113 @@ public class FlatListTreeNodeTests
 		Assert.That(SharpTreeNode.GetNodeByVisibleIndex(listRoot, 0), Is.SameAs(root));
 		Assert.That(SharpTreeNode.GetNodeByVisibleIndex(listRoot, 1), Is.SameAs(child));
 	}
+
+	[Test]
+	public void Move_ReordersChildrenAndRaisesOneMoveEvent()
+	{
+		var root = new TestNode("root");
+		var a = new TestNode("a");
+		var b = new TestNode("b");
+		var c = new TestNode("c");
+		root.Children.AddRange(new[] { a, b, c });
+		var events = new List<NotifyCollectionChangedEventArgs>();
+		root.Children.CollectionChanged += (_, e) => events.Add(e);
+
+		root.Children.Move(2, 0);
+
+		root.Children.Should().Equal(c, a, b);
+		events.Should().ContainSingle();
+		events[0].Action.Should().Be(NotifyCollectionChangedAction.Move);
+		events[0].OldStartingIndex.Should().Be(2);
+		events[0].NewStartingIndex.Should().Be(0);
+		events[0].OldItems!.Cast<SharpTreeNode>().Should().Equal(c);
+		events[0].NewItems!.Cast<SharpTreeNode>().Should().Equal(c);
+	}
+
+	[Test]
+	public void Move_ToSameIndex_DoesNothing()
+	{
+		var root = new TestNode("root");
+		var a = new TestNode("a");
+		var b = new TestNode("b");
+		root.Children.AddRange(new[] { a, b });
+		var events = new List<NotifyCollectionChangedEventArgs>();
+		root.Children.CollectionChanged += (_, e) => events.Add(e);
+
+		root.Children.Move(1, 1);
+
+		root.Children.Should().Equal(a, b);
+		events.Should().BeEmpty();
+	}
+
+	[Test]
+	public void Move_UpdatesTheFlattenedOrder()
+	{
+		var root = new TestNode("root");
+		var a = new TestNode("a");
+		var b = new TestNode("b");
+		var c = new TestNode("c");
+		root.Children.AddRange(new[] { a, b, c });
+		root.IsExpanded = true;
+		var flattener = new TreeFlattener(root, includeRoot: true);
+
+		root.Children.Move(2, 0);
+
+		Flatten(flattener).Should().Equal(root, c, a, b);
+	}
+
+	[Test]
+	public void Move_OfAnExpandedNode_MovesItsWholeRun()
+	{
+		var root = new TestNode("root");
+		var a = new TestNode("a");
+		var a1 = new TestNode("a1");
+		var a2 = new TestNode("a2");
+		var b = new TestNode("b");
+		a.Children.AddRange(new[] { a1, a2 });
+		root.Children.AddRange(new[] { a, b });
+		root.IsExpanded = true;
+		a.IsExpanded = true;
+		var flattener = new TreeFlattener(root, includeRoot: true);
+		Flatten(flattener).Should().Equal(root, a, a1, a2, b);
+
+		root.Children.Move(0, 1);
+
+		Flatten(flattener).Should().Equal(root, b, a, a1, a2);
+	}
+
+	[Test]
+	public void Move_RaisesOneRangedMoveOnTheFlattener()
+	{
+		var root = new TestNode("root");
+		var a = new TestNode("a");
+		var a1 = new TestNode("a1");
+		var b = new TestNode("b");
+		a.Children.Add(a1);
+		root.Children.AddRange(new[] { a, b });
+		root.IsExpanded = true;
+		a.IsExpanded = true;
+		var flattener = new TreeFlattener(root, includeRoot: true);
+		var events = new List<NotifyCollectionChangedEventArgs>();
+		flattener.CollectionChanged += (_, e) => events.Add(e);
+
+		// root, a, a1, b  ->  root, b, a, a1: the run [a, a1] moves from index 1 to index 2.
+		root.Children.Move(0, 1);
+
+		events.Should().ContainSingle();
+		events[0].Action.Should().Be(NotifyCollectionChangedAction.Move);
+		events[0].OldItems!.Cast<SharpTreeNode>().Should().Equal(a, a1);
+		events[0].OldStartingIndex.Should().Be(1);
+		// A forward move of a multi-row run reports the row the run ends on, which is how the
+		// consumer re-inserts it; a1 is the last row of [a, a1] and ends up at index 3.
+		events[0].NewStartingIndex.Should().Be(3);
+	}
+
+	static List<object> Flatten(TreeFlattener flattener)
+	{
+		var result = new List<object>();
+		for (int i = 0; i < flattener.Count; i++)
+			result.Add(flattener[i]);
+		return result;
+	}
 }

ILSpy.Tests/Controls/SharpTreeViewTests.cs

@@ -16,6 +16,7 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
 // DEALINGS IN THE SOFTWARE.
 
+using System.Collections.Generic;
 using System.Linq;
 
 using Avalonia.Controls;
@@ -272,4 +273,31 @@ public class SharpTreeViewTests
 				Children.Add(new TestNode($"{text}_{i}"));
 		}
 	}
+
+	[AvaloniaTest]
+	public void Moving_An_Expanded_Node_Reorders_The_Rendered_Rows()
+	{
+		var (_, tree, root) = Host();
+		var b = (TestNode)root.Children[1];
+		b.IsExpanded = true;
+		Dispatcher.UIThread.RunJobs();
+		RenderedRows(tree).Should().Equal("A", "B", "B1", "C");
+
+		// B carries B1 with it: the run [B, B1] moves past C.
+		root.Children.Move(1, 2);
+		Dispatcher.UIThread.RunJobs();
+
+		RenderedRows(tree).Should().Equal("A", "C", "B", "B1");
+	}
+
+	static List<string> RenderedRows(SharpTreeView tree)
+	{
+		var rows = new List<string>();
+		for (int i = 0; i < tree.ItemCount; i++)
+		{
+			var container = tree.ContainerFromIndex(i);
+			rows.Add((container?.DataContext as SharpTreeNode)?.Text?.ToString() ?? "<unrealized>");
+		}
+		return rows;
+	}
 }

ILSpy/TreeNodes/AssemblyListTreeNode.cs

@@ -48,6 +48,12 @@ namespace ICSharpCode.ILSpy.TreeNodes
 					case NotifyCollectionChangedAction.Remove:
 						Children.RemoveRange(e.OldStartingIndex, e.OldItems!.Count);
 						break;
+					case NotifyCollectionChangedAction.Move:
+						// Sorting the list reorders it in place. Mirror that as a move rather than a
+						// remove/insert pair, so the node - and with it the selection, the expanded
+						// subtree below it and its row - survives the reorder.
+						Children.Move(e.OldStartingIndex, e.NewStartingIndex);
+						break;
 					case NotifyCollectionChangedAction.Reset:
 						Children.Clear();
 						Children.AddRange(assemblyList.GetAssemblies().Select(a => new AssemblyTreeNode(a)));