From 649b6e5f7670fb4093c78351a83262853450db6f Mon Sep 17 00:00:00 2001 From: longvo920 Date: Tue, 28 Jul 2026 23:42:51 +0700 Subject: [PATCH 1/2] feat: make a review pass reachable from the keyboard Five things a reviewer hit on the way through a real compare: - A report that could not be written died with a traceback and exit 1 -- the same code a pipeline reads as "real changes found", so a run that produced no record looked like an ordinary one. It is exit 2 now, with the reason in a sentence, and the scan it could not write still prints. - A finished scan left the diff pane empty next to a tree full of results; it opens on the first change instead. - F7/F8 wrapped silently inside one file, so walking the compare meant going back to the mouse between every file. They now run off the end into the next (previous) file with something to review. - No way to find an identifier in the open file. Ctrl+F searches the rows, so a hit on either side counts once and both panes land on it; the query survives moving to another file, which is how a name gets chased across the compare. - A regenerated tree is mostly untouched files. `Hide identical` drops those rows -- a view only: verdicts, counts and the exported report do not notice it. The find box needed the rows of a one-sided file, which also gave whole added/deleted files a working _reveal: scrolling is now driven by the pane that actually holds the text. --- CHANGELOG.md | 11 ++ CLAUDE.md | 2 +- README.md | 14 +- compare_tool/main.py | 62 +++++++- compare_tool/qtviewer/app.py | 128 +++++++++++++++- compare_tool/qtviewer/dialogs.py | 26 +++- compare_tool/qtviewer/diffpane.py | 247 +++++++++++++++++++++++++++--- compare_tool/qtviewer/tree.py | 31 ++-- tests/test_diffpane_qt.py | 224 +++++++++++++++++++++++++++ tests/test_failsafe.py | 49 ++++++ tests/test_qtviewer.py | 26 ++++ 11 files changed, 767 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7938cad..3a2be6e 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/CLAUDE.md b/CLAUDE.md index 6e3d120..b2376fe 100644 --- a/CLAUDE.md +++ b/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 diff --git a/README.md b/README.md index 6a328d9..5d6cd7c 100644 --- a/README.md +++ b/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 # 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). diff --git a/compare_tool/main.py b/compare_tool/main.py index 786d807..abd6f94 100644 --- a/compare_tool/main.py +++ b/compare_tool/main.py @@ -16,6 +16,32 @@ 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' @@ -23,12 +49,17 @@ def default_report_name(arxml_only): 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) diff --git a/compare_tool/qtviewer/app.py b/compare_tool/qtviewer/app.py index 6b4f0f0..da350f6 100644 --- a/compare_tool/qtviewer/app.py +++ b/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; } """ diff --git a/compare_tool/qtviewer/dialogs.py b/compare_tool/qtviewer/dialogs.py index 78850d4..0053c81 100644 --- a/compare_tool/qtviewer/dialogs.py +++ b/compare_tool/qtviewer/dialogs.py @@ -36,9 +36,11 @@ ## 3. Read the tree -- Every file always listed -- a verdict never removes a row +- Every file listed -- a verdict never removes a row on its own - A folder shows its heaviest child verdict - Box above the tree filters by path +- `Hide identical` leaves only the files with a difference. It is a view, not a + rule: verdicts, counts and the exported report do not change - Right-click a row: show it in Explorer, or copy its full path | Mark | Verdict | Meaning | @@ -70,19 +72,29 @@ | dim red / green | noise: comment, UUID, rename, whitespace | | blue | moved block | -## 6. Navigate and export +## 6. Navigate, find and export +- The scan opens on the first change it found -- no hunting for it - Header shows `change 3 of 7` +- `F8` past the last change of a file goes on to the **next file** with + something to review; `F7` goes back the same way. One key walks the whole + compare, and it wraps round at the end +- `Ctrl+Home` / `Ctrl+End` stay inside the open file +- `Ctrl+F` finds text in the open file -- either side counts, the match is + marked amber, and the search is kept when you move to another file - Export writes the full HTML report -- folded categories still included - **Quick changes** panel: AUTOSAR / A2L rollup, click a row to jump straight to the line that names that object | Shortcut | Action | |---|---| -| `Ctrl+Home` | First change | -| `F7` | Previous change | -| `F8` | Next change | -| `Ctrl+End` | Last change | +| `Ctrl+Home` | First change (this file) | +| `F7` | Previous change, crossing into the previous changed file | +| `F8` | Next change, crossing into the next changed file | +| `Ctrl+End` | Last change (this file) | +| `Ctrl+F` | Find in this file | +| `F3` / `Shift+F3` | Next / previous match | +| `Esc` | Close the find bar | | `Ctrl+R` | Mark this change reviewed | | `Ctrl+Shift+R` | Mark the whole file reviewed | | `Ctrl+E` | Export report | @@ -112,6 +124,8 @@ ## Fail-safe - Unreadable or uncompared path -> red banner, `‼` mark, exit code `2` +- A report that could not be written is exit `2` too: a run with no record must + not look like a clean one - Never silently skipped """ diff --git a/compare_tool/qtviewer/diffpane.py b/compare_tool/qtviewer/diffpane.py index 38a05bf..368d219 100644 --- a/compare_tool/qtviewer/diffpane.py +++ b/compare_tool/qtviewer/diffpane.py @@ -17,11 +17,13 @@ from pathlib import Path -from PySide6.QtCore import QRect, QSize, Qt, Signal -from PySide6.QtGui import (QColor, QFont, QPainter, QTextBlockFormat, - QTextCharFormat, QTextCursor, QTextFormat) -from PySide6.QtWidgets import (QHBoxLayout, QLabel, QPlainTextEdit, QSplitter, - QStackedWidget, QTextEdit, QVBoxLayout, QWidget) +from PySide6.QtCore import QEvent, QRect, QSize, Qt, Signal +from PySide6.QtGui import (QColor, QFont, QKeySequence, QPainter, QShortcut, + QTextBlockFormat, QTextCharFormat, QTextCursor, + QTextFormat) +from PySide6.QtWidgets import (QHBoxLayout, QLabel, QLineEdit, QPlainTextEdit, + QSplitter, QStackedWidget, QTextEdit, + QToolButton, QVBoxLayout, QWidget) from .. import review from ..scanner import looks_binary, read_text @@ -102,6 +104,10 @@ def _semantic_summary(result): # F7/F8 are visibly doing something even when the file fits on screen and # there is nothing to scroll _CUR_BG = QColor(255, 255, 255, 34) +# the find hit itself. Amber on purpose: red, green and blue already mean +# removed, added and moved, so a fourth hue is the only way a search result can +# be told apart from a verdict about the code. +_FIND_BG = QColor('#7a6320') # OLD/NEW pane-banner accents: one source, used for both the tag text and the # underline so the two can never drift apart _OLD_ACCENT = '#c98b8b' @@ -298,6 +304,9 @@ def __init__(self): bl.setSpacing(0) bl.addWidget(self._split, 1) bl.addWidget(self.minimap) + self._find_bar = self._build_find_bar() + self._find_bar.setVisible(False) + diff_page = QWidget() dl = QVBoxLayout(diff_page) dl.setContentsMargins(0, 0, 0, 0) @@ -308,6 +317,7 @@ def __init__(self): # being pushed to the bottom by an oversized header gap dl.addLayout(head_row) dl.addWidget(self._sem) + dl.addWidget(self._find_bar) dl.addWidget(body, 1) self.addWidget(msg_page) # index 0 @@ -328,7 +338,19 @@ def __init__(self): self._head_base = '' # header without the "change k of N" suffix self._pos_text = '' # "change k of N", folded into the header text self._syncing = False + # which editor scrolling is driven from. The old pane normally carries + # both (the scrollbar mirror follows it), but a whole added/deleted file + # puts its text in ONE pane and leaves the other empty -- driving from + # an empty document scrolls nothing at all. + self._drive = self.old_edit + self._hits = [] # rows matching the find box, in file order + self._hit_idx = -1 self._link_scrolls() + # Ctrl+F is where every editor puts find; the pane owns the shortcut so + # it works wherever the focus sits inside the diff + QShortcut(QKeySequence.Find, self).activated.connect(self.open_find) + QShortcut(QKeySequence(Qt.Key_F3), self).activated.connect(self.find_next) + QShortcut(QKeySequence('Shift+F3'), self).activated.connect(self.find_prev) @staticmethod def _pane_banner(accent): @@ -351,6 +373,163 @@ def _pane(banner, editor): lay.addWidget(editor, 1) return w + # --- find in this file --- + + def _build_find_bar(self): + """One-line find strip under the header, hidden until Ctrl+F. + + It searches the ROWS, not the two documents: a row is one aligned pair, + so a hit is reported once whichever side carries it, and the jump can + reuse the same reveal-and-highlight the change navigation uses. Both + panes then land on the same line, which is the whole point of a + side-by-side view. + """ + bar = QWidget() + bar.setObjectName('findbar') + lay = QHBoxLayout(bar) + lay.setContentsMargins(10, 4, 6, 4) + lay.setSpacing(6) + self._find_edit = QLineEdit() + self._find_edit.setPlaceholderText('Find in this file… (Enter next, ' + 'Shift+Enter previous, Esc close)') + self._find_edit.setClearButtonEnabled(True) + self._find_edit.textChanged.connect(self._find_changed) + self._find_edit.returnPressed.connect(self.find_next) + self._find_edit.installEventFilter(self) + self._find_count = QLabel('') + self._find_count.setStyleSheet('color:#9aa1ad; font-size:12px;') + close = QToolButton() + close.setText('✕') + close.setToolTip('Close the find bar (Esc)') + close.setAutoRaise(True) + close.clicked.connect(self.close_find) + lay.addWidget(self._find_edit, 1) + lay.addWidget(self._find_count) + lay.addWidget(close) + return bar + + def eventFilter(self, obj, event): + # Esc closes, Shift+Enter steps back: QLineEdit has no signal for + # either, and both are what every find box in every editor does + if obj is self._find_edit and event.type() == QEvent.KeyPress: + if event.key() == Qt.Key_Escape: + self.close_find() + return True + if (event.key() in (Qt.Key_Return, Qt.Key_Enter) + and event.modifiers() & Qt.ShiftModifier): + self.find_prev() + return True + return super().eventFilter(obj, event) + + def open_find(self): + """Show the find bar and take the focus, selecting what is there so a + second Ctrl+F starts a new search instead of appending to the old one.""" + if self.currentIndex() != 1: + return # nothing to search: landing screen or a message page + self._find_bar.setVisible(True) + self._find_edit.setFocus() + self._find_edit.selectAll() + + def close_find(self): + self._find_bar.setVisible(False) + self._hits = [] + self._hit_idx = -1 + # the focus goes back to the diff, or the next keystroke would vanish + # into a hidden box + self._drive.setFocus() + + def _refresh_find(self): + """Re-run the current query against the file just loaded. + + The query is KEPT across files on purpose -- "where else does this + identifier change" is the reason to search a codegen diff at all, and + retyping it per file is the whole cost of asking. The pane does not + jump to a hit by itself, though: opening a file parks on its first + change, and a search from a previous file must not quietly override + that. Enter or F3 moves. + """ + self._hits = [] + self._hit_idx = -1 + if not self._find_bar.isVisible(): + return + text = self._find_edit.text() + if not text.strip(): + self._find_count.setText('') + return + self._hits = self.find_matches(text) + self._find_count.setText('{} match{}'.format(len(self._hits), + '' if len(self._hits) == 1 else 'es') + if self._hits else 'no match') + + def _find_changed(self, text): + self._hits = self.find_matches(text) + self._hit_idx = -1 + if not text.strip(): + self._find_count.setText('') + return + if not self._hits: + self._find_count.setText('no match') + return + self.find_next() + + def find_matches(self, text): + """Row indices containing `text` on either side, case-insensitive. + + Public so the behaviour can be tested without driving the widget: the + rows are the model, the bar is only a way to walk them. + """ + text = text.strip().lower() + if not text: + return [] + return [i for i, r in enumerate(self.rows) + if text in (r.old_txt or '').lower() + or text in (r.new_txt or '').lower()] + + def _goto_hit(self, idx): + self._hit_idx = idx % len(self._hits) + self._find_count.setText('{} of {}'.format(self._hit_idx + 1, + len(self._hits))) + row = self._hits[self._hit_idx] + self._reveal(row) + self._mark_match(row, self._find_edit.text()) + + def _mark_match(self, row, text): + """Paint the matched text itself, over the row overlay ``_reveal`` just + laid down. Without it a hit in a long line is 'the pane scrolled a bit' + and the reviewer still has to read the line to find the word.""" + needle = text.strip().lower() + if not needle: + return + for editor in (self.old_edit, self.new_edit): + doc = editor.document() + if doc.blockCount() <= row: + continue + block = doc.findBlockByNumber(row) + line = block.text().lower() + sels = list(editor.extraSelections()) + at = line.find(needle) + while at >= 0: + sel = QTextEdit.ExtraSelection() + sel.format.setBackground(_FIND_BG) + cur = QTextCursor(block) + cur.setPosition(block.position() + at) + cur.setPosition(block.position() + at + len(needle), + QTextCursor.KeepAnchor) + sel.cursor = cur + sels.append(sel) # after the row overlay, so it paints on top + at = line.find(needle, at + len(needle)) + editor.setExtraSelections(sels) + + def find_next(self): + if not self._hits: + return + self._goto_hit(self._hit_idx + 1) + + def find_prev(self): + if not self._hits: + return + self._goto_hit(self._hit_idx - 1) + def set_old_label(self, text=None, tip=None): """Name the BASELINE pane something other than its folder. @@ -421,6 +600,9 @@ def clear(self): self._logo.setVisible(False) self._msg.setText(_HINT) self._forget_units() + self._find_bar.setVisible(False) # no file: nothing to search + self._hits = [] + self._hit_idx = -1 self.setCurrentIndex(0) def _forget_units(self): @@ -458,6 +640,7 @@ def show_file(self, rel, result, old_root, new_root): self._units = [] # nothing to sign off on a file we could not read self._message('{}\n\nCould not render — treat as potentially ' 'changed.\n{}: {}'.format(rel, type(e).__name__, e)) + self._refresh_find() self.unitChanged.emit() def file_units(self): @@ -566,6 +749,7 @@ def _load_rows(self, rel, status, result=None, row_map=None): # back to the two-pane layout: the old editor drives again (its # scrollbar mirror carries the new pane), whatever a previous # one-sided file left the map pointing at + self._drive = self.old_edit self.minimap.set_editor(self.old_edit) self.minimap.set_rows(rows) @@ -617,7 +801,12 @@ def _load_rows(self, rel, status, result=None, row_map=None): self.old_edit.verticalScrollBar().setValue(0) def _load_one_side(self, rel, label, lines, side): - self.rows = [] + # rows are marked 'ctx': the pane is already one solid colour, so the + # map has nothing to add by repeating it -- but they ARE the file, and + # the find box searches rows, so a whole added file has to have them + self.rows = [Row(None, None, i + 1, line, 'ctx', 'ctx') + if side == 'new' else Row(i + 1, line, None, None, 'ctx', 'ctx') + for i, line in enumerate(lines)] self._stops = [] self._pos_text = '' self._sem.setVisible(False) @@ -641,12 +830,10 @@ def _load_one_side(self, rel, label, lines, side): for i in range(len(lines)): self._block_bg(edit, i, bg) # the map still shows the file's shape, so a whole added or deleted - # file scrolls like any other. Rows are marked 'ctx' on purpose: the - # pane is already one solid colour, and repeating that on the map would - # be a red or green rectangle carrying no information. + # file scrolls like any other -- driven by the pane that holds the text + self._drive = edit self.minimap.set_editor(edit) - self.minimap.set_rows([Row(None, None, i + 1, line, 'ctx', 'ctx') - for i, line in enumerate(lines)]) + self.minimap.set_rows(self.rows) self.setCurrentIndex(1) @staticmethod @@ -700,11 +887,12 @@ def _reveal(self, row, context=3): The change block is also highlighted on both sides: without it, a file that fits on screen has nothing to scroll and the navigation looks dead even though it moved.""" - block = self.old_edit.document().findBlockByNumber(row) - self.old_edit.setTextCursor(QTextCursor(block)) + drive = self._drive + block = drive.document().findBlockByNumber(row) + drive.setTextCursor(QTextCursor(block)) # NoWrap: the vertical scrollbar is in lines, so its value is the top # visible line index - self.old_edit.verticalScrollBar().setValue(max(0, row - context)) + drive.verticalScrollBar().setValue(max(0, row - context)) self._highlight_block(row) self._update_position(row) self.unitChanged.emit() @@ -720,6 +908,10 @@ def _highlight_block(self, row): while end + 1 < len(rows) and rows[end + 1].mode == rows[row].mode != 'ctx': end += 1 for editor in (self.old_edit, self.new_edit): + # a one-sided file leaves the other document empty: selecting a + # block it does not have would be a null cursor + if editor.document().blockCount() <= end: + continue sels = [] for i in range(start, end + 1): sel = QTextEdit.ExtraSelection() @@ -742,18 +934,33 @@ def _update_position(self, row): self._header.setText('{} · {}'.format(self._head_base, self._pos_text)) # --- change navigation (real/moved blocks; noise is skipped) --- + # + # next_change / prev_change stop at the end of THIS file and say so with + # False instead of wrapping round to the other end. Wrapping inside one + # file was silent and it dead-ended the review pass: the window catches the + # False and moves to the next changed file, so F8 walks the whole compare. def next_change(self): + """Step to the next change. False when this file has no next one.""" if not self._stops: - return - cur = self.old_edit.textCursor().blockNumber() - self._reveal(next((s for s in self._stops if s > cur), self._stops[0])) + return False + cur = self._drive.textCursor().blockNumber() + nxt = next((s for s in self._stops if s > cur), None) + if nxt is None: + return False + self._reveal(nxt) + return True def prev_change(self): + """Step to the previous change. False when this file has no earlier one.""" if not self._stops: - return - cur = self.old_edit.textCursor().blockNumber() - self._reveal(next((s for s in reversed(self._stops) if s < cur), self._stops[-1])) + return False + cur = self._drive.textCursor().blockNumber() + prv = next((s for s in reversed(self._stops) if s < cur), None) + if prv is None: + return False + self._reveal(prv) + return True def goto_name(self, key): """Jump to the first changed row naming `key`. True when it landed. diff --git a/compare_tool/qtviewer/tree.py b/compare_tool/qtviewer/tree.py index a6105e9..90d3c56 100644 --- a/compare_tool/qtviewer/tree.py +++ b/compare_tool/qtviewer/tree.py @@ -91,24 +91,35 @@ def walk(node): return walk(root) -def filter_nodes(nodes, text=''): +def filter_nodes(nodes, text='', hide_identical=False): """Narrow the tree to files whose path matches `text` (a directory survives only if a descendant matches, so empty folders collapse away). - Status is deliberately NOT a filter: the folder structure must stay stable - whatever the verdicts are, so a file never disappears from the tree just - because it is identical or noise-only. Hiding a change category folds it - into another verdict (see the compare rules) -- the row stays put and only - its label changes, so the tree never reshuffles under the reviewer.""" + `hide_identical` additionally drops every file the compare found no + difference in. It is the one status-driven filter, and it is OFF by + default and asked for explicitly: a verdict never removes a row on its + own, because the folder structure has to stay stable while the reviewer + works. A regenerated tree is mostly untouched files, though, so scrolling + past hundreds of `=` rows to reach five changed ones is its own kind of + hiding -- the reviewer gets to make that call with a button they can undo. + + Note this composes with the compare rules rather than fighting them: a + folded category re-judges its files as identical, so hiding identical + files hides those too. That is the point -- both say "this does not count". + """ text = text.strip().lower() - if not text: + if not text and not hide_identical: return list(nodes) out = [] for n in nodes: if n.is_dir: - kids = filter_nodes(n.children, text) + kids = filter_nodes(n.children, text, hide_identical) if kids: out.append(n._replace(children=kids)) - elif text in (n.rel or '').lower(): - out.append(n) + continue + if hide_identical and n.status == 'identical': + continue + if text and text not in (n.rel or '').lower(): + continue + out.append(n) return out diff --git a/tests/test_diffpane_qt.py b/tests/test_diffpane_qt.py index ccc1fd9..46fbed3 100644 --- a/tests/test_diffpane_qt.py +++ b/tests/test_diffpane_qt.py @@ -222,5 +222,229 @@ def test_every_row_lands_on_its_own_object_in_the_model_fixture(self): self._check(FIX / 'model_old', FIX / 'model_new') +@unittest.skipUnless(HAVE_QT, 'PySide6 not installed') +class TestFindInFile(unittest.TestCase): + """Ctrl+F searches the rows of the file on screen.""" + + def setUp(self): + from compare_tool.qtviewer.diffpane import DiffPane + self.app = _app() + self.results = scan(FIX / 'old', FIX / 'new') + self.pane = DiffPane() + self.pane.resize(1200, 600) + self.pane.setAttribute(Qt.WA_DontShowOnScreen, True) + self.pane.show() + self.addCleanup(self.pane.close) + + def _open(self, rel): + self.pane.show_file(rel, self.results[rel], + str(FIX / 'old'), str(FIX / 'new')) + for _ in range(5): + self.app.processEvents() + + def test_a_hit_on_either_side_counts_once(self): + self._open('src/rename_conflict.c') + hits = self.pane.find_matches('rtb_') + self.assertTrue(hits) + for row in hits: + r = self.pane.rows[row] + self.assertIn('rtb_', (r.old_txt or '') + (r.new_txt or '')) + # one row, not one per side, or "3 of 8" would count lines twice + self.assertEqual(len(hits), len(set(hits))) + + def test_search_is_case_insensitive_and_empty_finds_nothing(self): + self._open('src/rename_conflict.c') + self.assertEqual(self.pane.find_matches('RTB_'), + self.pane.find_matches('rtb_')) + self.assertEqual(self.pane.find_matches(' '), []) + + def test_a_whole_added_file_is_searchable(self): + # it renders through the one-sided path, which used to leave .rows + # empty -- a file with no rows is a file the find box cannot see + self._open('src/added.c') + self.assertTrue(self.pane.rows) + self.assertTrue(self.pane.find_matches('void')) + + def test_stepping_wraps_and_reports_position(self): + self._open('src/rename_conflict.c') + self.pane.open_find() + self.pane._find_edit.setText('rtb_') + for _ in range(5): + self.app.processEvents() + n = len(self.pane._hits) + self.assertGreater(n, 1) + self.assertEqual(self.pane._find_count.text(), '1 of {}'.format(n)) + for _ in range(n): # once round the whole file + self.pane.find_next() + self.assertEqual(self.pane._find_count.text(), '1 of {}'.format(n)) + self.pane.find_prev() + self.assertEqual(self.pane._find_count.text(), '{} of {}'.format(n, n)) + + def test_the_query_survives_a_file_change_without_moving_the_pane(self): + self._open('src/rename_conflict.c') + self.pane.open_find() + self.pane._find_edit.setText('rtb_') + for _ in range(5): + self.app.processEvents() + self._open('src/real_change.c') + self.assertEqual(self.pane._find_edit.text(), 'rtb_') + # opening a file parks on its FIRST CHANGE; a query carried over from + # another file must not quietly scroll somewhere else + self.assertEqual(self.pane._hit_idx, -1) + self.assertIn(self.pane.old_edit.textCursor().blockNumber(), + self.pane._stops) + + +@unittest.skipUnless(HAVE_QT, 'PySide6 not installed') +class TestChangeNavigationStopsAtTheEnd(unittest.TestCase): + """next/prev report False at the ends instead of wrapping silently. + + The window uses that False to step into the next file, which is what makes + F8 walk the whole compare; wrapping inside one file dead-ended the pass. + """ + + def setUp(self): + from compare_tool.qtviewer.diffpane import DiffPane + self.app = _app() + self.results = scan(FIX / 'old', FIX / 'new') + self.pane = DiffPane() + self.pane.resize(1200, 600) + self.pane.setAttribute(Qt.WA_DontShowOnScreen, True) + self.pane.show() + self.addCleanup(self.pane.close) + self.pane.show_file('src/rename_conflict.c', + self.results['src/rename_conflict.c'], + str(FIX / 'old'), str(FIX / 'new')) + for _ in range(5): + self.app.processEvents() + + def test_next_walks_the_file_then_stops(self): + n = len(self.pane._stops) + self.assertGreater(n, 1) + self.pane.first_change() + for i in range(n - 1): + self.assertTrue(self.pane.next_change(), 'stopped at change {}'.format(i)) + self.assertFalse(self.pane.next_change()) + + def test_prev_stops_at_the_first_change(self): + self.pane.first_change() + self.assertFalse(self.pane.prev_change()) + + def test_a_file_with_no_change_stops_never_claims_to_move(self): + self.pane.show_file('src/rename_only.c', self.results['src/rename_only.c'], + str(FIX / 'old'), str(FIX / 'new')) + for _ in range(5): + self.app.processEvents() + self.assertFalse(self.pane.next_change()) + self.assertFalse(self.pane.prev_change()) + + +@unittest.skipUnless(HAVE_QT, 'PySide6 not installed') +class TestWindowLevelReviewFlow(unittest.TestCase): + """What the reviewer meets right after a scan, and how F7/F8 walk it.""" + + def setUp(self): + from compare_tool.qtviewer.app import MainWindow + self.app = _app() + self.win = MainWindow(str(FIX / 'old'), str(FIX / 'new')) + self.win.resize(1400, 800) + self.win.setAttribute(Qt.WA_DontShowOnScreen, True) + self.win.show() + self.addCleanup(self.win.close) + _settle(self.app, self.win) + + def _settle_ui(self): + for _ in range(5): + self.app.processEvents() + + def test_a_finished_scan_opens_on_the_first_change(self): + rel = self.win._selected_rel() + self.assertIsNotNone(rel, 'the scan left the diff pane empty') + self.assertIn(self.win.results[rel]['status'], self.win._NAV_STATUS) + # the first one in tree order, not just any + self.assertEqual(rel, next(r for r in self.win._tree_rels() + if self.win._is_nav(r))) + + def test_flipping_a_compare_rule_does_not_move_the_reviewer(self): + self.win._reselect('src/rename_conflict.c') + self._settle_ui() + self.win.cb_comment.setChecked(False) + self._settle_ui() + self.assertEqual(self.win._selected_rel(), 'src/rename_conflict.c') + + def test_next_change_crosses_into_the_following_file(self): + nav = [r for r in self.win._tree_rels() if self.win._is_nav(r)] + self.assertGreater(len(nav), 1) + self.win._reselect(nav[0]) + self._settle_ui() + seen = [self.win._selected_rel()] + for _ in range(60): # bounded: pressing F8 must terminate, not loop + self.win._next_change() + self._settle_ui() + rel = self.win._selected_rel() + if rel != seen[-1]: + seen.append(rel) + if len(seen) > len(nav): + break + # every file with something to review, in tree order, then round again + self.assertEqual(seen, nav + [nav[0]]) + + def test_the_walk_wraps_back_to_the_first_file(self): + nav = [r for r in self.win._tree_rels() if self.win._is_nav(r)] + self.win._reselect(nav[-1]) + self._settle_ui() + for _ in range(20): + self.win._next_change() + self._settle_ui() + if self.win._selected_rel() == nav[0]: + return + self.fail('F8 never came back round to {}'.format(nav[0])) + + def test_prev_change_steps_back_a_file_at_its_last_change(self): + nav = [r for r in self.win._tree_rels() if self.win._is_nav(r)] + target = 'src/rename_conflict.c' # the fixture file with two changes + self.assertIn(target, nav) + # the file after it, wrapping: rename_conflict.c is the last nav file + # in this fixture, so this also covers stepping back over the wrap + after = nav[(nav.index(target) + 1) % len(nav)] + self.win._reselect(after) + self._settle_ui() + self.win._prev_change() + self._settle_ui() + self.assertEqual(self.win._selected_rel(), target) + self.assertEqual(self.win.diff._cur_idx, len(self.win.diff._stops) - 1) + + def test_hide_identical_drops_only_identical_rows(self): + before = self.win._tree_rels() + identical = [r for r in before + if self.win.results[r]['status'] == 'identical'] + self.assertTrue(identical, 'fixture has no identical file to hide') + self.win.cb_hide_identical.setChecked(True) + self._settle_ui() + after = self.win._tree_rels() + self.assertEqual(after, [r for r in before if r not in identical]) + self.win.cb_hide_identical.setChecked(False) + self._settle_ui() + self.assertEqual(self.win._tree_rels(), before) + + def test_hide_identical_keeps_the_file_on_screen(self): + self.win._reselect('src/real_change.c') + self._settle_ui() + self.win.cb_hide_identical.setChecked(True) + self._settle_ui() + self.assertEqual(self.win._selected_rel(), 'src/real_change.c') + + def test_hiding_rows_never_changes_a_verdict_or_the_counts(self): + # the filter is display-only: the record the export is built from must + # not notice it at all + before = {rel: r['status'] for rel, r in self.win.results.items()} + raw = dict(self.win._raw_results) + self.win.cb_hide_identical.setChecked(True) + self._settle_ui() + self.assertEqual({rel: r['status'] for rel, r in self.win.results.items()}, + before) + self.assertEqual(set(self.win._raw_results), set(raw)) + + if __name__ == '__main__': unittest.main() diff --git a/tests/test_failsafe.py b/tests/test_failsafe.py index 326f40d..1b949e2 100644 --- a/tests/test_failsafe.py +++ b/tests/test_failsafe.py @@ -201,5 +201,54 @@ def test_stale_arxml_report_replaced_with_no_changes_page(self): self.assertEqual(rc, 0) +class TestReportNotWritten(_TreeCase): + """A run whose report could not be written left no record of itself. + + Exit 1 is the gate's ordinary "real changes found"; a crash that returned 1 + was indistinguishable from it, and the traceback that came with it told the + reviewer nothing they could act on. + """ + + def _run(self, out): + out_buf, err_buf = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(out_buf), contextlib.redirect_stderr(err_buf): + rc = main([str(self.old), str(self.new), '--report', str(out)]) + return rc, out_buf.getvalue(), err_buf.getvalue() + + def test_a_missing_output_folder_is_exit_2_not_a_traceback(self): + out = Path(self.tmp.name) / 'nodir' / 'r.html' + rc, stdout, stderr = self._run(out) + self.assertEqual(rc, 2) + self.assertIn('REPORT NOT WRITTEN', stderr) + self.assertIn(str(out), stderr) + self.assertIn('does not exist', stderr) + self.assertNotIn('Traceback', stderr) + + def test_the_scan_it_could_not_write_is_still_reported(self): + # the compare itself succeeded; throwing its result away as well would + # cost the reviewer the one thing the run did produce + rc, stdout, _err = self._run(Path(self.tmp.name) / 'nodir' / 'r.html') + self.assertEqual(rc, 2) + self.assertIn('Summary:', stdout) + self.assertIn('MODIFIED a.c', stdout) + + def test_an_unwritable_path_fails_before_the_scan_runs(self): + # a directory where the report should go: the leftover-report unlink + # hits it first, so this covers the pre-scan branch + out = Path(self.tmp.name) / 'busy' + out.mkdir() + rc, _stdout, stderr = self._run(out) + self.assertEqual(rc, 2) + self.assertIn('REPORT NOT WRITTEN', stderr) + self.assertIn('INCOMPLETE', stderr) + + def test_exit_zero_cannot_mask_a_missing_report(self): + out_buf, err_buf = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(out_buf), contextlib.redirect_stderr(err_buf): + rc = main([str(self.old), str(self.new), '--exit-zero', + '--report', str(Path(self.tmp.name) / 'nodir' / 'r.html')]) + self.assertEqual(rc, 2) + + if __name__ == '__main__': unittest.main() diff --git a/tests/test_qtviewer.py b/tests/test_qtviewer.py index 185453b..473e7ef 100644 --- a/tests/test_qtviewer.py +++ b/tests/test_qtviewer.py @@ -96,6 +96,32 @@ def test_text_filter_drops_folders_without_a_match(self): kept = filter_nodes(nodes, text='x.c') self.assertEqual([n.name for n in kept], ['a']) + def test_hide_identical_drops_only_identical_files(self): + mapping = {'a.c': 'identical', 'b.c': 'real-change', + 'noise/c.c': 'ignorable-only', 'noise/d.c': 'comment-only', + 'x/added.c': 'added', 'x/gone.c': 'deleted', 'bad.c': 'error'} + kept = filter_nodes(self._nodes(mapping), hide_identical=True) + self.assertEqual(sorted(self._rels(kept)), + sorted(r for r in mapping if mapping[r] != 'identical')) + + def test_hide_identical_collapses_a_folder_with_nothing_left(self): + nodes = self._nodes({'quiet/a.c': 'identical', 'quiet/b.c': 'identical', + 'src/c.c': 'real-change'}) + kept = filter_nodes(nodes, hide_identical=True) + self.assertEqual([n.name for n in kept], ['src']) + + def test_hide_identical_and_the_text_filter_compose(self): + nodes = self._nodes({'src/ctrl.c': 'identical', 'src/ctrl_b.c': 'real-change', + 'other/ctrl.c': 'real-change'}) + kept = filter_nodes(nodes, text='src/', hide_identical=True) + self.assertEqual(self._rels(kept), ['src/ctrl_b.c']) + + def test_hide_identical_is_off_by_default(self): + # a verdict removing a row is opt-in: the folder structure has to stay + # stable unless the reviewer asked for it + nodes = self._nodes({'a.c': 'identical'}) + self.assertEqual(self._rels(filter_nodes(nodes)), ['a.c']) + class TestReviewState(unittest.TestCase): """The colour behind the tree's Review column. 'done' is a claim that From 5abdf5004f8aa7b5f4316ae9b144579f4a517185 Mon Sep 17 00:00:00 2001 From: longvo920 Date: Wed, 29 Jul 2026 07:48:00 +0700 Subject: [PATCH 2/2] fix: clear find highlights when the query stops matching The search marks and the current-change overlay were sharing one extraSelections list per editor, and only the "found something" path ever rewrote it. Typing on past the last hit ("begin" -> "beginal") left the old word lit while the bar said "no match" -- the pane contradicting its own answer, on a tool whose whole claim is that what you see is what it found. The two overlays are separate layers now, composited on every repaint, so either can be rebuilt without wiping the other. Every path out of the find box repaints: no query, no hits, bar closed, file changed. While the layer existed, marking only the current hit was leaving "3 of 8" unverifiable. All occurrences are marked now, the current one brighter. --- compare_tool/qtviewer/diffpane.py | 117 ++++++++++++++++++++---------- tests/test_diffpane_qt.py | 59 +++++++++++++++ 2 files changed, 136 insertions(+), 40 deletions(-) diff --git a/compare_tool/qtviewer/diffpane.py b/compare_tool/qtviewer/diffpane.py index 368d219..7782a18 100644 --- a/compare_tool/qtviewer/diffpane.py +++ b/compare_tool/qtviewer/diffpane.py @@ -104,10 +104,13 @@ def _semantic_summary(result): # F7/F8 are visibly doing something even when the file fits on screen and # there is nothing to scroll _CUR_BG = QColor(255, 255, 255, 34) -# the find hit itself. Amber on purpose: red, green and blue already mean -# removed, added and moved, so a fourth hue is the only way a search result can -# be told apart from a verdict about the code. -_FIND_BG = QColor('#7a6320') +# the find hits. Amber on purpose: red, green and blue already mean removed, +# added and moved, so a fourth hue is the only way a search result can be told +# apart from a verdict about the code. Every occurrence is marked, the one the +# counter is pointing at brighter -- "3 of 8" is only useful if the other seven +# are visible too. +_FIND_BG = QColor('#5a4715') +_FIND_CUR_BG = QColor('#8f7220') # OLD/NEW pane-banner accents: one source, used for both the tag text and the # underline so the two can never drift apart _OLD_ACCENT = '#c98b8b' @@ -345,6 +348,10 @@ def __init__(self): self._drive = self.old_edit self._hits = [] # rows matching the find box, in file order self._hit_idx = -1 + # the two extraSelections layers, per editor (old, new): see + # _paint_selections + self._sel_rows = [[], []] + self._sel_match = [[], []] self._link_scrolls() # Ctrl+F is where every editor puts find; the pane owns the shortcut so # it works wherever the focus sits inside the diff @@ -434,6 +441,7 @@ def close_find(self): self._find_bar.setVisible(False) self._hits = [] self._hit_idx = -1 + self._mark_matches() # a closed find bar leaves nothing lit behind it # the focus goes back to the diff, or the next keystroke would vanish # into a hidden box self._drive.setFocus() @@ -451,24 +459,29 @@ def _refresh_find(self): self._hits = [] self._hit_idx = -1 if not self._find_bar.isVisible(): + self._mark_matches() return text = self._find_edit.text() if not text.strip(): self._find_count.setText('') + self._mark_matches() return self._hits = self.find_matches(text) self._find_count.setText('{} match{}'.format(len(self._hits), '' if len(self._hits) == 1 else 'es') if self._hits else 'no match') + self._mark_matches() def _find_changed(self, text): self._hits = self.find_matches(text) self._hit_idx = -1 if not text.strip(): self._find_count.setText('') + self._mark_matches() # every path repaints: see _mark_matches return if not self._hits: self._find_count.setText('no match') + self._mark_matches() return self.find_next() @@ -489,36 +502,42 @@ def _goto_hit(self, idx): self._hit_idx = idx % len(self._hits) self._find_count.setText('{} of {}'.format(self._hit_idx + 1, len(self._hits))) - row = self._hits[self._hit_idx] - self._reveal(row) - self._mark_match(row, self._find_edit.text()) - - def _mark_match(self, row, text): - """Paint the matched text itself, over the row overlay ``_reveal`` just - laid down. Without it a hit in a long line is 'the pane scrolled a bit' - and the reviewer still has to read the line to find the word.""" - needle = text.strip().lower() - if not needle: - return - for editor in (self.old_edit, self.new_edit): - doc = editor.document() - if doc.blockCount() <= row: - continue - block = doc.findBlockByNumber(row) - line = block.text().lower() - sels = list(editor.extraSelections()) - at = line.find(needle) - while at >= 0: - sel = QTextEdit.ExtraSelection() - sel.format.setBackground(_FIND_BG) - cur = QTextCursor(block) - cur.setPosition(block.position() + at) - cur.setPosition(block.position() + at + len(needle), - QTextCursor.KeepAnchor) - sel.cursor = cur - sels.append(sel) # after the row overlay, so it paints on top - at = line.find(needle, at + len(needle)) - editor.setExtraSelections(sels) + self._reveal(self._hits[self._hit_idx]) + self._mark_matches() + + def _mark_matches(self): + """Repaint the search layer from the CURRENT hits: every occurrence in + amber, the one being stepped through brighter. + + Rebuilt from scratch on every call, and every path through the find box + calls it -- a query that stops matching (typing on past the last hit) + has to take its highlights with it, or the pane says 'no match' while + the old word is still lit, which reads as a wrong answer. + """ + self._sel_match = [[], []] + needle = self._find_edit.text().strip().lower() + cur = self._hits[self._hit_idx] if 0 <= self._hit_idx < len(self._hits) else None + if needle and self._hits: + for i, editor in enumerate((self.old_edit, self.new_edit)): + doc = editor.document() + for row in self._hits: + if doc.blockCount() <= row: + continue # the other side of a one-sided file + block = doc.findBlockByNumber(row) + line = block.text().lower() + colour = _FIND_CUR_BG if row == cur else _FIND_BG + at = line.find(needle) + while at >= 0: + sel = QTextEdit.ExtraSelection() + sel.format.setBackground(colour) + c = QTextCursor(block) + c.setPosition(block.position() + at) + c.setPosition(block.position() + at + len(needle), + QTextCursor.KeepAnchor) + sel.cursor = c + self._sel_match[i].append(sel) + at = line.find(needle, at + len(needle)) + self._paint_selections() def find_next(self): if not self._hits: @@ -603,6 +622,7 @@ def clear(self): self._find_bar.setVisible(False) # no file: nothing to search self._hits = [] self._hit_idx = -1 + self._clear_selections() self.setCurrentIndex(0) def _forget_units(self): @@ -671,6 +691,7 @@ def current_unit(self): def _message(self, text): self.rows = [] self._stops = [] + self._clear_selections() self.minimap.set_rows([]) self._pos_text = '' self._logo.setVisible(False) @@ -796,8 +817,7 @@ def _load_rows(self, rel, status, result=None, row_map=None): self._reveal(self._stops[0]) else: self._pos_text = '' - self.old_edit.setExtraSelections([]) - self.new_edit.setExtraSelections([]) + self._clear_selections() self.old_edit.verticalScrollBar().setValue(0) def _load_one_side(self, rel, label, lines, side): @@ -809,6 +829,7 @@ def _load_one_side(self, rel, label, lines, side): for i, line in enumerate(lines)] self._stops = [] self._pos_text = '' + self._clear_selections() # nothing of the previous file may survive self._sem.setVisible(False) # keep _head_base in step with the shown header (an added/deleted file # has no change stops, but leaving a stale base from the previous file @@ -897,6 +918,22 @@ def _reveal(self, row, context=3): self._update_position(row) self.unitChanged.emit() + # Two overlays share one extraSelections list per editor: the block the + # reviewer is on, and the search hits. They are kept apart so either can be + # rebuilt without wiping the other -- one list meant a cleared search took + # the current-change overlay with it, and a search that stopped matching + # left its old highlights behind. + + def _paint_selections(self): + for i, editor in enumerate((self.old_edit, self.new_edit)): + # matches last: they paint on top of the row overlay + editor.setExtraSelections(self._sel_rows[i] + self._sel_match[i]) + + def _clear_selections(self): + self._sel_rows = [[], []] + self._sel_match = [[], []] + self._paint_selections() + def _highlight_block(self, row): """Overlay the whole contiguous change block containing `row`.""" rows = self.rows @@ -907,12 +944,12 @@ def _highlight_block(self, row): start -= 1 while end + 1 < len(rows) and rows[end + 1].mode == rows[row].mode != 'ctx': end += 1 - for editor in (self.old_edit, self.new_edit): + self._sel_rows = [[], []] + for k, editor in enumerate((self.old_edit, self.new_edit)): # a one-sided file leaves the other document empty: selecting a # block it does not have would be a null cursor if editor.document().blockCount() <= end: continue - sels = [] for i in range(start, end + 1): sel = QTextEdit.ExtraSelection() sel.format.setBackground(_CUR_BG) @@ -920,8 +957,8 @@ def _highlight_block(self, row): cur = QTextCursor(editor.document().findBlockByNumber(i)) cur.clearSelection() sel.cursor = cur - sels.append(sel) - editor.setExtraSelections(sels) + self._sel_rows[k].append(sel) + self._paint_selections() def _update_position(self, row): if not self._stops: diff --git a/tests/test_diffpane_qt.py b/tests/test_diffpane_qt.py index 46fbed3..6742790 100644 --- a/tests/test_diffpane_qt.py +++ b/tests/test_diffpane_qt.py @@ -280,6 +280,65 @@ def test_stepping_wraps_and_reports_position(self): self.pane.find_prev() self.assertEqual(self.pane._find_count.text(), '{} of {}'.format(n, n)) + def _match_marks(self): + return sum(len(s) for s in self.pane._sel_match) + + def _type(self, text): + self.pane._find_edit.setText(text) + for _ in range(5): + self.app.processEvents() + + def test_a_query_that_stops_matching_takes_its_highlights_with_it(self): + # typing on past the last hit ("begin" -> "beginal") said "no match" + # while the old word stayed lit, which reads as the wrong answer + self._open('a2l/comment_only.a2l') + self.pane.open_find() + self._type('begin') + self.assertGreater(self._match_marks(), 0) + self._type('beginal') + self.assertEqual(self.pane._find_count.text(), 'no match') + self.assertEqual(self._match_marks(), 0) + + def test_clearing_the_box_clears_the_marks(self): + self._open('a2l/comment_only.a2l') + self.pane.open_find() + self._type('begin') + self._type('') + self.assertEqual(self._match_marks(), 0) + self.assertEqual(self.pane._find_count.text(), '') + + def test_closing_the_bar_clears_the_marks(self): + self._open('a2l/comment_only.a2l') + self.pane.open_find() + self._type('begin') + self.pane.close_find() + for _ in range(5): + self.app.processEvents() + self.assertEqual(self._match_marks(), 0) + + def test_every_occurrence_is_marked_not_only_the_current_one(self): + self._open('a2l/comment_only.a2l') + self.pane.open_find() + self._type('begin') + # one mark per occurrence per pane that has the row + self.assertGreaterEqual(self._match_marks(), len(self.pane._hits)) + + def test_the_current_change_overlay_survives_a_cleared_search(self): + # the two overlays share one selection list; clearing the search used + # to wipe the block the reviewer is standing on as well + self._open('src/rename_conflict.c') + self.pane.open_find() + self._type('rtb_') + self._type('') + self.assertGreater(sum(len(s) for s in self.pane._sel_rows), 0) + + def test_leaving_a_file_takes_its_marks_with_it(self): + self._open('src/rename_conflict.c') + self.pane.open_find() + self._type('rtb_') + self._open('src/real_change.c') # no rtb_ in this one + self.assertEqual(self._match_marks(), 0) + def test_the_query_survives_a_file_change_without_moving_the_pane(self): self._open('src/rename_conflict.c') self.pane.open_find()