Merge pull request #4076 from icsharpcode/fix/sharptreeview-stranded-container

Scroll the tree by offset, so no container is left stranded

ILSpy/Controls/TreeView/SharpTreeView.cs

@@ -197,8 +197,8 @@ namespace ICSharpCode.ILSpy.Controls.TreeView
 			}
 			if (lastVisibleChild != node)
 			{
-				ScrollIntoView(lastVisibleChild);
-				Dispatcher.UIThread.Post(() => ScrollIntoView(node), DispatcherPriority.Loaded);
+				ScrollRowIntoView(lastVisibleChild, centre: false);
+				Dispatcher.UIThread.Post(() => ScrollRowIntoView(node, centre: false), DispatcherPriority.Loaded);
 			}
 		}
 
@@ -251,48 +251,100 @@ namespace ICSharpCode.ILSpy.Controls.TreeView
 
 		/// <summary>
 		/// Reveals <paramref name="node"/> centred in the viewport so the user's eye lands on a
-		/// newly selected row, rather than at the nearest edge (where <see cref="ListBox.ScrollIntoView"/>
-		/// leaves it). Skips the move when the row is already fully visible, so clicking a visible
-		/// row -- or selecting a freshly-loaded top-level entry that's already on screen -- never
-		/// yanks the viewport.
+		/// newly selected row, rather than at the nearest edge. Skips the move when the row is
+		/// already roughly centred, so re-selecting it never twitches the viewport. We
+		/// deliberately do NOT skip a merely-visible row sitting at an edge: the ListBox's
+		/// AutoScrollToSelectedItem drags the selected row to the nearest edge first, and a
+		/// reveal should still pull it to the centre from there. (Skipping an already-visible
+		/// row is decided one level up, before AutoScroll runs, in the model-&gt;tree sync --
+		/// see TreeSelectionBinder.SyncModelToTree.)
 		/// </summary>
-		void CenterNodeInView(SharpTreeNode node)
+		void CenterNodeInView(SharpTreeNode node) => ScrollRowIntoView(node, centre: true);
+
+		/// <summary>
+		/// Hides <see cref="ItemsControl.ScrollIntoView(object)"/> so the obvious way to reveal a
+		/// row is also the safe one -- see <see cref="ScrollRowIntoView"/> for what the base
+		/// method does to a container it realises. Hiding is not enforcement: a call through an
+		/// <see cref="ItemsControl"/>-typed reference still reaches the base method. It is here
+		/// so that code written later, on a SharpTreeView-typed variable, lands on the safe path
+		/// without its author having to know any of this.
+		/// </summary>
+		public new void ScrollIntoView(object item)
 		{
-			var scrollViewer = this.GetVisualDescendants().OfType<ScrollViewer>().FirstOrDefault();
-			if (scrollViewer is null)
-			{
-				ScrollIntoView(node);
+			if (item is SharpTreeNode node)
+				ScrollRowIntoView(node, centre: false);
+		}
+
+		/// <inheritdoc cref="ScrollIntoView(object)"/>
+		public new void ScrollIntoView(int index)
+		{
+			if (flattener is { } rows && (uint)index < (uint)rows.Count)
+				ScrollIntoView(rows[index]);
+		}
+
+		/// <summary>
+		/// Scrolls to a row by setting the offset the row's index implies, rather than calling
+		/// <see cref="ListBox.ScrollIntoView(object)"/>.
+		///
+		/// That method realises a container for the target, arranges it at its own desired
+		/// width, parks it aside for the duration of a few layout passes and then drops the
+		/// reference. A container the passes do not adopt back into the realized range is left
+		/// a visible child of the panel that nothing arranges again -- it keeps painting its
+		/// old item at its old position, over whatever row now occupies that spot. Rows here
+		/// are a uniform height, so the arithmetic below reaches the same place without ever
+		/// entering that code path.
+		/// </summary>
+		void ScrollRowIntoView(SharpTreeNode node, bool centre)
+		{
+			if (this.GetVisualDescendants().OfType<ScrollViewer>().FirstOrDefault() is not { } scrollViewer)
 				return;
-			}
+			if (flattener is null || flattener.IndexOf(node) is not (>= 0 and var index))
+				return;
+
+			// An expansion has just changed how many rows there are; the extent has to catch up
+			// before an offset can be clamped against it.
+			UpdateLayout();
 
-			// If the row is already realised and roughly centred, leave the viewport alone -- this
-			// keeps a re-selection of an already-centred row from twitching. We deliberately do NOT
-			// skip a merely-visible row sitting at an edge: the ListBox's AutoScrollToSelectedItem
-			// drags the selected row to the nearest edge first, and a reveal should still pull it to
-			// the centre from there. (Skipping an already-visible row is decided one level up, before
-			// AutoScroll runs, in the model->tree sync -- see TreeSelectionBinder.SyncModelToTree.)
-			if (ContainerFromItem(node) is Control visible && visible.IsVisible
-				&& visible.TranslatePoint(new Point(0, 0), scrollViewer) is { } top)
+			double rowHeight = RowHeight();
+			double viewport = scrollViewer.Viewport.Height;
+			double rowTop = index * rowHeight;
+			double offset = scrollViewer.Offset.Y;
+
+			if (centre)
 			{
-				var rowMid = top.Y + visible.Bounds.Height / 2;
-				var viewportMid = scrollViewer.Viewport.Height / 2;
-				if (Math.Abs(rowMid - viewportMid) <= visible.Bounds.Height)
+				double rowMidInViewport = rowTop - offset + rowHeight / 2;
+				if (Math.Abs(rowMidInViewport - viewport / 2) <= rowHeight)
 					return;
+				offset = rowTop - (viewport - rowHeight) / 2;
 			}
-
-			// Bring it on screen (edge), force layout so the container realises, then offset so the
-			// row sits at the vertical centre.
-			ScrollIntoView(node);
-			UpdateLayout();
-			if (ContainerFromItem(node) is not Control row)
-				return;
-			if (row.TranslatePoint(new Point(0, 0), scrollViewer) is not { } rowTop)
+			else if (rowTop < offset)
+			{
+				offset = rowTop;
+			}
+			else if (rowTop + rowHeight > offset + viewport)
+			{
+				offset = rowTop + rowHeight - viewport;
+			}
+			else
+			{
 				return;
-			var desiredTop = (scrollViewer.Viewport.Height - row.Bounds.Height) / 2;
-			var newOffsetY = scrollViewer.Offset.Y + (rowTop.Y - desiredTop);
-			var maxOffset = Math.Max(0, scrollViewer.Extent.Height - scrollViewer.Viewport.Height);
-			newOffsetY = Math.Clamp(newOffsetY, 0, maxOffset);
-			scrollViewer.Offset = new Vector(scrollViewer.Offset.X, newOffsetY);
+			}
+
+			double maxOffset = Math.Max(0, scrollViewer.Extent.Height - viewport);
+			scrollViewer.Offset = new Vector(scrollViewer.Offset.X, Math.Clamp(offset, 0, maxOffset));
+		}
+
+		/// <summary>The height of a row, taken from one that exists rather than assumed. The
+		/// fallback matches the item template, for the moment before the first row is
+		/// realised.</summary>
+		double RowHeight()
+		{
+			foreach (var container in GetRealizedContainers())
+			{
+				if (container.Bounds.Height > 0)
+					return container.Bounds.Height;
+			}
+			return 20;
 		}
 
 		/// <summary>