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/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); 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");