LT-22676: Stop browse views dragging the selected row back on scroll

Src/Common/Controls/XMLViews/BrowseViewer.cs

@@ -613,6 +613,15 @@ public int SelectedIndex
 			}
 		}
 
+		/// <summary>
+		/// Scroll the selected row into view without changing which row is selected.
+		/// </summary>
+		public void ScrollSelectedRowIntoView()
+		{
+			CheckDisposed();
+			m_xbv.ScrollSelectedRowIntoView();
+		}
+
 		/// <summary>
 		/// Gets the column count. This count does not include the check box column.
 		/// </summary>

Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseTests.cs

@@ -9,6 +9,7 @@
 using SIL.FieldWorks.Common.ViewsInterfaces;
 using SIL.FieldWorks.Common.Controls;
 using SIL.LCModel;
+using SIL.LCModel.Application;
 using SIL.LCModel.Core.KernelInterfaces;
 using SIL.LCModel.Utils;
 
@@ -69,6 +70,62 @@ public Size GetScrollRange()
 			return ScrollRange;
 		}
 
+		/// <summary>How many times the view asked to make a selection visible.</summary>
+		public int MakeSelectionVisibleCalls;
+
+		/// <summary>Unit test helper: publish the rows the view reads.</summary>
+		public void SetList(ISilDataAccessManaged sda, int hvoRoot, int flid)
+		{
+			m_sda = sda;
+			m_hvoRoot = hvoRoot;
+			m_fakeFlid = flid;
+		}
+
+		/// <summary>Unit test helper: make a row current, bypassing the setter.</summary>
+		public void SetSelectedRow(int index, int hvo)
+		{
+			m_selectedIndex = index;
+			m_hvoOldSel = hvo;
+		}
+
+		/// <summary>Unit test helper</summary>
+		public void CallSaveSelectionInfo(Rectangle rcIdeal, int ydTop)
+		{
+			SaveSelectionInfo(rcIdeal, ydTop);
+		}
+
+		/// <summary>Unit test helper</summary>
+		public bool CallAdjustScrollRange(int dxdSize, int dxdPosition, int dydSize, int dydPosition)
+		{
+			return AdjustScrollRange1(dxdSize, dxdPosition, dydSize, dydPosition);
+		}
+
+		/// <summary>Unit test helper</summary>
+		public bool CallMakeSelectionVisible(IVwSelection sel)
+		{
+			return MakeSelectionVisible(sel, true, true, true);
+		}
+
+		/// <summary>
+		/// The fake root box cannot supply a selection, so only record the request.
+		/// </summary>
+		protected override bool MakeSelectionVisible(IVwSelection sel, bool fWantOneLineSpace)
+		{
+			MakeSelectionVisibleCalls++;
+			return true;
+		}
+
+		/// <summary>
+		/// Record the request, then let the product code run (and clear its anchor) on the null
+		/// selection the fake root box yields.
+		/// </summary>
+		protected override bool MakeSelectionVisible(IVwSelection vwsel, bool fWantOneLineSpace,
+			bool fWantBothEnds, bool fForcePrepareToDraw)
+		{
+			MakeSelectionVisibleCalls++;
+			return base.MakeSelectionVisible(vwsel, fWantOneLineSpace, fWantBothEnds, fForcePrepareToDraw);
+		}
+
 		/// <summary/>
 		public class FakeRootBox : IVwRootBox
 		{
@@ -935,5 +992,105 @@ public void ScrollMinSize_SettingSetsScrollBarMaximumToSame()
 			m_view.ScrollMinSize = new Size(123, height);
 			Assert.That(m_view.m_bv.ScrollBar.Maximum, Is.EqualTo(height));
 		}
