Make a review pass reachable from the keyboard

CHANGELOG.md

@@ -16,6 +16,11 @@ All notable changes to this project are documented here. Versions follow
   way, grey none, `—` nothing to sign off. Folders count what is underneath.
 - **Right-click a row** to show that file in Explorer, or copy its full path.
 - **Syntax colouring** in the diff panes for C/H and ARXML/XML.
+- **Find in the open file** (`Ctrl+F`) — steps through every line that names
+  what you typed, on either side, and keeps the search when you move to
+  another file.
+- **`Hide identical`** leaves only the files with a difference in the tree.
+  Verdicts, counts and the exported report are unchanged.
 
 ### Changed
 
@@ -47,9 +52,15 @@ All notable changes to this project are documented here. Versions follow
   `Release notes` and `About`; `Export report` sits next to `Review mode`;
   per-file navigation moved into the diff header. Two-line landing screen, a
   `Ready` status chip, and the CURRENT folder's name as the window title.
+- **A finished scan opens on the first change**, instead of on an empty pane.
+- **`F7` / `F8` walk the whole compare.** At the end of a file they carry on
+  into the next one with something to review, and round again at the end.
 
 ### Fixed
 
+- A report that cannot be written — missing folder, or the file open in
+  another program — now says so and exits `2` (compare incomplete) instead of
+  printing a Python error and exiting like an ordinary run with changes.
 - Added and deleted files now show a minimap, so they scroll like any other.
 - Quick-changes rows open on the object they name — an A2L characteristic, a
   port, an RTE access point — instead of on the file's first change.

CLAUDE.md

@@ -151,7 +151,7 @@ The exit code is a contract with somebody's pipeline. Do not change it:
 |---|---|
 | 0 | No real change |
 | 1 | Real changes found (the CI gate) |
-| 2 | Compare INCOMPLETE — a path could not be listed, read or compared |
+| 2 | Compare INCOMPLETE — a path could not be listed, read or compared, or the report could not be written (no record == not a clean run) |
 
 `build.ps1` produces one `dist\compare-tool.exe` carrying the CLI and the
 viewer. It is a **console** build on purpose: a terminal run must keep stdout

README.md

@@ -69,10 +69,15 @@ Exit codes:
 |---|---|
 | `0` | No real changes |
 | `1` | Real changes found (useful as a CI gate) |
-| `2` | **Compare INCOMPLETE** — some path could not be listed, read or compared (permissions, file locked by another process, long paths, …) |
+| `2` | **Compare INCOMPLETE** — some path could not be listed, read or compared (permissions, file locked by another process, long paths, …), or the report could not be written |
 
 Exit `2` always shows: `!!` in the terminal, a red banner in the report. `--exit-zero` does not suppress it.
 
+A report path that cannot be written (missing folder, file open in a browser,
+read-only) is exit `2` with a one-line reason — never a traceback, and never
+exit `1`, which a pipeline reads as the ordinary "real changes found". What the
+scan did find is still printed.
+
 ## Command line
 
 | Flag | Meaning |
@@ -100,6 +105,13 @@ python -m compare_tool --qt <old_gen_folder> <new_gen_folder> # or start loaded
 
 Two ways in: `Open folders…` for two folders you name yourself, and `Git compare…` for **one** folder in a git checkout — it lists the commits that touched that folder, checks the one you pick out to a temp folder (read-only — your working copy is never touched), and compares as usual.
 
+Reading a scan:
+
+- The scan **opens on the first change** — the pane is never empty next to a tree full of results.
+- `F8` / `F7` step through the changes in the open file and then **carry on into the next (previous) file** with something to review, wrapping at the end. `Ctrl+Home` / `Ctrl+End` stay inside the file.
+- `Ctrl+F` **finds text in the open file** (either side, `F3` / `Shift+F3` to step, `Esc` to close). The query survives moving to another file, so an identifier can be chased across the compare.
+- `Hide identical` leaves only the files with a difference in the tree. It is a view: verdicts, counts and the exported report are untouched.
+
 `Review mode` adds the note box and a `Review` column in the tree — green when every change in a row is signed off, amber part way, grey when none is. Sign off one change (`Ctrl+R`) or a whole file (`Ctrl+Shift+R`); the notes travel into the exported report.
 
 Full walkthrough is built into the app — `Help` → `User guide` (`F1`), which works offline. Standalone `.exe` (no Python needed): see [Single-file build](#single-file-build).

compare_tool/main.py

@@ -16,19 +16,50 @@
                       summarize_rte, summarize_swcs)
 
 
