Skip to content

Commit 02bce76

Browse files
authored
Do not throw the reader to the top for selecting what is already selected (#830)
Select reset the scroll unconditionally. A right click on the highlighted entry goes through it - OpenMenu selects the row first, so the menu acts on what is highlighted - so opening that menu, to reveal the source file say, jumped the comparison back to line 1 before the menu appeared. A left click on the highlighted row and a socket focus naming the entry already on screen did the same. Selecting the entry already selected now leaves the scroll alone. A different entry is a different comparison and still starts at its top.
1 parent 459e5a9 commit 02bce76

2 files changed

Lines changed: 81 additions & 0 deletions

File tree

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
/// <summary>
2+
/// Selecting the entry that is already selected. A left click on the highlighted row, a right
3+
/// click to open its menu, and a focus naming it all arrive as a selection, and the reader is part
4+
/// way down a comparison while they do.
5+
/// </summary>
6+
public class ReselectTests
7+
{
8+
[Test]
9+
public async Task Opening_the_menu_on_the_entry_being_read_keeps_the_scroll()
10+
{
11+
var state = Scrolled();
12+
var row = VisibleRowOf(state, state.Selected);
13+
14+
var opened = ViewerSession.OpenMenu(state, row);
15+
16+
await Assert.That(opened.Menu).IsNotNull();
17+
await Assert.That(opened.ScrollTop).IsEqualTo(state.ScrollTop);
18+
}
19+
20+
[Test]
21+
public async Task Focusing_the_entry_being_read_keeps_the_scroll()
22+
{
23+
var state = Scrolled();
24+
25+
var focused = ViewerSession.SelectKey(state, state.Current!.Key);
26+
27+
await Assert.That(focused.ScrollTop).IsEqualTo(state.ScrollTop);
28+
}
29+
30+
/// <summary>
31+
/// A different entry is a different comparison, so that one does start at the top.
32+
/// </summary>
33+
[Test]
34+
public async Task Selecting_another_entry_starts_at_its_top()
35+
{
36+
var state = Scrolled();
37+
38+
var selected = ViewerSession.SelectKey(state, state.Queue[0].Key);
39+
40+
await Assert.That(selected.Current!.Key).IsEqualTo(state.Queue[0].Key);
41+
await Assert.That(selected.ScrollTop).IsEqualTo(0);
42+
}
43+
44+
static SessionState Scrolled()
45+
{
46+
var state = Fixtures.Inline(
47+
Fixtures.Patch("A.cs", 1, null, Fixtures.Long(true)),
48+
Fixtures.Patch("B.cs", 2, null, Fixtures.Long(true)));
49+
state = ViewerSession.SelectKey(state, state.Queue[1].Key);
50+
state = ViewerSession.Apply(state, CommandKind.PageDown);
51+
if (state.ScrollTop == 0)
52+
{
53+
throw new("The entry did not scroll, so nothing below asserts anything.");
54+
}
55+
56+
return state;
57+
}
58+
59+
static int VisibleRowOf(SessionState state, int entry)
60+
{
61+
var visible = QueueProjection.Visible(state, ScreenBuilder.BodyRows(state), out _);
62+
for (var index = 0; index < visible.Count; index++)
63+
{
64+
if (visible[index].Kind == QueueRowKind.Entry &&
65+
visible[index].EntryIndex == entry)
66+
{
67+
return index;
68+
}
69+
}
70+
71+
throw new($"Entry {entry} is not on screen.");
72+
}
73+
}

src/DiffEngineViewer/ViewerSession.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -900,6 +900,14 @@ static SessionState Select(SessionState state, int index)
900900
return state;
901901
}
902902

903+
// Already the entry on screen. Selecting it is what a click on it does, what a right click
904+
// opening its menu does, and what a focus naming it does, and none of those asks to be
905+
// taken back to the top of what is being read.
906+
if (index == state.Selected)
907+
{
908+
return Clamp(state);
909+
}
910+
903911
return Clamp(state with
904912
{
905913
Selected = index,

0 commit comments

Comments
 (0)