"n" and "p" keys cycle over files

README.md

@@ -22,7 +22,7 @@ Keys inside the review view:
 | `C` or `m` | commit the staged files, typing the message in an input panel |
 | `o` or Enter | open the real file at this line |
 | `]` / `[` | next / previous change |
-| `n` / `p` | next / previous file of the current list |
+| `n` / `p` | next / previous file of the current list (wraps at the ends) |
 | `j` / `k` | move down / up one line (vim keys) |
 | `r` | refresh |
 | `?` | show this key list in a popup |

tests/test_commands.py

@@ -329,3 +329,77 @@ def test_esc_rewinds_the_file_navigation_counts_from(loading):
     st.restore(kept)
     commands.ACTIONS["next_file"](view, window, st)
     assert requested_path(queue) == "f4.py"
+
+
+# --- cycling past the ends --------------------------------------------------
+
+
+@pytest.fixture
+def rings(monkeypatch):
+    """Record visual bells instead of drawing them"""
+    bells = []
+    monkeypatch.setattr(commands.render, "flash", lambda view: bells.append(view))
+    return bells
+
+
+def at_file(files, path):
+    """A loaded working-tree view sitting on `path`"""
+    view, window = sublime.View(), sublime.Window()
+    st = state.ViewState("/repo")
+    st.files = files
+    st.change = st.requested = next(c for c in files if c.path == path)
+    return view, window, st
+
+
+def test_next_file_wraps_past_the_last(loading, rings):
+    queue, _ = loading
+    files = numbered()
+    view, window, st = at_file(files, "f4.py")
+
+    commands.ACTIONS["next_file"](view, window, st)
+    assert requested_path(queue) == "f0.py" and len(rings) == 1
+
+
+def test_prev_file_wraps_before_the_first(loading, rings):
+    queue, _ = loading
+    files = numbered()
+    view, window, st = at_file(files, "f0.py")
+
+    commands.ACTIONS["prev_file"](view, window, st)
+    assert requested_path(queue) == "f4.py" and len(rings) == 1
+
+
+def test_stepping_inside_the_list_does_not_ring(loading, rings):
+    queue, _ = loading
+    files = numbered()
+    view, window, st = at_file(files, "f2.py")
+
+    commands.ACTIONS["next_file"](view, window, st)
+    commands.ACTIONS["prev_file"](view, window, st)
+    assert requested_path(queue) == "f2.py" and rings == []
+
+
+def test_single_file_list_rings_without_refetching(loading, rings):
+    queue, _ = loading
+    files = numbered(1)
+    view, window, st = at_file(files, "f0.py")
+
+    commands.ACTIONS["next_file"](view, window, st)
+    commands.ACTIONS["prev_file"](view, window, st)
+    # it rings both times, but never re-runs git for the file already on screen
+    assert len(rings) == 2 and queue == []
+
+
+def test_first_step_without_a_shown_file_picks_an_end(loading, rings):
+    queue, _ = loading
+    files = numbered()
+    view, window, st = commit_panel(files)  # opening a commit leaves nothing shown
+    assert st.index() == -1
+
+    commands.ACTIONS["next_file"](view, window, st)
+    assert requested_path(queue) == "f0.py"
+    st.requested = None
+    commands.ACTIONS["prev_file"](view, window, st)
+    assert requested_path(queue) == "f4.py"
+    # neither is a wrap: there was no position to run past
+    assert rings == []

tests/test_render.py

@@ -270,3 +270,36 @@ def test_summary_is_omitted_without_a_file_list():
     st = make_state()
     render.render(view, st)
     assert 'class="summary"' not in header(view)
+
+
+def test_flash_tints_the_buffer_then_clears(monkeypatch):
+    view = sublime.View("some text")
+    timers = []
+    monkeypatch.setattr(render.sublime, "set_timeout", lambda fn, ms=0: timers.append((fn, ms)))
+
+    render.flash(view)
+    regions, scope, _, flags = view.regions[render.BELL_KEY]
+    assert regions == [sublime.Region(0, len(view.text))] and flags == sublime.DRAW_NO_OUTLINE
+    assert scope and timers[0][1] == render.BELL_MS
+
+    timers[0][0]()
+    assert render.BELL_KEY not in view.regions
+
+
+def test_flash_survives_a_re_render(monkeypatch):
+    """The wrap that rings also re-renders; _replace must not wipe the bell before it is seen"""
+    assert render.BELL_KEY not in render.REGION_KEYS
+    monkeypatch.setattr(render.sublime, "set_timeout", lambda fn, ms=0: None)  # hold the bell up
+    view = sublime.View()
+    render.init_view(view)
+    render.flash(view)
+    render.render(view, make_state())
+    assert render.BELL_KEY in view.regions
+
+
+def test_bell_scope_falls_back_when_the_scheme_lacks_it(monkeypatch):
+    view = sublime.View()
+    assert render._bell_scope(view) == render.SCOPE_BELL
+    monkeypatch.setattr(view, "style_for_scope", lambda scope: {"foreground": "#fff"})
+    # an invisible bell is a silent failure, so fall back to a scope the plugin guarantees
+    assert render._bell_scope(view) == render.SCOPE_INS

