diff --git a/Default.sublime-keymap b/Default.sublime-keymap index b2acaab..1464540 100644 --- a/Default.sublime-keymap +++ b/Default.sublime-keymap @@ -23,6 +23,8 @@ "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] }, { "keys": ["p"], "command": "vdiff_action", "args": {"action": "prev_file"}, "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] }, + { "keys": ["?"], "command": "vdiff_action", "args": {"action": "help"}, + "context": [{"key": "setting.vdiff_view"}, {"key": "setting.command_mode", "operand": false}] }, { "keys": ["escape"], "command": "vdiff_action", "args": {"action": "close"}, "context": [ {"key": "setting.vdiff_view"}, diff --git a/README.md b/README.md index 21af1c1..7045e2b 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,7 @@ Keys inside the review view: | `]` / `[` | next / previous change | | `n` / `p` | next / previous file of the current list | | `r` | refresh | +| `?` | show this key list in a popup | | Esc | close the view | The header links do the same with the mouse. Settings: `vDiff.sublime-settings`. diff --git a/tests/stubs/sublime.py b/tests/stubs/sublime.py index a0c9786..6ef8251 100644 --- a/tests/stubs/sublime.py +++ b/tests/stubs/sublime.py @@ -113,6 +113,9 @@ class View: self.name_ = "" self.sel_ = [] self.status = {} + self.commands = [] + self.popup = None + self.popup_navigate = None def id(self): return 1 @@ -129,6 +132,8 @@ class View: def run_command(self, name, args=None): if name == "vdiff_replace": self.replace(None, Region(0, self.size()), args["text"]) + else: + self.commands.append((name, args)) def set_read_only(self, flag): self.read_only = flag @@ -185,6 +190,18 @@ class View: def show_at_center(self, point): pass + def show_popup(self, content, flags=0, location=-1, max_width=320, max_height=240, on_navigate=None, on_hide=None): + self.popup, self.popup_navigate = content, on_navigate + + def hide_popup(self): + self.popup = None + + def is_popup_visible(self): + return self.popup is not None + + def em_width(self): + return 8.0 + def style_for_scope(self, scope): return {"foreground": "#ffffff", "background": "#333333"} diff --git a/tests/test_commands.py b/tests/test_commands.py index 23fb891..65ef149 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -124,3 +124,14 @@ def test_run_async_reports_git_error(monkeypatch): commands.run_async(boom, lambda r: messages.append("done")) assert messages == ["vDiff: fatal: bad revision 'nope'"] + + +def test_run_async_reports_generic_error(monkeypatch): + messages = [] + monkeypatch.setattr(commands.sublime, "status_message", lambda m: messages.append(m)) + + def boom(): + raise ValueError("boom") + + commands.run_async(boom, lambda r: messages.append("done")) + assert messages == ["vDiff: boom"] diff --git a/tests/test_render.py b/tests/test_render.py index 6a6f108..c5279df 100644 --- a/tests/test_render.py +++ b/tests/test_render.py @@ -1,3 +1,7 @@ +import json +import os +import re + import sublime # the stub, via conftest from vdiff import commands, git, listeners, parse, render, state # noqa: F401 @@ -110,3 +114,93 @@ def test_binary_and_caret_index(): view.sel().clear() view.sel().add(sublime.Region(0)) assert render.caret_index(view) == -1 + + +# --- key help popup --------------------------------------------------------- + + +def help_popup(st): + """Open the popup through the action registry and return (view, html)""" + view = sublime.View() + render.init_view(view) + commands.ACTIONS["help"](view, None, st) + return view, view.popup + + +def test_help_popup_lists_every_key(): + st = make_state() + view, html = help_popup(st) + + assert view.is_popup_visible() + for group in ("Navigate", "Panels", "This file", "View"): + assert ">{}<".format(group) in html + for keys in ("] [", "n p", "o ⏎", "f", "c", "s", "u", "d", "r", "?", "esc"): + assert ">{}<".format(keys) in html + assert "next / previous change" in html and "open the real file at this line" in html + # working-tree mode: nothing dimmed, every single-action row is a link + assert 'class="dim"' not in html and "uncommitted only" not in html + assert 'href="stage"' in html and 'href="files"' in html and 'href="close"' in html + # rows covering two actions stay plain text + assert 'href="next_change"' not in html + # the status bar advertises it too + assert "? keys" in view.status[render.STATUS_KEY] + + +def test_help_popup_dims_file_actions_in_commit_mode(): + st = make_state() + st.enter("abc", git.CommitInfo("abc", "abc1234", "Ann", "2026-09-23 10:00", "Fix things", "")) + _, html = help_popup(st) + + assert "uncommitted only" in html + assert html.count('class="dim"') == 3 + assert 'href="stage"' not in html and 'href="unstage"' not in html and 'href="discard"' not in html + # everything that still applies stays clickable + assert 'href="files"' in html and 'href="open"' in html and 'href="refresh"' in html + + +def test_help_popup_toggles_and_navigates(): + st = make_state() + view, _ = help_popup(st) + + commands.ACTIONS["help"](view, None, st) + assert not view.is_popup_visible() + + commands.ACTIONS["help"](view, None, st) + assert view.is_popup_visible() + view.popup_navigate("files") + assert not view.is_popup_visible() + assert view.commands[-1] == ("vdiff_action", {"action": "files"}) + + +# keymap spellings that the popup shows as a symbol +DISPLAY_KEYS = {"enter": "⏎", "escape": "esc"} + + +def test_keymap_matches_help_table(): + """The keymap, ACTIONS and the popup agree on both the actions and the keys that reach them""" + path = os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "Default.sublime-keymap") + with open(path, encoding="utf-8") as f: + bindings = json.load(f) + + assert all(b["command"] == "vdiff_action" for b in bindings) + bound = {b["args"]["action"] for b in bindings} + listed = {action for k in render.KEYS for action in k.actions} + assert bound == set(commands.ACTIONS) == listed + + # rebinding a key without updating the table would leave the popup and status bar lying + bound_keys = {DISPLAY_KEYS.get(key, key) for b in bindings for key in b["keys"]} + assert bound_keys == {token for k in render.KEYS for token in k.keys.split()} + + +def test_minihtml_stylesheets_reject_unsupported_selectors(): + """minihtml drops the rest of a stylesheet at a selector it cannot parse; these are the forms + that silently broke HELP_CSS, so no stylesheet constant may use them""" + # descendant/child/sibling combinators, pseudo-classes, attribute selectors, compound classes + unsupported = re.compile(r"[\s:>~+\[]|[.#][^.#]*[.#]") + sheets = [(name, css) for name, css in vars(render).items() if name.endswith("_CSS") and isinstance(css, str)] + assert len(sheets) >= 2, sheets + for name, css in sheets: + for rule in css.split("}"): + head = rule.split("{")[0].strip() + for selector in filter(None, (s.strip() for s in head.split(","))): + assert not unsupported.search(selector), "{}: {}".format(name, selector) diff --git a/vdiff/commands.py b/vdiff/commands.py index 598e40d..8be8272 100644 --- a/vdiff/commands.py +++ b/vdiff/commands.py @@ -107,12 +107,10 @@ def file_items(files): description = git.STATUS_NAMES.get(change.status, "") colour = STATUS_COLOURS.get(change.status) kind = (colour, change.status, description) if colour else sublime.KIND_AMBIGUOUS - details = description - if change.staged: - details += ", staged" + details = ", ".join(filter(None, (description, "staged" if change.staged else ""))) if change.old_path: details += " ← " + change.old_path - if change.insertions is None or change.deletions is None: + if None in (change.insertions, change.deletions): annotation = "binary" else: annotation = "-{} +{}".format(change.deletions, change.insertions) @@ -283,6 +281,10 @@ def _act_close(view, window, st): view.close() +def _act_help(view, window, st): + render.show_help(view, st) + + def _git_action(verb, done_verb, operation, confirm=None): """Action running `operation(repo, change)` on the shown working-tree file, then re-listing""" @@ -362,6 +364,7 @@ ACTIONS = { "files": _act_files, "commits": _act_commits, "close": _act_close, + "help": _act_help, "stage": _git_action("stage", "staged", git.stage), "unstage": _git_action("unstage", "unstaged", git.unstage), "discard": _git_action("discard", "discarded", git.discard, confirm=_confirm_discard), diff --git a/vdiff/git.py b/vdiff/git.py index 1d23ac1..4b9ba67 100644 --- a/vdiff/git.py +++ b/vdiff/git.py @@ -45,10 +45,12 @@ class GitError(Exception): def run(repo, args, ok_codes=(0,)): """Run git in `repo`, return stdout as text""" cmd = [GIT_BINARY, "-c", "core.quotepath=off", "--no-pager"] + list(args) - proc = subprocess.run(cmd, cwd=repo, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + proc = subprocess.run( + cmd, cwd=repo, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8", errors="replace" + ) if proc.returncode not in ok_codes: - raise GitError(proc.stderr.decode("utf-8", "replace"), proc.returncode) - return proc.stdout.decode("utf-8", "replace") + raise GitError(proc.stderr, proc.returncode) + return proc.stdout def repo_root(path): @@ -79,6 +81,11 @@ def base_ref(repo): return "HEAD" if has_head(repo) else EMPTY_TREE +def _int_or_none(value): + """A numstat count: "-" marks a binary file""" + return None if value == "-" else int(value) + + def _parse_numstat(text): """`--numstat -z` output -> {path: (insertions, deletions, old_path)}""" result = {} @@ -91,9 +98,9 @@ def _parse_numstat(text): old_path = None if path == "": # rename: counts, then old and new path as separate NUL fields - old_path = next(fields, None) + old_path = next(fields, "") or None path = next(fields, "") - result[path] = (None if ins == "-" else int(ins), None if dele == "-" else int(dele), old_path) + result[path] = (_int_or_none(ins), _int_or_none(dele), old_path) return result @@ -146,7 +153,8 @@ def changed_files(repo): for entry in entries: kind = entry[0] if kind == "1": - xy, path = entry.split(" ", 8)[1], entry.split(" ", 8)[-1] + fields = entry.split(" ", 8) + xy, path = fields[1], fields[-1] elif kind == "u": xy, path = "UU", entry.split(" ", 10)[-1] elif kind == "?": diff --git a/vdiff/listeners.py b/vdiff/listeners.py index dadf30a..928642e 100644 --- a/vdiff/listeners.py +++ b/vdiff/listeners.py @@ -17,7 +17,7 @@ class VdiffEventListener(sublime_plugin.EventListener): def on_post_save(self, view): """Review views showing the saved file re-render; the fingerprint decides which ones""" - for view_id, st in list(state.STATES.items()): + for view_id, st in state.tracked(): review = sublime.View(view_id) if review.is_valid(): refresh_if_changed(review, st) diff --git a/vdiff/parse.py b/vdiff/parse.py index 8631f6b..8fc0b34 100644 --- a/vdiff/parse.py +++ b/vdiff/parse.py @@ -1,7 +1,8 @@ """ Unified diff -> lines with old/new numbers, change blocks, word-level ranges; no sublime imports """ import difflib import re -from collections import namedtuple +from collections import Counter, namedtuple +from itertools import groupby # kind is " " (context), "-" (deleted) or "+" (inserted); the missing number is None Line = namedtuple("Line", "kind old_no new_no text") @@ -51,31 +52,22 @@ def parse_unified(text): return Diff(lines, binary) +def runs(items, key): + """(key value, start, end) for each maximal run of consecutive items with the same key""" + for value, group in groupby(enumerate(items), key=lambda pair: key(pair[1])): + indexes = [i for i, _ in group] + yield value, indexes[0], indexes[-1] + 1 + + def change_blocks(lines): """[(start, end)) index ranges of consecutive changed lines""" - blocks = [] - start = None - for i, line in enumerate(lines): - if line.kind != " ": - if start is None: - start = i - elif start is not None: - blocks.append((start, i)) - start = None - if start is not None: - blocks.append((start, len(lines))) - return blocks + return [(start, end) for changed, start, end in runs(lines, lambda line: line.kind != " ") if changed] def counts(lines): """(insertions, deletions)""" - ins = dele = 0 - for line in lines: - if line.kind == "+": - ins += 1 - elif line.kind == "-": - dele += 1 - return ins, dele + tally = Counter(line.kind for line in lines) + return tally["+"], tally["-"] def paired_lines(lines, block): diff --git a/vdiff/render.py b/vdiff/render.py index 041282d..cfe51e4 100644 --- a/vdiff/render.py +++ b/vdiff/render.py @@ -3,6 +3,7 @@ import html import json import logging import os +from collections import namedtuple import sublime import sublime_plugin @@ -46,7 +47,39 @@ FALLBACK_RULES = [ {"scope": SCOPE_INS_CHAR, "background": "hsla(114, 45%, 68%, 0.30)", "foreground_adjust": "l(+ 10%)"}, ] -KEY_HINTS = "f files c commits s stage u unstage d discard o open ] [ change n p file r refresh" +# one source for the status-bar hints and the ? popup. +# actions: the ACTIONS names the row covers; a single one makes the row a link. short: status-bar +# wording, "" keeps the row out of it. The group flag is False for groups needing a working-tree file. +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("o \u23ce", "open the real file at this line", "open", ("open",)), + ]), + ("Panels", True, [ + Key("f", "changed files", "files", ("files",)), + Key("c", "recent commits", "commits", ("commits",)), + ]), + ("This file", False, [ + Key("s", "stage", "stage", ("stage",)), + Key("u", "unstage", "unstage", ("unstage",)), + Key("d", "discard", "discard", ("discard",)), + ]), + ("View", True, [ + Key("r", "refresh", "refresh", ("refresh",)), + Key("?", "this list", "keys", ("help",)), + Key("esc", "close the view", "", ("close",)), + ]), +] + +KEYS = [key for _, _, group in KEY_GROUPS for key in group] + +KEY_HINTS = " ".join("{} {}".format(k.keys, k.short) for k in KEYS if k.short) + +# the popup font is the editor's, so the key column is padded to a fixed width in characters +_KEY_WIDTH = max(len(k.keys) for k in KEYS) HEADER_CSS = """ body { margin: 0; padding: 0.5rem 0.8rem; background-color: color(var(--background) blend(var(--foreground) 93%)); } @@ -64,6 +97,21 @@ a { text-decoration: none; padding: 0.15rem 0.5rem; margin-left: 0.3rem; border- .body { color: color(var(--foreground) alpha(0.6)); } """ +# minihtml stops applying a stylesheet at the first selector it cannot parse, and its documentation +# covers properties only, not selectors: every rule here sticks to one bare element or one class, +# the forms HEADER_CSS proves work. The first group's margin stands in for body's top padding, and +# .kdim takes its colour from the .dim row around it. +HELP_CSS = """ +body { margin: 0; padding: 0 0.8rem 0.6rem 0.8rem; } +.group { margin-top: 0.6rem; color: var(--bluish); font-weight: bold; } +.note { color: color(var(--foreground) alpha(0.5)); font-weight: normal; } +.row { color: color(var(--foreground) alpha(0.85)); } +.dim { color: color(var(--foreground) alpha(0.35)); } +.k { color: var(--orangish); font-weight: bold; } +.kdim { font-weight: bold; } +a { text-decoration: none; color: color(var(--foreground) alpha(0.85)); } +""" + class VdiffReplaceCommand(sublime_plugin.TextCommand): """Replace the whole buffer; the only writer of a review view""" @@ -212,16 +260,10 @@ def _row_starts(lines): def _line_regions(lines, starts): """Full-line regions of deleted and inserted rows, consecutive rows merged""" regions = {"-": [], "+": []} - run_kind = None - run_start = 0 - for i in range(len(lines) + 1): - kind = lines[i].kind if i < len(lines) else None - if kind == run_kind: - continue - if run_kind in regions: - # min(): the last line's region stops at the buffer end instead of one past it - regions[run_kind].append(sublime.Region(starts[run_start], min(starts[i], starts[-1]))) - run_kind, run_start = kind, i + for kind, start, end in parse.runs(lines, lambda line: line.kind): + if kind in regions: + # starts has a sentinel at len(lines): the last line stops at the buffer end + regions[kind].append(sublime.Region(starts[start], starts[end])) return regions["-"], regions["+"] @@ -261,6 +303,7 @@ def header_html(st, ins, dele): actions.append('Open') actions.append('Files') actions.append('Commits') + actions.append('?') commit = "" if st.in_commit and st.commit: @@ -278,16 +321,60 @@ def header_html(st, ins, dele): ) +def _run_action(view, href): + """Dispatch a header or popup link through the same command the keymap uses""" + view.run_command("vdiff_action", {"action": href}) + + def _header(view, st, ins, dele): """One block phantom above the text; redrawn from scratch, so no PhantomSet state to keep""" def navigate(href): - view.run_command("vdiff_action", {"action": href}) + _run_action(view, href) view.erase_phantoms(PHANTOM_KEY) view.add_phantom(PHANTOM_KEY, sublime.Region(0), header_html(st, ins, dele), sublime.LAYOUT_BLOCK, navigate) +# --- key help popup --------------------------------------------------------- + + +def help_html(st): + """The minihtml key list; rows that need a working-tree file are dimmed while a commit is shown""" + parts = [] + for title, applies, group in KEY_GROUPS: + dim = st.in_commit and not applies + heading = html.escape(title) + if dim: + heading += ' uncommitted only' + parts.append('
{}
'.format(heading)) + # one class per element: a compound selector would silently drop the rest of HELP_CSS + row_class, key_class = ("dim", "kdim") if dim else ("row", "k") + for key in group: + row = '{}{}{}'.format( + key_class, html.escape(key.keys), " " * (_KEY_WIDTH - len(key.keys) + 2), html.escape(key.label) + ) + if len(key.actions) == 1 and not dim: + row = '{}'.format(key.actions[0], row) + parts.append('
{}
'.format(row_class, row)) + return '{}'.format(HELP_CSS, "".join(parts)) + + +def show_help(view, st): + """Toggle the key list popup at the caret""" + if view.is_popup_visible(): + view.hide_popup() + return + + def navigate(href): + view.hide_popup() + _run_action(view, href) + + # minihtml sizes follow the editor font, so fixed pixel bounds would clip at a large font size + em = view.em_width() or 8 + view.show_popup(help_html(st), location=-1, max_width=int(em * 56), max_height=int(em * 44), on_navigate=navigate) + + # --- colour scheme fallback ------------------------------------------------- _scheme_checked = set() diff --git a/vdiff/state.py b/vdiff/state.py index 996f123..2415268 100644 --- a/vdiff/state.py +++ b/vdiff/state.py @@ -1,5 +1,6 @@ """ Per-view state of review views, keyed by view id; the essentials are mirrored into view settings for restore """ from collections import namedtuple +from dataclasses import dataclass, field # view setting marking a review view (keymap context "setting.vdiff_view") VIEW_FLAG = "vdiff_view" @@ -18,25 +19,25 @@ def find_change(files, path): return -1 +@dataclass(eq=False) class ViewState: """Everything a review view needs to re-render itself and act on its file `sha` is None in working-tree mode and the reviewed commit's hash otherwise. """ - def __init__(self, repo): - self.repo = repo - self.sha = None - self.commit = None # git.CommitInfo when sha is set - self.change = None # git.FileChange currently shown - self.files = [] # git.FileChange list the user is browsing (next/prev file) - self.lines = [] # parse.Line list currently rendered - self.blocks = [] # parse.change_blocks(lines) - self.binary = False - self.token = 0 # bumped per show request; a stale async result is dropped - self.rendered_token = -1 # token of the request that produced the current render - self.highlight = 0 # bumped per quick-panel highlight, to debounce previews - self.fingerprint = None # (file mtime, index mtime) at render time, working-tree mode + repo: str + 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 + 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) + binary: bool = False + token: int = 0 # bumped per show request; a stale async result is dropped + rendered_token: int = -1 # token of the request that produced the current render + highlight: int = 0 # bumped per quick-panel highlight, to debounce previews + fingerprint: object = None # (file mtime, index mtime) at render time, working-tree mode @property def in_commit(self): @@ -77,6 +78,11 @@ def get(view): return STATES.get(view.id()) +def tracked(): + """Snapshot of (view id, state) for every tracked review view""" + return list(STATES.items()) + + def put(view, st): STATES[view.id()] = st return st diff --git a/vdiff/utils.py b/vdiff/utils.py index 9f239d2..77360a3 100644 --- a/vdiff/utils.py +++ b/vdiff/utils.py @@ -38,10 +38,10 @@ def run_async(work, done): def job(): try: result = work() - except (git.GitError, OSError) as error: - LOG.warning("git failed: %s", error) - # `error` is unbound once the except block ends; the callback runs later + except Exception as error: + # the traceback helps when debugging; the status message is what the user sees message = str(error) or error.__class__.__name__ + LOG.warning("vdiff worker failed: %s", message, exc_info=LOG.isEnabledFor(logging.DEBUG)) sublime.set_timeout(lambda: status(message)) return sublime.set_timeout(lambda: done(result))