fix "n" and "p" keys not working all the time

Default.sublime-keymap

@@ -1,38 +1,38 @@
 [
     { "keys": ["f"], "command": "vdiff_action", "args": {"action": "files"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["c"], "command": "vdiff_action", "args": {"action": "commits"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["s"], "command": "vdiff_action", "args": {"action": "stage"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["u"], "command": "vdiff_action", "args": {"action": "unstage"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["d"], "command": "vdiff_action", "args": {"action": "discard"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["o"], "command": "vdiff_action", "args": {"action": "open"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["enter"], "command": "vdiff_action", "args": {"action": "open"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["r"], "command": "vdiff_action", "args": {"action": "refresh"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["]"], "command": "vdiff_action", "args": {"action": "next_change"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["["], "command": "vdiff_action", "args": {"action": "prev_change"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["n"], "command": "vdiff_action", "args": {"action": "next_file"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["p"], "command": "vdiff_action", "args": {"action": "prev_file"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["j"], "command": "vdiff_action", "args": {"action": "line_down"},
       "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["k"], "command": "vdiff_action", "args": {"action": "line_up"},
       "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["C"], "command": "vdiff_action", "args": {"action": "commit"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["m"], "command": "vdiff_action", "args": {"action": "commit"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["?"], "command": "vdiff_action", "args": {"action": "help"},
-      "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] },
+      "context": [{"key": "setting.vdiff_view"}] },
     { "keys": ["escape"], "command": "vdiff_action", "args": {"action": "close"},
       "context": [
         {"key": "setting.vdiff_view"},

tests/test_commands.py

@@ -1,3 +1,5 @@
+import pytest
+
 import sublime  # the stub, via conftest
 
 from vdiff import commands, git, state
@@ -236,3 +238,94 @@ def test_commit_items_keep_a_readable_subject_when_the_annotation_is_huge():
 
 def test_commit_items_fall_back_for_an_empty_subject():
     assert commands.commit_items([commit("")])[0].trigger == "(no subject)"
+
+
+# --- navigating while a diff is still loading -------------------------------
+
+
+@pytest.fixture
+def loading(monkeypatch):
+    """Queue run_async instead of running it inline, so a test can act mid-load like a user can
+
+    The stub runs set_timeout synchronously, which is why the real plugin's async window - the one
+    n / p used to read stale state through - is invisible to every other test in the suite.
+    """
+    queue = []
+    monkeypatch.setattr(commands, "run_async", lambda work, done: queue.append((work, done)))
+    monkeypatch.setattr(commands.render, "render", lambda view, st: None)
+    monkeypatch.setattr(commands.git, "full_diff", lambda *a, **k: "--- a/x\n+++ b/x\n@@ -1 +1 @@\n-a\n+b\n")
+
+    def flush():
+        while queue:
+            work, done = queue.pop(0)
+            done(work())
+
+    return queue, flush
+
+
+def commit_panel(files):
+    """A review view showing a commit, its file panel open, nothing loaded yet"""
+    view, window = sublime.View(), sublime.Window()
+    st = state.ViewState("/repo")
+    st.enter("abc", git.CommitInfo("abc", "abc1234", "A", "d", "s", ""))
+    commands.show_files_panel(window, view, st, files, st.kept(), placeholder="c")
+    return view, window, st
+
+
+def numbered(count=5):
+    return [git.FileChange("f{}.py".format(i), "M", False, 1, 1, None) for i in range(count)]
+
+
+def requested_path(queue):
+    """Path of the file the newest queued load will fetch"""
+    return queue[-1][0].__closure__[0].cell_contents.path
+
+
+def test_next_file_steps_from_the_file_being_loaded(loading):
+    queue, _ = loading
+    files = numbered()
+    view, window, st = commit_panel(files)
+
+    window.panel[1](2)  # Enter on the third file; its diff has not arrived yet
+    assert st.change is None
+    commands.ACTIONS["next_file"](view, window, st)
+    assert requested_path(queue) == "f3.py"
+
+
+def test_prev_file_works_before_the_diff_arrives(loading, monkeypatch):
+    queue, _ = loading
+    messages = []
+    monkeypatch.setattr(commands.sublime, "status_message", lambda m: messages.append(m))
+    files = numbered()
+    view, window, st = commit_panel(files)
+
+    window.panel[1](2)
+    commands.ACTIONS["prev_file"](view, window, st)
+    assert requested_path(queue) == "f1.py" and messages == []
+
+
+def test_rapid_next_file_presses_each_advance(loading):
+    queue, flush = loading
+    files = numbered()
+    view, window, st = commit_panel(files)
+
+    window.panel[1](1)
+    for _ in range(3):
+        commands.ACTIONS["next_file"](view, window, st)
+    assert requested_path(queue) == "f4.py"
+    flush()
+    assert st.change.path == "f4.py"
+
+
+def test_esc_rewinds_the_file_navigation_counts_from(loading):
+    queue, flush = loading
+    files = numbered()
+    view, window, st = commit_panel(files)
+    window.panel[1](3)
+    flush()
+
+    kept = st.kept()
+    window.panel[1](0)  # preview another file, then back out
+    st.restore(kept)
+    commands.ACTIONS["next_file"](view, window, st)
+    assert requested_path(queue) == "f4.py"

tests/test_render.py

@@ -197,18 +197,18 @@ def test_keymap_matches_help_table():
     assert bound_keys == {token for k in render.KEYS for token in k.keys.split()}
 
 
-# j / k stay live in Vintage command mode on purpose: the view is read-only, so the worst they can do
+def test_no_binding_yields_to_vintage_command_mode():
-# is move the caret, and a vim user should not have to leave command mode to scroll a diff.
+    """The review view is read-only, so its letters are vdiff commands in either vim mode
-# escape carries its own context (panel / overlay / popup / selection guards) instead.
-UNGUARDED_ACTIONS = {"line_down", "line_up", "close"}
 
-
+    init_view opens the view in command mode; a binding that still carried the old
-def test_command_mode_guard_is_deliberate():
+    `command_mode: false` context would go dead there, which is what broke n / p.
-    """Every binding yields to Vintage except the ones that documented a reason not to"""
+    """
     guard = {"key": "setting.command_mode", "operand": False}
     for binding in keymap_bindings():
-        unguarded = binding["args"]["action"] in UNGUARDED_ACTIONS
+        assert guard not in binding["context"], binding["keys"]
-        assert (guard in binding["context"]) != unguarded, binding["keys"]
+    view = sublime.View()
+    render.init_view(view)
+    assert view.settings().get("command_mode") is True
 
 
 def test_minihtml_stylesheets_reject_unsupported_selectors():

vdiff/commands.py

@@ -84,6 +84,8 @@ def open_review(window):
 def show_file(view, st, change):
     """Fetch and parse the file's diff on the worker thread, then render it"""
     st.token += 1
+    # set before the worker starts: n / p count from here while the diff is still loading
+    st.requested = change
     token = st.token
     repo, sha = st.repo, st.sha
 

vdiff/render.py

@@ -138,6 +138,10 @@ def init_view(view):
     s.set("show_definitions", False)
     s.set("mini_diff", False)
     s.set("spell_check", False)
+    # a read-only viewer, so vim's insert mode is meaningless: open in command mode with the block
+    # caret. vdiff's own keys carry no command_mode context, so they fire in either mode regardless.
+    s.set("command_mode", True)
+    s.set("inverse_caret_state", True)
     view.set_scratch(True)
     view.set_read_only(True)
     if not view.name():

vdiff/state.py

@@ -30,6 +30,7 @@ class ViewState:
     sha: object = None  # None in working-tree mode, the reviewed commit's hash otherwise
     commit: object = None  # git.CommitInfo when sha is set
     change: object = None  # git.FileChange currently shown
+    requested: object = None  # file asked for, maybe still loading; navigation counts from here
     files: list = field(default_factory=list)  # git.FileChange list the user is browsing (next/prev file)
     lines: list = field(default_factory=list)  # parse.Line list currently rendered
     blocks: list = field(default_factory=list)  # parse.change_blocks(lines)
@@ -44,8 +45,13 @@ class ViewState:
         return self.sha is not None
 
     def index(self):
-        """Position of the shown file in `files`, or -1"""
+        """Position in `files` of the file being shown or loaded, or -1
-        return find_change(self.files, self.change.path) if self.change else -1
+
+        `requested` leads: a diff arrives asynchronously, so counting from `change` alone would make
+        n / p step from whatever was on screen before the pending file landed.
+        """
+        change = self.requested or self.change
+        return find_change(self.files, change.path) if change else -1
 
     def kept(self):
         return Kept(self.sha, self.commit, self.change, list(self.files))
@@ -54,12 +60,13 @@ class ViewState:
         """Switch to browsing a commit (or the working tree); returns what to restore on Esc"""
         kept = self.kept()
         if sha != self.sha:
-            self.change = None
+            self.change = self.requested = None
         self.sha, self.commit = sha, commit
         return kept
 
     def restore(self, kept):
         self.sha, self.commit, self.change, self.files = kept
+        self.requested = kept.change
 
     def persist(self, view):
         """Mirror the essentials into view settings (survives a restart)"""