From b40da3944f29df8011278bfcf3ed9f4c831527bd Mon Sep 17 00:00:00 2001 From: nelsonduarte Date: Tue, 4 Aug 2026 19:27:06 +0100 Subject: [PATCH] feat(editor): commit inline edits on focus-out and default to Text mode Two low-risk UX quick-wins for the visual PDF editor (issue #147). 1. Clicking outside an inline text edit now CONFIRMS it (Word-style) instead of silently discarding what the user typed. The canvas FocusOut handler calls _commit_inline() instead of _cancel_inline(); switching editor modes mid-edit likewise commits. _commit_inline's existing guards keep this safe: it resets its own state before emitting (so the focus-out that hide() triggers early-returns and cannot double-commit) and drops unchanged edits and empty inserts, so no spurious pending edit is created. Escape still discards. 2. The editor now STARTS in Text mode (the most useful, least destructive tool) instead of Redact, which was the default only because it was the first button in the grid. The initial mode is activated through _on_mode_btn() so the button styling, options page, canvas text-mode/IBeam cursor and hint stay in sync with a real click. The Text-mode discovery hint is made prominent (accent colour, bold, wrapped) so users immediately learn that a single click on any text starts editing it. Adds tests/test_editor_inline_ux.py (10 discriminative tests: focus-out commit/cancel, unchanged/empty no-op, no double-commit, mode-switch commit, default Text mode, prominent hint) and updates the r9 audit guard to assert the new commit-on-mode-change behaviour. Co-Authored-By: Claude Opus 4.8 --- app/editor/canvas.py | 13 +- app/editor/tab.py | 44 +++++-- tests/test_editor_audit_r9.py | 12 +- tests/test_editor_inline_ux.py | 226 +++++++++++++++++++++++++++++++++ 4 files changed, 276 insertions(+), 19 deletions(-) create mode 100644 tests/test_editor_inline_ux.py diff --git a/app/editor/canvas.py b/app/editor/canvas.py index 9348230..38971eb 100644 --- a/app/editor/canvas.py +++ b/app/editor/canvas.py @@ -523,9 +523,16 @@ def eventFilter(self, obj, event): self._commit_inline() return True elif event.type() == QEvent.Type.FocusOut: - # Clicking outside cancels the edit (matches VSCode/most - # editors). Enter/Tab still commit; Escape still cancels. - self._cancel_inline() + # Clicking outside (or Alt-Tab / opening a dialog) COMMITS + # the edit, Word-style — see #147. Enter/Tab already commit; + # Escape still discards. _commit_inline's guards keep this + # safe: it resets its own state and early-returns once + # ``_inline_mode is None`` (so the focus-out that ``hide()`` + # itself may trigger cannot double-commit), and it skips + # unchanged edits (``new_text == original``) and empty + # inserts (``not new_text.strip()``), so no spurious pending + # edit is ever created. + self._commit_inline() return False return super().eventFilter(obj, event) diff --git a/app/editor/tab.py b/app/editor/tab.py index 8446ba2..8b4afae 100644 --- a/app/editor/tab.py +++ b/app/editor/tab.py @@ -367,6 +367,7 @@ def _reinsert_edited_text(fitz, doc, page, edit, warn_fn=None): # Mode indices in `_mode_btns` — kept as constants for readability so # call-sites like `if self._mode_idx == _MODE_FORMS:` document intent # without forcing a refactor of the existing numeric layout. +_MODE_TEXT = 1 _MODE_IMAGE = 2 _MODE_FORMS = 5 _MODE_SIGNATURE = 6 @@ -450,7 +451,7 @@ def __init__(self, status_fn): self._redo_stack = [] self._doc_path = None self._pdf_password = "" - self._mode_idx = 0 + self._mode_idx = _MODE_TEXT self._dark_mode = True self.setObjectName("content_area") @@ -538,11 +539,10 @@ def __init__(self, status_fn): btn.clicked.connect(lambda checked, b=btn: self._on_mode_btn(b)) self._mode_btns.append(btn) gm.addWidget(btn, i // cols, i % cols) - self._mode_btns[0].setChecked(True) - self._mode_btns[0].setIcon(qta.icon(self._MODE_DEFS[0][1], color=ACCENT)) - self._mode_btns[0].setStyleSheet( - f"background:#0D3D38; border:1px solid {ACCENT}; " - f"border-radius:6px;") + # The initial active mode (Text — see the __init__ tail) is applied + # once every widget exists, by calling _on_mode_btn(), so the button + # styling, options page, canvas mode/cursor and hint all match a real + # user click instead of being hand-rolled here. cv.addWidget(grp_mode) # -- Options per mode -- @@ -578,10 +578,16 @@ def __init__(self, status_fn): self._text_color = ColorPickerButton((0, 0, 0)) row1.addWidget(self._text_color); row1.addStretch() v1.addLayout(row1) - hint1 = QLabel(t("edit.hint.text")) - hint1.setStyleSheet(f"color:{TEXT_SEC}; font-size:11px;") - self._hint_labels.append(hint1) - v1.addWidget(hint1); v1.addStretch() + # Text is the default mode (#147); make its discovery hint stand out + # (accent colour, bold, wrapped) so users immediately learn a single + # click on any text starts editing it. ACCENT is theme-independent, so + # this label is deliberately kept OUT of self._hint_labels — whose grey + # gets re-flattened on every theme toggle by update_theme(). + self._text_hint = QLabel(t("edit.hint.text")) + self._text_hint.setWordWrap(True) + self._text_hint.setStyleSheet( + f"color:{ACCENT}; font-size:12px; font-weight:bold;") + v1.addWidget(self._text_hint); v1.addStretch() self._opt_stack.addWidget(w1) # 2 - Image @@ -763,6 +769,13 @@ def __init__(self, status_fn): QShortcut(QKeySequence("Ctrl+Y"), self, self._redo) QShortcut(QKeySequence("Ctrl+Shift+Z"), self, self._redo) + # Start in Text mode (#147): the most useful and least destructive + # default (Redact used to be the default only because it was the first + # button). Routing through _on_mode_btn keeps the button styling, the + # options page, the canvas text-mode/IBeam cursor and the prominent + # hint fully in sync with a real user click. + self._on_mode_btn(self._mode_btns[_MODE_TEXT]) + self._update_nav() def paintEvent(self, event): @@ -895,10 +908,15 @@ def _on_mode_btn(self, btn): else: self._btn_undo.setToolTip(t("edit.undo_tip")) self._btn_redo.setToolTip(t("edit.redo_tip")) - # Commit/cancel any inline-edit-in-progress before changing - # modes — otherwise the text the user typed lands in limbo. + # Commit any inline-edit-in-progress before changing modes — + # consistent with "clicking outside confirms" (#147). In the real GUI + # the mode button steals focus first, so the focus-out already commits + # and this call is a no-op (early-returns on the hidden editor); when + # _on_mode_btn is invoked without a focus change it commits here. Either + # way _commit_inline resets its own state before emitting, so the edit + # is committed exactly once (never commit+cancel nor double commit). if hasattr(self, "_canvas") and self._canvas._inline_edit.isVisible(): - self._canvas._cancel_inline() + self._canvas._commit_inline() self._canvas.set_select_mode(idx == 8) is_draw = (idx == 7) self._canvas.set_draw_mode( diff --git a/tests/test_editor_audit_r9.py b/tests/test_editor_audit_r9.py index a7f3841..143d1c7 100644 --- a/tests/test_editor_audit_r9.py +++ b/tests/test_editor_audit_r9.py @@ -219,14 +219,20 @@ def test_forms_undo_message_in_run(): assert "self._btn_undo.setToolTip(tip)" in src -# ── #9 — Mode change cancels inline edit ───────────────────────────────── +# ── #9 — Mode change commits the inline edit (#147) ────────────────────── -def test_mode_change_cancels_inline_edit(): +def test_mode_change_commits_inline_edit(): + """Switching modes mid-edit must COMMIT the in-progress inline edit + (Word-style, #147) rather than discard it — the guarded call keeps the + text the user typed from landing in limbo when a different tool is + picked. Originally this cancelled; it now commits for consistency with + "clicking outside confirms".""" src = _read("app/editor/tab.py") block = src[src.find("def _on_mode_btn"): src.find("def _pick_pdf")] - assert "self._canvas._cancel_inline()" in block + assert "self._canvas._commit_inline()" in block + assert "self._canvas._cancel_inline()" not in block assert "_inline_edit.isVisible()" in block diff --git a/tests/test_editor_inline_ux.py b/tests/test_editor_inline_ux.py new file mode 100644 index 0000000..4e755cf --- /dev/null +++ b/tests/test_editor_inline_ux.py @@ -0,0 +1,226 @@ +"""Editor inline-edit UX quick-wins (issue #147). + +Two low-risk discoverability/consistency wins are covered here: + + 1. Clicking OUTSIDE an inline text edit now CONFIRMS it (Word-style), + instead of silently discarding what the user typed. Enter/Tab still + commit; Escape still discards. Switching editor modes mid-edit + likewise commits (once) rather than throwing the edit away. + + 2. The editor now STARTS in Text mode (the most useful, least + destructive action) with a prominent hint, so users discover the + click-to-edit interaction instead of landing in Redact. + +All tests run headless with ``QT_QPA_PLATFORM=offscreen``. +""" + +import os +import sys +from pathlib import Path + +os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") + +import pytest # noqa: E402 + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +fitz = pytest.importorskip("pymupdf") + +from PySide6.QtCore import Qt, QEvent # noqa: E402 +from PySide6.QtGui import QFocusEvent, QKeyEvent # noqa: E402 +from PySide6.QtWidgets import QApplication # noqa: E402 + + +# A span dict shaped exactly like the ones the canvas passes to +# begin_inline_text_edit (text + bbox + style metadata). +_SPAN = { + "text": "Original", "bbox": [10.0, 20.0, 110.0, 40.0], + "size": 12.0, "color": 0, "font": "Helvetica", "flags": 0, +} + + +@pytest.fixture(scope="module") +def _app(): + return QApplication.instance() or QApplication([]) + + +def _make_canvas(_app): + from app.editor.canvas import PdfEditCanvas + c = PdfEditCanvas() + c.show() # child _inline_edit is only isVisible() once an ancestor is + return c + + +def _focus_out(widget): + """Deliver a real FocusOut so it routes through the canvas eventFilter.""" + QApplication.sendEvent(widget, QFocusEvent(QEvent.Type.FocusOut)) + + +def _press_escape(widget): + ev = QKeyEvent(QEvent.Type.KeyPress, Qt.Key.Key_Escape, + Qt.KeyboardModifier.NoModifier) + QApplication.sendEvent(widget, ev) + + +# ── PEDIDO 1: click-outside / focus-out confirms ───────────────────────── + +def test_focus_out_commits_changed_edit(_app): + """Editing a span then clicking away COMMITS the change (was: discard).""" + c = _make_canvas(_app) + committed = [] + c.text_edit_committed.connect(lambda p, e: committed.append(e)) + c.begin_inline_text_edit(_SPAN, 0) + assert c._inline_edit.isVisible(), "precondition: inline editor shown" + c._inline_edit.setText("Modified") + + _focus_out(c._inline_edit) + + assert len(committed) == 1, "focus-out must commit the edit exactly once" + assert committed[0]["new_text"] == "Modified" + assert committed[0]["type"] == "text_edit" + # State fully reset so the hidden editor can't double-commit. + assert c._inline_mode is None + assert not c._inline_edit.isVisible() + + +def test_focus_out_unchanged_edit_creates_no_pending(_app): + """A focus-out with no text change must NOT emit a spurious edit.""" + c = _make_canvas(_app) + committed = [] + c.text_edit_committed.connect(lambda p, e: committed.append(e)) + c.begin_inline_text_edit(_SPAN, 0) # text left == original + + _focus_out(c._inline_edit) + + assert committed == [], "unchanged edit must not create a pending edit" + assert c._inline_mode is None # still tidied up / hidden + assert not c._inline_edit.isVisible() + + +def test_escape_still_discards_edit(_app): + """Escape keeps its cancel semantics even though focus-out now commits.""" + c = _make_canvas(_app) + committed = [] + c.text_edit_committed.connect(lambda p, e: committed.append(e)) + c.begin_inline_text_edit(_SPAN, 0) + c._inline_edit.setText("Modified") + + _press_escape(c._inline_edit) + + assert committed == [], "Escape must discard, not commit" + assert c._inline_mode is None + assert not c._inline_edit.isVisible() + + +def test_focus_out_empty_insert_creates_no_pending(_app): + """Focus-out on an empty INSERT must not insert whitespace-only text.""" + c = _make_canvas(_app) + inserted = [] + c.text_inserted.connect(lambda p, e: inserted.append(e)) + c.begin_inline_text_insert(0, fitz.Point(30, 40), 12.0, (0, 0, 0), + "Helvetica") + + _focus_out(c._inline_edit) + + assert inserted == [], "empty insert must not create a pending edit" + assert c._inline_mode is None + + +def test_focus_out_commits_nonempty_insert(_app): + """Focus-out on a non-empty INSERT commits the new text exactly once.""" + c = _make_canvas(_app) + inserted = [] + c.text_inserted.connect(lambda p, e: inserted.append(e)) + c.begin_inline_text_insert(0, fitz.Point(30, 40), 12.0, (0, 0, 0), + "Helvetica") + c._inline_edit.setText("New text") + + _focus_out(c._inline_edit) + + assert len(inserted) == 1, "insert must commit exactly once on focus-out" + assert inserted[0]["text"] == "New text" + assert c._inline_mode is None + + +def test_focus_out_does_not_double_commit(_app): + """The focus-out that hide() itself may trigger must not commit twice. + + _commit_inline resets ``_inline_mode`` to None *before* emitting, so any + re-entrant focus-out early-returns. A second manual focus-out is a no-op. + """ + c = _make_canvas(_app) + committed = [] + c.text_edit_committed.connect(lambda p, e: committed.append(e)) + c.begin_inline_text_edit(_SPAN, 0) + c._inline_edit.setText("Modified") + + _focus_out(c._inline_edit) + _focus_out(c._inline_edit) # extra focus-out on the now-hidden editor + + assert len(committed) == 1, "edit must be committed once, never twice" + + +# ── PEDIDO 1: switching modes mid-edit commits (once) ──────────────────── + +def test_mode_switch_commits_edit_once(_app): + """Changing editor mode while editing COMMITS the in-progress edit.""" + from app.editor.tab import TabEditar, _MODE_TEXT + tab = TabEditar(lambda *a, **k: None) + tab.show() + c = tab._canvas + c.begin_inline_text_edit(_SPAN, 0) + c._inline_edit.setText("Modified") + before = len(tab._pending) + + # Switch away from Text (default) to Redact (index 0). + tab._on_mode_btn(tab._mode_btns[0]) + + assert len(tab._pending) == before + 1, "mode switch must commit the edit" + assert tab._pending[-1]["new_text"] == "Modified" + assert c._inline_mode is None + assert tab._mode_idx == 0 # the new mode really took effect + assert _MODE_TEXT == 1 # sanity on the constant used elsewhere + + +def test_mode_switch_unchanged_edit_no_spurious_pending(_app): + """Switching modes with an untouched edit must not add a pending edit.""" + from app.editor.tab import TabEditar + tab = TabEditar(lambda *a, **k: None) + tab.show() + c = tab._canvas + c.begin_inline_text_edit(_SPAN, 0) # unchanged text + before = len(tab._pending) + + tab._on_mode_btn(tab._mode_btns[0]) + + assert len(tab._pending) == before, "no spurious pending on unchanged edit" + assert c._inline_mode is None + + +# ── PEDIDO 4: default Text mode + prominent hint ───────────────────────── + +def test_editor_starts_in_text_mode(_app): + """The editor boots into Text mode, not Redact (#147 discoverability).""" + from app.editor.tab import TabEditar, _MODE_TEXT + tab = TabEditar(lambda *a, **k: None) + + assert tab._mode_idx == _MODE_TEXT + checked = [i for i, b in enumerate(tab._mode_btns) if b.isChecked()] + assert checked == [_MODE_TEXT], "exactly the Text button is active" + # The whole mode is wired, not just the button: options page, canvas + # text-mode flag all follow. + assert tab._opt_stack.currentIndex() == _MODE_TEXT + assert tab._canvas._text_mode is True + + +def test_text_mode_hint_is_prominent(_app): + """The Text-mode hint is shown and visually prominent (accent + bold).""" + from app.i18n import t + from app.constants import ACCENT + from app.editor.tab import TabEditar + tab = TabEditar(lambda *a, **k: None) + + assert tab._text_hint.text() == t("edit.hint.text") + style = tab._text_hint.styleSheet().lower() + assert ACCENT.lower() in style, "hint should use the accent colour" + assert "bold" in style, "hint should be bold to stand out"