+
+		private const int kRowListFlid = ObjectListPublisher.MinFakeFlid;
+		private const int kHvoRowList = 1;
+		/// <summary>Where the third row sits in a fully expanded 25-pixel-row view.</summary>
+		private static readonly Rectangle s_thirdRowRect = new Rectangle(0, 56, 100, 26);
+
+		private ObjectListPublisher PublishRows(params int[] hvos)
+		{
+			var publisher = new ObjectListPublisher(Cache.MainCacheAccessor as ISilDataAccessManaged, kRowListFlid);
+			publisher.CacheVecProp(kHvoRowList, hvos, false);
+			m_view.SetList(publisher, kHvoRowList, kRowListFlid);
+			return publisher;
+		}
+
+		private void ScrollTo(int y)
+		{
+			m_view.ScrollPosition = new Point(0, y);
+			Assert.That(-m_view.ScrollPosition.Y, Is.EqualTo(y), "Unit test bad assumption");
+		}
+
+		/// <summary>
+		/// LT-22676: after MakeSelectionVisible returns, a lazy-box expansion caused by user
+		/// scrolling must not use its stale row rectangle to drag the selected row back.
+		/// </summary>
+		[Test]
+		public void AdjustScrollRange_AfterMakeSelectionVisible_DoesNotDragSelectedRowBack()
+		{
+			m_view.m_rowCount = 100;
+			ConfigureScrollBars();
+			m_view.SetSelectedRow(2, 103);
+			m_view.CallSaveSelectionInfo(s_thirdRowRect, 0);
+			m_view.CallMakeSelectionVisible(null);
+			ScrollTo(600);
+
+			m_view.CallAdjustScrollRange(0, 0, 48, 1143);
+
+			Assert.That(-m_view.ScrollPosition.Y, Is.EqualTo(600));
+		}
+
+		/// <summary>
+		/// LT-3607: while MakeSelectionVisible is expanding lazy boxes, an expansion must
+		/// keep the row it is scrolling to in view.
+		/// </summary>
+		[Test]
+		public void AdjustScrollRange_WhileMakingSelectionVisible_KeepsSelectedRowInView()
+		{
+			m_view.m_rowCount = 100;
+			ConfigureScrollBars();
+			m_view.SetSelectedRow(2, 103);
+			ScrollTo(600);
+			m_view.CallSaveSelectionInfo(s_thirdRowRect, 600);
+
+			m_view.CallAdjustScrollRange(0, 0, 48, 1143);
+
+			Assert.That(-m_view.ScrollPosition.Y, Is.EqualTo(s_thirdRowRect.Top));
+		}
+
+		/// <summary/>
+		[Test]
+		public void SelectedIndex_ReassertSameRowSameObject_DoesNotScroll()
+		{
+			m_view.m_rowCount = 100;
+			ConfigureScrollBars();
+			PublishRows(101, 102, 103, 104, 105);
+			m_view.SetSelectedRow(2, 103);
+			ScrollTo(600);
+
+			m_view.SelectedIndex = 2;
+
+			Assert.That(m_view.MakeSelectionVisibleCalls, Is.EqualTo(0));
+			Assert.That(-m_view.ScrollPosition.Y, Is.EqualTo(600));
+		}
+
+		/// <summary/>
+		[Test]
+		public void SelectedIndex_ReassertSameRowDifferentObject_ScrollsRowIntoView()
+		{
+			m_view.m_rowCount = 100;
+			ConfigureScrollBars();
+			var rows = PublishRows(101, 102, 103, 104, 105);
+			m_view.SetSelectedRow(2, 103);
+			// The list was re-sorted under the fixed index, so row 2 now shows another object.
+			rows.CacheVecProp(kHvoRowList, new[] { 101, 102, 999, 104, 105 }, false);
+
+			m_view.SelectedIndex = 2;
+
+			Assert.That(m_view.MakeSelectionVisibleCalls, Is.EqualTo(1));
+		}
+
+		/// <summary/>
+		[Test]
+		public void ScrollSelectedRowIntoView_RequestsTheSelectedRow()
+		{
+			PublishRows(101, 102, 103);
+			m_view.SetSelectedRow(2, 103);
+
+			m_view.ScrollSelectedRowIntoView();
+
+			Assert.That(m_view.MakeSelectionVisibleCalls, Is.EqualTo(1));
+		}
 	}
-}
+}

Src/Common/Controls/XMLViews/XmlBrowseViewBase.cs

@@ -277,9 +277,9 @@ public int SelectedIndex
 					throw new ArgumentOutOfRangeException("XmlBrowseViewBase.SelectedIndex", value.ToString(), "Index cannot be set to less than -1.");
 				if (m_selectedIndex == value)
 				{
-					// It's useful to check this anyway, since the width of the window or something else
-					// that affects visibility may have changed...but don't CHANGE the selection, the user may be editing...(LT-12092)
-					if (value >= 0 && m_wantScrollIntoView)
+					// Never touch the editing selection here (LT-12092). Scroll only when the
+					// row now shows a different object, e.g. after a re-sort (LT-22676).
+					if (value >= 0 && m_wantScrollIntoView && GetNewSelectionObject(value) != m_hvoOldSel)
 						MakeSelectionVisible(GetRowSelection(value));
 					return;
 				}
@@ -491,6 +491,36 @@ internal bool ShouldConvertDummiesInView()
 		protected int m_ydSelScrollPos = 0;
 		/// <summary></summary>
 		protected int m_iSelIndex = 0;