+class ReportWriteError(Exception):
+    """The report path could not be written, so this run left no record.
+
+    Carries the scan it could not write (``results`` / ``counts``, None when
+    the failure came before the scan) so the caller can still print what was
+    found instead of throwing the whole run away.
+    """
+
+    def __init__(self, message, results=None, counts=None):
+        super().__init__(message)
+        self.results = results
+        self.counts = counts
+
+
+def _write_hint(out, err):
+    """Why a report path could not be written, in the reviewer's terms rather
+    than as a traceback: the two everyday causes are a folder that does not
+    exist and a report still open in a browser holding the file."""
+    reason = err.strerror or str(err)
+    if not out.parent.is_dir():
+        reason += ' -- the folder {} does not exist'.format(out.parent)
+    elif isinstance(err, PermissionError):
+        reason += ' -- the file may be open in a browser or editor, or read-only'
+    return '{}: {}'.format(out, reason)
+
+
 def default_report_name(arxml_only):
     return 'arxml_update.html' if arxml_only else 'compare_report.html'
 
 
 def run_compare(old_root, new_root, out, arxml_only=False, exclude=(),
                 progress=None, reviews=None):
     """Scan two trees and write the HTML report.
-    Returns (results, counts)."""
+    Returns (results, counts). Raises :class:`ReportWriteError` when the report
+    could not be written -- a run whose record does not exist is not a run that
+    may report success."""
     out = Path(out)
     # delete a leftover report from an earlier run BEFORE scanning: if this
     # run dies, a stale report must not pass for this run's result
-    if out.exists():
-        out.unlink()
+    try:
+        if out.exists():
+            out.unlink()
+    except OSError as e:
+        raise ReportWriteError(_write_hint(out, e))
     include = tuple('*' + ext for ext, rs in RULES.items()
                     if rs in ('arxml', 'a2l')) if arxml_only else ()
     results = scan(old_root, new_root, progress=progress, exclude=exclude,
@@ -40,7 +71,10 @@ def run_compare(old_root, new_root, out, arxml_only=False, exclude=(),
         page = build_arxml_report(results, old_root, new_root)
     else:
         page = build_report(results, old_root, new_root, reviews)
-    out.write_text(page, encoding='utf-8')
+    try:
+        out.write_text(page, encoding='utf-8')
+    except OSError as e:
+        raise ReportWriteError(_write_hint(out, e), results, counts)
     return results, counts
 
 
@@ -238,9 +272,23 @@ def progress(done, total, rel):
         if done % 50 == 0 or done == total:
             print('  {}/{} {}'.format(done, total, rel))
 
-    results, counts = run_compare(old_root, new_root, out, args.arxml_only,
-                                  exclude=args.exclude, progress=progress,
-                                  reviews=reviews)
+    try:
+        results, counts = run_compare(old_root, new_root, out, args.arxml_only,
+                                      exclude=args.exclude, progress=progress,
+                                      reviews=reviews)
+    except ReportWriteError as e:
+        # what WAS scanned still goes to the terminal -- the compare itself may
+        # have been fine, it is only the record that is missing
+        if e.results is not None:
+            for line in summary_lines(e.results, e.counts):
+                print(line)
+        print('!! REPORT NOT WRITTEN -- {}'.format(e), file=sys.stderr)
+        print('!! This run left no record: treat it as INCOMPLETE.',
+              file=sys.stderr)
+        # exit 2, never 1: 1 is the gate's "real changes found", a perfectly
+        # normal outcome, and a run that produced no report must not be
+        # indistinguishable from it (--exit-zero cannot mask this either)
+        return 2
     for line in summary_lines(results, counts):
         print(line)
 

compare_tool/qtviewer/app.py

@@ -104,6 +104,10 @@ def __init__(self, old=None, new=None, exclude=(), arxml_only=False):
         self._units = {}          # rel -> reviewable units, read once per scan
         self._git_temp = None     # where commits are checked out, this session
         self._old_label = None    # what OLD is, when its folder does not say
+        # one scan, one automatic jump to the first change. Flipping a compare
+        # rule re-judges the same scan and must NOT drag the reviewer off the
+        # file they are reading.
+        self._autoselect = False
 
         self.setWindowTitle('AUTOSAR CodeGen Compare — viewer')
         self.setWindowIcon(app_icon())
@@ -160,12 +164,24 @@ def __init__(self, old=None, new=None, exclude=(), arxml_only=False):
             'timestamps, renames, whitespace): each such file is then reported '
             'as Identical or Modified.')
         self.cb_unimportant.toggled.connect(self._apply_rules)
+        # a display filter, NOT a compare rule: it removes rows from the tree
+        # without touching a verdict, which is why it sits apart from the two
+        # above. A regenerated tree is mostly untouched files, and scrolling
+        # past hundreds of '=' rows to reach five changed ones is its own way
+        # of hiding them.
+        self.cb_hide_identical = QCheckBox('Hide identical')
+        self.cb_hide_identical.setToolTip(
+            'Leave only the files with a difference in the tree. Nothing is '
+            're-judged: verdicts, counts and the exported report are unchanged. '
+            'Files folded to Identical by the two boxes on the left go too.')
+        self.cb_hide_identical.toggled.connect(self._refresh_tree_keep_selection)
         rules = QHBoxLayout()
         rules.setContentsMargins(0, 0, 0, 0)
         rules.addWidget(QLabel('Report:'))
         rules.addWidget(self.cb_comment)
         rules.addWidget(self.cb_unimportant)
         rules.addStretch(1)
+        rules.addWidget(self.cb_hide_identical)
 
         tree_box = QWidget()
         lv = QVBoxLayout(tree_box)
@@ -272,18 +288,21 @@ def _make_actions(self):
                                 'commits — no second folder to choose')
         self.act_git.triggered.connect(self._pick_commit)
 
+        # First/Last stay inside the open file -- they mean "this file's ends".
+        # Previous/Next run off them into the next file with something to
+        # review, so a whole compare can be walked on F7/F8 alone.
         nav = (('act_first', 'nav-first-change', 'First change', 'Ctrl+Home',
-                self.diff.first_change),
+                self.diff.first_change, 'in this file'),
                ('act_prev', 'nav-prev-change', 'Previous change', 'F7',
-                self.diff.prev_change),
+                self._prev_change, 'crosses into the previous changed file'),
                ('act_next', 'nav-next-change', 'Next change', 'F8',
-                self.diff.next_change),
+                self._next_change, 'crosses into the next changed file'),
                ('act_last', 'nav-last-change', 'Last change', 'Ctrl+End',
-                self.diff.last_change))
-        for attr, glyph, text, key, slot in nav:
+                self.diff.last_change, 'in this file'))
+        for attr, glyph, text, key, slot, scope in nav:
             act = QAction(icon(glyph), text, self)
             act.setShortcut(key)
-            act.setToolTip('{} ({}) — noise is skipped'.format(text, key))
+            act.setToolTip('{} ({}) — noise is skipped, {}'.format(text, key, scope))
             act.triggered.connect(slot)
             setattr(self, attr, act)
         # these four live in the diff pane's own header, beside the file name
@@ -796,6 +815,7 @@ def _on_progress(self, done, total, rel):
 
     def _on_done(self, results):
         self._raw_results = results
+        self._autoselect = True  # consumed by _apply_rules, below
         # the rollup reports the scan itself, never the folded view: a hidden
         # category must not make the model look untouched
         self.summary.set_results(results)
@@ -823,6 +843,9 @@ def _apply_rules(self):
         self.diff.set_fold_modes([self._FOLD_MODE[f] for f in fold])
         self._refresh_tree()
         self._reselect(keep)  # keep the reviewer on the file they were reading
+        if self._autoselect:
+            self._autoselect = False
+            self._select_first_change()
         counts = summarize(self.results)
         self.counts_label.setText(
             '{real-change} modified · {comment-only} comment-only · '
@@ -907,8 +930,17 @@ def _refresh_tree(self):
         self.tree.clear()
         if not self.results:
             return
-        self._fill_tree(filter_nodes(build_nodes(self.results),
-                                     text=self.filter_edit.text()))
+        self._fill_tree(filter_nodes(
+            build_nodes(self.results), text=self.filter_edit.text(),
+            hide_identical=self.cb_hide_identical.isChecked()))
+
+    def _refresh_tree_keep_selection(self):
+        """Rebuild the tree and put the reviewer back on the file they were
+        reading. Toggling a display filter is not a reason to lose the diff on
+        screen -- unless that very file is what the filter just hid."""
+        keep = self._selected_rel()
+        self._refresh_tree()
+        self._reselect(keep)
 
     def _fill_tree(self, nodes):
         def add(parent, node, prefix):
@@ -1039,6 +1071,80 @@ def _selected_rel(self):
         items = self.tree.selectedItems()
         return items[0].data(0, REL_ROLE) if items else None
 
+    # --- walking the files, not just the changes inside one ---
+    #
+    # verdicts a review pass has to look at: everything that is not noise. A
+    # finished scan opens on the first of these, and F7/F8 step between them
+    # once the current file runs out of changes.
+    _NAV_STATUS = ('error', 'real-change', 'added', 'deleted')
+
+    def _tree_rels(self):
+        """Every file row, in the order the tree shows it.
+
+        Read from the TREE and not from the results dict on purpose: rows the
+        path filter or `Hide identical` took off screen must not be what F8
+        jumps to, and the tree is the only thing that knows what is visible.
+        """
+        out = []
+
+        def walk(item):
+            rel = item.data(0, REL_ROLE)
+            if rel is not None:
+                out.append(rel)
+            for i in range(item.childCount()):
+                walk(item.child(i))
+
+        for i in range(self.tree.topLevelItemCount()):
+            walk(self.tree.topLevelItem(i))
+        return out
+
+    def _is_nav(self, rel):
+        r = self.results.get(rel)
+        return bool(r) and r['status'] in self._NAV_STATUS
+
+    def _select_first_change(self):
+        """Open the first file a reviewer would have to read. Called once per
+        scan: landing on an empty pane next to a tree full of results makes the
+        reviewer's first act a hunt for where the changes are, which the tool
+        already knows."""
+        rel = next((r for r in self._tree_rels() if self._is_nav(r)), None)
+        if rel:
+            self._reselect(rel)
+
+    def _step_file(self, delta, to_last=False):
+        """Move to the next (delta +1) or previous (-1) file with something to
+        review, wrapping round the whole compare. `to_last` parks on that
+        file's LAST change, so stepping backwards continues where the eye is."""
+        order = self._tree_rels()
+        nav = [r for r in order if self._is_nav(r)]
+        if not nav:
+            return
+        cur = self._selected_rel()
+        nxt = None
+        if cur in order:
+            i = order.index(cur)
+            after = order[i + 1:] if delta > 0 else list(reversed(order[:i]))
+            nxt = next((r for r in after if self._is_nav(r)), None)
+        wrapped = nxt is None
+        if wrapped:
+            nxt = nav[0] if delta > 0 else nav[-1]
+        self._reselect(nxt)  # loads the file and parks on its first change
+        if to_last:
+            self.diff.last_change()
+        self.statusBar().showMessage(
+            '{}{}'.format('Wrapped to ' if wrapped else '', nxt), 4000)
+
+    def _next_change(self):
+        """F8: the next change in this file, or the first change of the next
+        file with any. One key walks the whole compare instead of dead-ending
+        at the bottom of whichever file happens to be open."""
+        if not self.diff.next_change():
+            self._step_file(1)
+
+    def _prev_change(self):
+        if not self.diff.prev_change():
+            self._step_file(-1, to_last=True)
+
     def _jump_to_name(self, rel, key):
         """Open `rel` from the quick-changes panel, on the hunk about `key`.
 
@@ -1112,6 +1218,12 @@ def _on_select(self):
                text-align:center; color:#d0d0d0; }
 QProgressBar::chunk { background:#4F46E5; border-radius:5px; }
 QCheckBox { spacing:6px; }
+/* find strip: a band of its own between the header and the code, so it reads
+   as a tool over the diff rather than as part of the file being read */
+QWidget#findbar { background:#212226; border-top:1px solid #34363c;
+                  border-bottom:1px solid #34363c; }
+QWidget#findbar QToolButton { color:#9aa1ad; padding:2px 6px; border-radius:4px; }
+QWidget#findbar QToolButton:hover { background:#34363c; color:#e8e8e8; }
 """