From 419b1a95fbc602c3199be2b4546aa1fe8f332837 Mon Sep 17 00:00:00 2001 From: Vaclav Tvrdik Date: Thu, 24 Sep 2026 16:49:52 +0200 Subject: [PATCH] fix "n" and "p" keys not working all the time --- Default.sublime-keymap | 30 +++++++------- tests/test_commands.py | 93 ++++++++++++++++++++++++++++++++++++++++++ tests/test_render.py | 18 ++++---- vdiff/commands.py | 2 + vdiff/render.py | 4 ++ vdiff/state.py | 13 ++++-- 6 files changed, 133 insertions(+), 27 deletions(-) diff --git a/Default.sublime-keymap b/Default.sublime-keymap index 85f01aa..721d42e 100644 --- a/Default.sublime-keymap +++ b/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"}, diff --git a/tests/test_commands.py b/tests/test_commands.py index a2f33a8..8972d8f 100644 --- a/tests/test_commands.py +++ b/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" diff --git a/tests/test_render.py b/tests/test_render.py index b9b4055..7579d8e 100644 --- a/tests/test_render.py +++ b/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 -# is move the caret, and a vim user should not have to leave command mode to scroll a diff. -# escape carries its own context (panel / overlay / popup / selection guards) instead. -UNGUARDED_ACTIONS = {"line_down", "line_up", "close"} +def test_no_binding_yields_to_vintage_command_mode(): + """The review view is read-only, so its letters are vdiff commands in either vim mode - -def test_command_mode_guard_is_deliberate(): - """Every binding yields to Vintage except the ones that documented a reason not to""" + init_view opens the view in command mode; a binding that still carried the old + `command_mode: false` context would go dead there, which is what broke n / p. + """ guard = {"key": "setting.command_mode", "operand": False} for binding in keymap_bindings(): - unguarded = binding["args"]["action"] in UNGUARDED_ACTIONS - assert (guard in binding["context"]) != unguarded, binding["keys"] + assert guard not in binding["context"], binding["keys"] + view = sublime.View() + render.init_view(view) + assert view.settings().get("command_mode") is True def test_minihtml_stylesheets_reject_unsupported_selectors(): diff --git a/vdiff/commands.py b/vdiff/commands.py index 592b1f8..7f4c935 100644 --- a/vdiff/commands.py +++ b/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 diff --git a/vdiff/render.py b/vdiff/render.py index 491fb82..1b1fdf7 100644 --- a/vdiff/render.py +++ b/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(): diff --git a/vdiff/state.py b/vdiff/state.py index 2415268..8505f42 100644 --- a/vdiff/state.py +++ b/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""" - return find_change(self.files, self.change.path) if self.change else -1 + """Position in `files` of the file being shown or loaded, or -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)"""