+
+		/// <summary>
+		/// Forget the row rectangle SaveSelectionInfo stored for AdjustScrollRange1.
+		/// </summary>
+		private void ClearSelectionAnchor()
+		{
+			m_iSelIndex = 0;
+			m_ydSelBottom = 0;
+			m_ydSelScrollPos = 0;
+			m_ydSelTop = 0;
+		}
+
+		/// <summary>
+		/// Make the selection visible, then forget the row rectangle so it only steers
+		/// AdjustScrollRange1 during this call's own lazy-box expansion. Left in place, an
+		/// expansion caused by user scrolling would drag the row back on screen (LT-22676).
+		/// </summary>
+		protected override bool MakeSelectionVisible(IVwSelection vwsel, bool fWantOneLineSpace,
+			bool fWantBothEnds, bool fForcePrepareToDraw)
+		{
+			try
+			{
+				return base.MakeSelectionVisible(vwsel, fWantOneLineSpace, fWantBothEnds, fForcePrepareToDraw);
+			}
+			finally
+			{
+				ClearSelectionAnchor();
+			}
+		}
+
 		/// <summary>
 		/// Handle the special aspects of adjusting the scroll position for a table of cells
 		/// like we have in the browse view.  See LT-3607 for details of what can go wrong
@@ -1683,10 +1713,7 @@ protected IVwSelection GetRowSelection(int index)
 		/// ------------------------------------------------------------------------------------
 		protected virtual void DoSelectAndScroll(int hvo, int index)
 		{
-			m_iSelIndex = 0;
-			m_ydSelBottom = 0;
-			m_ydSelScrollPos = 0;
-			m_ydSelTop = 0;
+			ClearSelectionAnchor();
 			if (m_rootb == null)
 				return;
 			IVwSelection selRow = GetRowSelection(index);
@@ -1966,9 +1993,16 @@ protected override void OnPaint(PaintEventArgs e)
 		protected override void HandleSelectionChange(IVwRootBox prootb, IVwSelection vwselNew)
 		{
 			base.HandleSelectionChange(prootb, vwselNew);
-			m_mediator.IdleQueue.Add(IdleQueuePriority.Medium, RemoveRootBoxSelectionOnIdle);
+			if (!m_fReplacingIdleSelection)
+				m_mediator.IdleQueue.Add(IdleQueuePriority.Medium, RemoveRootBoxSelectionOnIdle);
 		}
 
+		/// <summary>
+		/// True while RemoveRootBoxSelectionOnIdle installs its replacement insertion point, so
+		/// the selection change that raises does not queue another cleanup pass.
+		/// </summary>
+		private bool m_fReplacingIdleSelection;
+
 		bool RemoveRootBoxSelectionOnIdle(object parameter)
 		{
 			if (IsDisposed || m_rootb == null)
@@ -1992,7 +2026,19 @@ bool RemoveRootBoxSelectionOnIdle(object parameter)
 				{
 					m_rootb.DestroySelection();
 					if (idxFromSel == m_selectedIndex)
-						SetDefaultInsertionPointInRow(idxFromSel);
+					{
+						// A non-editable replacement must not queue another pass, or it gets
+						// destroyed and re-created on every idle tick (LT-22676).
+						m_fReplacingIdleSelection = true;
+						try
+						{
+							SetDefaultInsertionPointInRow(idxFromSel);
+						}
+						finally
+						{
+							m_fReplacingIdleSelection = false;
+						}
+					}
 				}
 			}
 			return true;
@@ -2205,6 +2251,15 @@ private bool UpdateSelectedRow(object args)
 		/// </summary>
 		public void PostLayoutInit()
 		{
+			ScrollSelectedRowIntoView();
+		}
+
+		/// <summary>
+		/// Scroll the selected row into view without changing which row is selected.
+		/// </summary>
+		internal void ScrollSelectedRowIntoView()
+		{
+			CheckDisposed();
 			if (m_rootb == null || SelectedIndex < 0)
 				return;
 			MakeSelectionVisible(GetRowSelection(SelectedIndex), true, true, true);

Src/xWorks/RecordBrowseView.cs

@@ -580,6 +580,10 @@ public override bool OnRecordNavigation(object argument)
 				if (clerk != null && sendingClerk == clerk && clerk.IsActiveInGui)
 				{
 					m_browseViewer.SelectedIndex = clerk.CurrentIndex;
+					// The index did not change, so the setter left the scroll position alone; the
+					// user asked for this record, so bring it back on screen (LT-22676).
+					if (rni.JumpedToCurrentRecord)
+						m_browseViewer.ScrollSelectedRowIntoView();
 					// go ahead and SetInfoBarText even if we didn't change indices
 					// we may have changed objects or root object classes (from Entries to Senses)
 					SetInfoBarText();