vdiff/commands.py

@@ -359,10 +359,19 @@ def _step_file(delta):
         if not st.files:
             status("no file list, press f or c")
             return
-        index = st.index() + delta
+        count = len(st.files)
-        if not 0 <= index < len(st.files):
+        position = st.index()
-            status("no more files")
+        if position < 0:
-            return
+            # nothing shown yet: n starts at the first file, p at the last. Not a wrap.
+            index, wrapped = (0 if delta > 0 else count - 1), False
+        else:
+            index = position + delta
+            wrapped = not 0 <= index < count
+            index %= count  # -1 is the last file, count is the first
+        if wrapped:
+            render.flash(view)
+        if index != position:
+            # a one-file list wraps onto itself: ring, but do not re-run git for the same file
             show_file(view, st, st.files[index])
 
     return act

vdiff/render.py

@@ -26,6 +26,11 @@ REGION_KEYS = (KEY_DEL, KEY_INS, KEY_DEL_GUTTER, KEY_INS_GUTTER, KEY_DEL_CHAR, K
 PHANTOM_KEY = "vdiff.header"
 STATUS_KEY = "vdiff"
 
+# visual bell: a whole-buffer tint, erased on a timer. Deliberately not in REGION_KEYS, so the
+# re-render a wrap triggers does not wipe it before it has been seen.
+BELL_KEY = "vdiff.bell"
+BELL_MS = 90
+
 # a block phantom anchored at point 0 is drawn below row 0, so row 0 stays empty and the file starts at row 1.
 # Only index_point / caret_index know about it.
 _HEADER_ROWS = 1
@@ -38,6 +43,8 @@ SCOPE_INS_CHAR = "diff.inserted.char"
 # gutter dots use the scope's foreground
 SCOPE_DEL_MARK = "markup.deleted"
 SCOPE_INS_MARK = "markup.inserted"
+# ST4 defines region.<colour>ish in every bundled scheme for exactly this kind of transient marking
+SCOPE_BELL = "region.yellowish"
 
 # rules written into a colour-scheme override when the active scheme lacks the scopes above
 FALLBACK_RULES = [
@@ -55,7 +62,7 @@ Key = namedtuple("Key", "keys label short actions")
 KEY_GROUPS = [
     ("Navigate", True, [
         Key("] [", "next / previous change", "change", ("next_change", "prev_change")),
-        Key("n p", "next / previous file", "file", ("next_file", "prev_file")),
+        Key("n p", "next / previous file (wraps)", "file", ("next_file", "prev_file")),
         Key("o \u23ce", "open the real file at this line", "open", ("open",)),
         Key("j k", "down / up one line (vim)", "", ("line_down", "line_up")),
     ]),
@@ -176,6 +183,23 @@ def _mtime(path):
         return None
 
 
+def flash(view):
+    """Visual bell: tint the buffer for a moment, for feedback that has nothing to say in words"""
+    view.add_regions(BELL_KEY, [sublime.Region(0, view.size())], _bell_scope(view), flags=sublime.DRAW_NO_OUTLINE)
+    # unconditional erase: overlapping flashes may cut one short, but none can leave the tint stuck
+    sublime.set_timeout(lambda: view.is_valid() and view.erase_regions(BELL_KEY), BELL_MS)
+
+
+def _bell_scope(view):
+    """region.yellowish, or a scope the plugin guarantees a background for when a scheme lacks it"""
+    try:
+        if view.style_for_scope(SCOPE_BELL).get("background"):
+            return SCOPE_BELL
+    except Exception:
+        LOG.debug("style_for_scope failed for %s", SCOPE_BELL, exc_info=True)
+    return SCOPE_INS
+
+
 def render_message(view, st, message):
     """Show a one-line notice instead of a file (nothing to review, binary, ...)"""
     st.change = None