From be47e35eb015b7b85c32e5ce9af47b4efb485170 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Wed, 23 Sep 2026 10:36:46 -0700 Subject: [PATCH 1/2] LT-22676: Stop browse views dragging the selected row back on scroll Clear the row rectangle XmlBrowseViewBase saves for AdjustScrollRange1 as soon as MakeSelectionVisible returns, so a lazy-box expansion caused by the user scrolling no longer forces the selected row back on screen. Guard the idle selection cleanup so installing its replacement insertion point does not queue another cleanup pass. Re-asserting the same index now scrolls only when the row shows a different object, and PostLayoutInit goes through ScrollSelectedRowIntoView. Co-Authored-By: Claude Fable 5.1 --- .../XMLViewsTests/XmlBrowseViewBaseTests.cs | 159 +++++++++++++++++- .../Controls/XMLViews/XmlBrowseViewBase.cs | 73 +++++++- 2 files changed, 222 insertions(+), 10 deletions(-) diff --git a/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseTests.cs b/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseTests.cs index 46babcaadd..a303d7cbb5 100644 --- a/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseTests.cs +++ b/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; } + /// How many times the view asked to make a selection visible. + public int MakeSelectionVisibleCalls; + + /// Unit test helper: publish the rows the view reads. + public void SetList(ISilDataAccessManaged sda, int hvoRoot, int flid) + { + m_sda = sda; + m_hvoRoot = hvoRoot; + m_fakeFlid = flid; + } + + /// Unit test helper: make a row current, bypassing the setter. + public void SetSelectedRow(int index, int hvo) + { + m_selectedIndex = index; + m_hvoOldSel = hvo; + } + + /// Unit test helper + public void CallSaveSelectionInfo(Rectangle rcIdeal, int ydTop) + { + SaveSelectionInfo(rcIdeal, ydTop); + } + + /// Unit test helper + public bool CallAdjustScrollRange(int dxdSize, int dxdPosition, int dydSize, int dydPosition) + { + return AdjustScrollRange1(dxdSize, dxdPosition, dydSize, dydPosition); + } + + /// Unit test helper + public bool CallMakeSelectionVisible(IVwSelection sel) + { + return MakeSelectionVisible(sel, true, true, true); + } + + /// + /// The fake root box cannot supply a selection, so only record the request. + /// + protected override bool MakeSelectionVisible(IVwSelection sel, bool fWantOneLineSpace) + { + MakeSelectionVisibleCalls++; + return true; + } + + /// + /// Record the request, then let the product code run (and clear its anchor) on the null + /// selection the fake root box yields. + /// + protected override bool MakeSelectionVisible(IVwSelection vwsel, bool fWantOneLineSpace, + bool fWantBothEnds, bool fForcePrepareToDraw) + { + MakeSelectionVisibleCalls++; + return base.MakeSelectionVisible(vwsel, fWantOneLineSpace, fWantBothEnds, fForcePrepareToDraw); + } + /// 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; + /// Where the third row sits in a fully expanded 25-pixel-row view. + 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"); + } + + /// + /// 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. + /// + [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)); + } + + /// + /// LT-3607: while MakeSelectionVisible is expanding lazy boxes, an expansion must + /// keep the row it is scrolling to in view. + /// + [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)); + } + + /// + [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)); + } + + /// + [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)); + } + + /// + [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)); + } } -} \ No newline at end of file +} diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBase.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBase.cs index ec4dc567ce..0858bc7562 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBase.cs +++ b/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; /// protected int m_iSelIndex = 0; + + /// + /// Forget the row rectangle SaveSelectionInfo stored for AdjustScrollRange1. + /// + private void ClearSelectionAnchor() + { + m_iSelIndex = 0; + m_ydSelBottom = 0; + m_ydSelScrollPos = 0; + m_ydSelTop = 0; + } + + /// + /// 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). + /// + protected override bool MakeSelectionVisible(IVwSelection vwsel, bool fWantOneLineSpace, + bool fWantBothEnds, bool fForcePrepareToDraw) + { + try + { + return base.MakeSelectionVisible(vwsel, fWantOneLineSpace, fWantBothEnds, fForcePrepareToDraw); + } + finally + { + ClearSelectionAnchor(); + } + } + /// /// 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); } + /// + /// True while RemoveRootBoxSelectionOnIdle installs its replacement insertion point, so + /// the selection change that raises does not queue another cleanup pass. + /// + 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) /// public void PostLayoutInit() { + ScrollSelectedRowIntoView(); + } + + /// + /// Scroll the selected row into view without changing which row is selected. + /// + internal void ScrollSelectedRowIntoView() + { + CheckDisposed(); if (m_rootb == null || SelectedIndex < 0) return; MakeSelectionVisible(GetRowSelection(SelectedIndex), true, true, true); From 70f69daa47773a18dde46bffe90e99d197e1ca6d Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Thu, 24 Sep 2026 14:08:15 -0700 Subject: [PATCH 2/2] LT-22676: Scroll the current row into view on an explicit jump to it JumpToRecord now tells the RecordNavigation payload when it asked for the record that was already current, and RecordBrowseView scrolls the selected row into view for that case. JumpToIndex on the current index still leaves the scroll position alone, so refreshes do not drag the row back. Co-Authored-By: Claude Fable 5.1 --- Src/Common/Controls/XMLViews/BrowseViewer.cs | 9 ++++ Src/xWorks/RecordBrowseView.cs | 4 ++ Src/xWorks/RecordClerk.cs | 38 ++++++++++++-- .../RecordClerkNavigationContextTests.cs | 50 +++++++++++++++++++ 4 files changed, 98 insertions(+), 3 deletions(-) diff --git a/Src/Common/Controls/XMLViews/BrowseViewer.cs b/Src/Common/Controls/XMLViews/BrowseViewer.cs index ee0171e387..2654429df1 100644 --- a/Src/Common/Controls/XMLViews/BrowseViewer.cs +++ b/Src/Common/Controls/XMLViews/BrowseViewer.cs @@ -613,6 +613,15 @@ public int SelectedIndex } } + /// + /// Scroll the selected row into view without changing which row is selected. + /// + public void ScrollSelectedRowIntoView() + { + CheckDisposed(); + m_xbv.ScrollSelectedRowIntoView(); + } + /// /// Gets the column count. This count does not include the check box column. /// diff --git a/Src/xWorks/RecordBrowseView.cs b/Src/xWorks/RecordBrowseView.cs index c9cf56c52e..7ea5271d10 100644 --- a/Src/xWorks/RecordBrowseView.cs +++ b/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(); diff --git a/Src/xWorks/RecordClerk.cs b/Src/xWorks/RecordClerk.cs index 90815e4402..c6fdb99546 100644 --- a/Src/xWorks/RecordClerk.cs +++ b/Src/xWorks/RecordClerk.cs @@ -1130,7 +1130,7 @@ bool JumpToRecord(object argument) return false; } } - JumpToIndex(index); + JumpToIndex(index, false, true); return true; //we handled this. } finally @@ -2347,7 +2347,7 @@ public void JumpToRecord(int jumpToHvo, bool suppressFocusChange) var index = m_list.IndexOf(jumpToHvo); if (index < 0) return; // not found (maybe suppressed by filter?) - JumpToIndex(index, suppressFocusChange); + JumpToIndex(index, suppressFocusChange, true); } public void JumpToIndex(int index) @@ -2361,6 +2361,16 @@ public void JumpToIndex(int index) /// The index. /// if set to true focus changes will be suppressed. public void JumpToIndex(int index, bool suppressFocusChange) + { + JumpToIndex(index, suppressFocusChange, false); + } + + /// + /// Jump to the specified index. is true when a + /// JumpToRecord asked for this record by object, so a view can tell that request apart + /// from a refresh that re-asserts the current index. + /// + private void JumpToIndex(int index, bool suppressFocusChange, bool jumpedToRecord) { CheckDisposed(); //if we aren't changing the index, just bail out. (Fixes, LT-11401) @@ -2374,7 +2384,8 @@ public void JumpToIndex(int index, bool suppressFocusChange) //RecordBrowseView line 483 and elsewhere that we rely on the re-broadcasting. //in order to maintain the LT-11401 fix we directly use the mediator here and pass true in the //second parameter so that we don't save the record and lose the undo history. -naylor 2011-11-03 - var rni = new RecordNavigationInfo(this, true, SkipShowRecord, suppressFocusChange); + var rni = new RecordNavigationInfo(this, true, SkipShowRecord, suppressFocusChange, + jumpedToRecord); #pragma warning disable 618 // suppress obsolete warning m_mediator.BroadcastMessage("RecordNavigation", rni); #pragma warning restore 618 @@ -3667,12 +3678,33 @@ public class RecordNavigationInfo : IComparable /// /// public RecordNavigationInfo(RecordClerk clerk, bool suppressSaveOnChangeRecord, bool skipShowRecord, bool suppressFocusChange) + : this(clerk, suppressSaveOnChangeRecord, skipShowRecord, suppressFocusChange, false) + { + } + + /// + /// Make one, recording whether a JumpToRecord asked for the record that was already + /// current. + /// + public RecordNavigationInfo(RecordClerk clerk, bool suppressSaveOnChangeRecord, bool skipShowRecord, + bool suppressFocusChange, bool jumpedToCurrentRecord) { Clerk = clerk; HvoOfCurrentObjAtTimeOfNavigation = Clerk != null && Clerk.CurrentObjectHvo != 0 ? Clerk.CurrentObjectHvo : 0; SuppressSaveOnChangeRecord = suppressSaveOnChangeRecord; SkipShowRecord = skipShowRecord; SuppressFocusChange = suppressFocusChange; + JumpedToCurrentRecord = jumpedToCurrentRecord; + } + + /// + /// True when a JumpToRecord asked for the record that was already current, so the index + /// did not change. A view that leaves its scroll position alone when the current record + /// is merely re-asserted should still bring the record into view for this navigation. + /// + public bool JumpedToCurrentRecord + { + get; private set; } /// diff --git a/Src/xWorks/xWorksTests/Avalonia/Hosting/RecordClerkNavigationContextTests.cs b/Src/xWorks/xWorksTests/Avalonia/Hosting/RecordClerkNavigationContextTests.cs index f11b34ad8c..278a55d0a3 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Hosting/RecordClerkNavigationContextTests.cs +++ b/Src/xWorks/xWorksTests/Avalonia/Hosting/RecordClerkNavigationContextTests.cs @@ -144,6 +144,56 @@ public void SelectionBridge_PublishSelection_ByHvo_AndRejectsUnknownKeys() "unknown key shapes are rejected, not guessed"); } + /// + /// LT-22676: both ways of re-asserting the current record broadcast RecordNavigation + /// with the same index, and only the JumpToRecord one says so. + /// + [Test] + public void JumpToCurrentRecord_MarksTheBroadcast_ButReassertingTheIndexDoesNot() + { + LoadRecordEditView("lexiconEdit"); + DrainMediatorAndIdleQueues(); + var control = m_propertyTable.GetValue("currentContentControlObject", null) as RecordEditView; + Assert.That(control, Is.Not.Null); + EnsureCurrentRecord(control); + var clerk = control.Clerk; + var mediator = m_propertyTable.GetValue("window").Mediator; + var spy = new RecordNavigationSpy(); + mediator.AddColleague(spy); + try + { + clerk.JumpToRecord(clerk.CurrentObjectHvo); + DrainMediatorAndIdleQueues(); + Assert.That(spy.Last, Is.Not.Null, "JumpToRecord on the current record still broadcasts"); + Assert.That(spy.Last.JumpedToCurrentRecord, Is.True); + + spy.Last = null; + clerk.JumpToIndex(clerk.CurrentIndex); + DrainMediatorAndIdleQueues(); + Assert.That(spy.Last, Is.Not.Null, "JumpToIndex on the current index still broadcasts"); + Assert.That(spy.Last.JumpedToCurrentRecord, Is.False); + } + finally + { + mediator.RemoveColleague(spy); + } + } + + // Keeps the most recent RecordNavigation payload the mediator delivered. + private sealed class RecordNavigationSpy : IxCoreColleague + { + public RecordNavigationInfo Last { get; set; } + public void Init(Mediator mediator, PropertyTable propertyTable, XmlNode configurationParameters) { } + public IxCoreColleague[] GetMessageTargets() => new IxCoreColleague[] { this }; + public bool ShouldNotCall => false; + public int Priority => (int)ColleaguePriority.High; + public bool OnRecordNavigation(object argument) + { + Last = argument as RecordNavigationInfo; + return false; + } + } + private void LoadRecordEditView(string toolValue) { var windowConfiguration = m_propertyTable.GetValue("WindowConfiguration");