diff --git a/.gitignore b/.gitignore index 3c674bd..5153297 100644 --- a/.gitignore +++ b/.gitignore @@ -35,7 +35,9 @@ pyinstaller.log listingData-*.csv # IDE -.vscode/ +.vscode/* +!.vscode/settings.json +!.vscode/extensions.json .idea/ *.swp *.swo diff --git a/.vscode/extensions.json b/.vscode/extensions.json new file mode 100644 index 0000000..aec92ea --- /dev/null +++ b/.vscode/extensions.json @@ -0,0 +1,7 @@ +{ + "recommendations": [ + "ms-python.python", + "ms-python.vscode-pylance", + "ms-python.debugpy" + ] +} diff --git a/.vscode/settings.json b/.vscode/settings.json index c9ebf2d..532638e 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,3 +1,13 @@ { - "python-envs.defaultEnvManager": "ms-python.python:system" -} \ No newline at end of file + "python-envs.defaultEnvManager": "ms-python.python:system", + "python.defaultInterpreterPath": "${workspaceFolder}/venv/Scripts/python.exe", + "python.analysis.extraPaths": ["${workspaceFolder}"], + "python.terminal.activateEnvironment": true, + "python.analysis.typeCheckingMode": "basic", + "python.analysis.autoImportCompletions": true, + "files.exclude": { + "**/__pycache__": true, + "**/*.pyc": true + }, + "python.testing.pytestEnabled": false +} diff --git a/app/viewer/annotation_hud.py b/app/viewer/annotation_hud.py index b4aa848..a111506 100644 --- a/app/viewer/annotation_hud.py +++ b/app/viewer/annotation_hud.py @@ -1,7 +1,7 @@ """PDFApps – Floating HUD toolbar for presentation-mode annotations.""" from PySide6.QtCore import Qt, Signal, QSize -from PySide6.QtGui import QColor +from PySide6.QtGui import QColor, QIcon, QTransform from PySide6.QtWidgets import QWidget, QHBoxLayout, QPushButton, QFrame import qtawesome as qta @@ -41,6 +41,45 @@ def _rgba(hex_color: str, alpha: int) -> str: (ToolMode.LASER, "fa5s.dot-circle", "present.laser"), ] +# FontAwesome renders the pen and highlighter with their writing tips at the +# bottom-right, which visually clashes with the mouse-pointer icon whose tip +# points up-left (standard cursor arrow convention). We apply a +90° clockwise +# rotation via QTransform (not qtawesome's ``rotated=`` kwarg) so the same +# quarter-turn behaviour survives the icon → pixmap conversion path used by +# the cursor helper in :mod:`app.viewer.annotation_layer`. That maps the +# FontAwesome bottom-right tip to the top-left corner — matching the +# mouse-pointer and making every "pointing" icon in the HUD row read as +# pointing the same way. (Pixel analysis confirms 90° — not 180° — is the +# quarter turn that maps bottom-right → top-left.) +_ICON_ROTATION: dict[str, float] = { + "fa5s.pen": 90.0, + "fa5s.highlighter": 90.0, +} + + +def _tool_qta_icon(name: str, color: str) -> QIcon: + """Render a HUD toolbar icon with rotation but no outline. + + HUD buttons sit on the dark toolbar background, so the outline stroke + used by the cursor icons (see :func:`app.viewer.annotation_layer._icon_with_outline`) + is redundant here and would clutter the visual. Cursor icons still get + the outline because they float over arbitrary slide content. + + Rotation is applied via :class:`QTransform` on the rendered pixmap so + the transform is guaranteed to survive the icon → pixmap conversion + (qtawesome's ``rotated=`` kwarg does not always round-trip through + that path). + """ + rotation = _ICON_ROTATION.get(name, 0.0) + pix = qta.icon(name, color=color).pixmap(_ICON_PX, _ICON_PX) + if rotation: + transform = QTransform() + transform.rotate(rotation) + pix = pix.transformed( + transform, mode=Qt.TransformationMode.SmoothTransformation + ) + return QIcon(pix) + class AnnotationHUD(QFrame): """Bottom-centred floating toolbar. Sibling of `AnnotationOverlay`, @@ -209,7 +248,7 @@ def _refresh_icons(self) -> None: for b in list(self._tool_btns.values()) + [self._clear_btn]: name = b.property("_icon_name") if name: - b.setIcon(qta.icon(name, color=fg)) + b.setIcon(_tool_qta_icon(name, color=fg)) for s in self._swatches: hex_color = s.property("_swatch_color") self._style_swatch(s, hex_color, active=False) @@ -271,11 +310,11 @@ def _refresh_active(self) -> None: name = b.property("_icon_name") if name: col = ACCENT if is_active else fg - b.setIcon(qta.icon(name, color=col)) + b.setIcon(_tool_qta_icon(name, color=col)) self._clear_btn.setStyleSheet(inactive_qss) name = self._clear_btn.property("_icon_name") if name: - self._clear_btn.setIcon(qta.icon(name, color=fg)) + self._clear_btn.setIcon(_tool_qta_icon(name, color=fg)) active_hex = self._active_color.name().lower() for s in self._swatches: diff --git a/app/viewer/annotation_layer.py b/app/viewer/annotation_layer.py index daa7e4b..504173b 100644 --- a/app/viewer/annotation_layer.py +++ b/app/viewer/annotation_layer.py @@ -5,9 +5,11 @@ from PySide6.QtCore import Qt, QPoint from PySide6.QtGui import ( - QColor, QPainter, QPainterPath, QPainterPathStroker, QPen, + QColor, QCursor, QPainter, QPainterPath, QPainterPathStroker, QPen, + QPixmap, QTransform, ) from PySide6.QtWidgets import QWidget +import qtawesome as qta class ToolMode(IntEnum): @@ -32,6 +34,100 @@ class Stroke: _LASER_RADIUS = 14 _ERASER_TOL = 6 _POINT_MERGE_SQ = 4 +_CURSOR_ICON_PX = 24 +# 1 px black outline dilation on each side; keep symmetric so hotspot math +# stays predictable (the icon body lives at offset (_OUTLINE_PAD, _OUTLINE_PAD) +# inside the enlarged pixmap and the hotspot must add the same offset). +_OUTLINE_PAD = 2 +# Standard 8-direction dilation offsets used to fake a 1 px stroke around +# the icon glyph; composited by drawing the icon in the outline colour once +# per offset before overlaying the fill-coloured glyph on top. +_OUTLINE_OFFSETS = ( + (-1, 0), (1, 0), (0, -1), (0, 1), + (-1, -1), (1, 1), (-1, 1), (1, -1), +) + + +def _icon_with_outline( + icon_name: str, + fill_color: str, + rotation: float, + size: int, + outline_color: str = "#000000", +) -> QPixmap: + """Render a qtawesome icon with a black outline for visibility. + + Rotation is applied via :class:`QTransform` on the resulting pixmap + (not via qtawesome's ``rotated=`` kwarg) because qtawesome's icon + options do not always round-trip cleanly through the ``icon → pixmap`` + conversion path used for cursors — the QTransform approach is + Qt-native and works identically for cursors and toolbar icons. + + Output is padded by ``_OUTLINE_PAD`` px on every side so callers must + shift their hotspot by the same amount to keep it aligned with the + original glyph pixel. + """ + base_pix = qta.icon(icon_name, color=fill_color).pixmap(size, size) + outline_pix = qta.icon(icon_name, color=outline_color).pixmap(size, size) + + if rotation: + t = QTransform() + t.rotate(rotation) + mode = Qt.TransformationMode.SmoothTransformation + base_pix = base_pix.transformed(t, mode) + outline_pix = outline_pix.transformed(t, mode) + + outer = size + _OUTLINE_PAD * 2 + result = QPixmap(outer, outer) + result.fill(Qt.GlobalColor.transparent) + + painter = QPainter(result) + for dx, dy in _OUTLINE_OFFSETS: + painter.drawPixmap(_OUTLINE_PAD + dx, _OUTLINE_PAD + dy, outline_pix) + painter.drawPixmap(_OUTLINE_PAD, _OUTLINE_PAD, base_pix) + painter.end() + return result + + +def _cursor_for_tool(tool: "ToolMode", dark: bool = True) -> QCursor: + """Build a QCursor that mirrors the active HUD tool. + + The pen and highlighter share the +90° clockwise rotation used by + :mod:`app.viewer.annotation_hud` so the cursor tip visually matches the + icon shown on the toolbar. That quarter turn maps the FontAwesome native + bottom-right tip to the top-left corner, so hotspots are anchored to + ``(2, 2)`` (top-left) for pen/highlighter — matching the mouse-pointer + arrow convention — and to the centre for the eraser. LASER returns + ``BlankCursor`` because the overlay draws the red dot manually — showing + a system cursor on top would be visually noisy. + + Every rendered cursor also carries a 1 px black outline (via + :func:`_icon_with_outline`) so the glyph stays legible on light-coloured + slides regardless of theme. + """ + if tool == ToolMode.POINTER: + return QCursor(Qt.CursorShape.ArrowCursor) + if tool == ToolMode.LASER: + return QCursor(Qt.CursorShape.BlankCursor) + + icon_color = "#FFFFFF" if dark else "#000000" + size = _CURSOR_ICON_PX + + if tool == ToolMode.PEN: + pix = _icon_with_outline("fa5s.pen", icon_color, 90, size) + # Original hotspot was (2, 2); shift by outline pad so the tip still + # anchors on the same glyph pixel inside the enlarged pixmap. + return QCursor(pix, 2 + _OUTLINE_PAD, 2 + _OUTLINE_PAD) + if tool == ToolMode.HIGHLIGHTER: + pix = _icon_with_outline("fa5s.highlighter", icon_color, 90, size) + return QCursor(pix, 2 + _OUTLINE_PAD, 2 + _OUTLINE_PAD) + if tool == ToolMode.ERASER: + pix = _icon_with_outline("fa5s.eraser", icon_color, 0, size) + # Hotspot at glyph centre; enlarged pixmap is (size + 2*pad), + # centre is at (size + 2*pad) // 2. + c = (size + _OUTLINE_PAD * 2) // 2 + return QCursor(pix, c, c) + return QCursor(Qt.CursorShape.ArrowCursor) class AnnotationOverlay(QWidget): @@ -138,15 +234,13 @@ def set_laser_pos(self, pos: QPoint | None) -> None: # ── internals ───────────────────────────────────────────────────────── def _apply_cursor(self) -> None: - t = self._tool - if t == ToolMode.POINTER: + # POINTER hands the cursor back to the parent (the overlay is also + # transparent-to-mouse in that mode, so this call is mostly for + # correctness after switching away from another tool). + if self._tool == ToolMode.POINTER: self.unsetCursor() - elif t == ToolMode.LASER: - self.setCursor(Qt.CursorShape.BlankCursor) - elif t == ToolMode.ERASER: - self.setCursor(Qt.CursorShape.PointingHandCursor) - else: - self.setCursor(Qt.CursorShape.CrossCursor) + return + self.setCursor(_cursor_for_tool(self._tool, dark=self._dark_mode)) def _new_stroke(self, start: QPoint) -> Stroke: if self._tool == ToolMode.HIGHLIGHTER: diff --git a/tests/test_annotation_hud.py b/tests/test_annotation_hud.py new file mode 100644 index 0000000..8f9c8c5 --- /dev/null +++ b/tests/test_annotation_hud.py @@ -0,0 +1,234 @@ +"""Regression tests for the presentation-mode HUD and annotation overlay. + +Source-level assertions (no QApplication required in CI) that lock in the +two UX fixes shipped in ``fix/presentation-hud-cursor-icons``: + +1. The HUD Pen and Highlighter icons must be rotated so their writing tip + aligns with the mouse-pointer arrow. +2. ``AnnotationOverlay.set_tool`` must update the on-screen cursor so the + user gets visual feedback when switching tools from the HUD. +""" +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + + +_HUD_PATH = Path(__file__).resolve().parent.parent / "app" / "viewer" / "annotation_hud.py" +_OVERLAY_PATH = Path(__file__).resolve().parent.parent / "app" / "viewer" / "annotation_layer.py" + + +def _hud_src() -> str: + return _HUD_PATH.read_text(encoding="utf-8") + + +def _overlay_src() -> str: + return _OVERLAY_PATH.read_text(encoding="utf-8") + + +class TestHudIconRotation: + def test_hud_pen_icon_rotated(self): + """Regression: Pen icon must have a rotation applied. + + The rotation may be inlined at the call site or table-driven via a + module-level dict - both count. We assert both facets: (a) some + rotation directive exists in the module and (b) the pen icon is + registered in the rotation registry. + """ + src = _hud_src() + assert '"fa5s.pen"' in src + has_rotation_directive = ("rotated" in src) or ("IconOptions" in src) + assert has_rotation_directive, ( + "HUD module must apply a rotation (rotated=... or IconOptions) " + "somewhere so the pen/highlighter icons align with the pointer" + ) + assert ("_ICON_ROTATION" in src and '"fa5s.pen"' in src) or ( + "fa5s.pen" in src and "rotated" in src.split('"fa5s.pen"')[1][:300] + ), "Pen icon must be included in the HUD rotation registry" + + def test_hud_highlighter_icon_rotated(self): + src = _hud_src() + assert '"fa5s.highlighter"' in src + has_rotation_directive = ("rotated" in src) or ("IconOptions" in src) + assert has_rotation_directive, ( + "HUD module must apply a rotation somewhere so the highlighter " + "icon aligns with the pointer" + ) + assert ("_ICON_ROTATION" in src and '"fa5s.highlighter"' in src) or ( + "fa5s.highlighter" in src + and "rotated" in src.split('"fa5s.highlighter"')[1][:300] + ), "Highlighter icon must be included in the HUD rotation registry" + + def test_rotation_table_covers_pen_and_highlighter(self): + """The rotation table must include both pen and highlighter and must + not accidentally rotate the pointer/eraser/laser icons.""" + src = _hud_src() + assert "_ICON_ROTATION" in src, ( + "Expected an _ICON_ROTATION table listing icons that must rotate" + ) + # Extract just the _ICON_ROTATION literal block so we can verify + # membership without accidentally matching the _TOOLS list. + rot_idx = src.find("_ICON_ROTATION") + rot_end = src.find("}", rot_idx) + assert rot_end > rot_idx + rot_block = src[rot_idx:rot_end] + assert '"fa5s.pen"' in rot_block + assert '"fa5s.highlighter"' in rot_block + # Guardrail: pointer / eraser / laser icons must NOT be listed - + # rotating them would break the fix. + assert '"fa5s.mouse-pointer"' not in rot_block, ( + "Pointer icon is the reference direction and must not appear in " + "_ICON_ROTATION" + ) + assert '"fa5s.eraser"' not in rot_block + assert '"fa5s.dot-circle"' not in rot_block + + +class TestOverlayCursor: + def test_annotation_overlay_sets_cursor_on_tool_change(self): + """set_tool must (transitively) apply a cursor so switching tools + gives immediate visual feedback.""" + src = _overlay_src() + set_tool_idx = src.find("def set_tool") + assert set_tool_idx > 0 + snippet = src[set_tool_idx:set_tool_idx + 1200] + # set_tool delegates to _apply_cursor, which is where setCursor lives. + assert "_apply_cursor" in snippet, ( + "set_tool must invoke _apply_cursor to reflect the newly selected " + "tool on the on-screen cursor" + ) + assert "setCursor" in src, ( + "The overlay must call setCursor somewhere to change the cursor" + ) + + def test_cursor_helper_covers_all_tools(self): + """A module-level helper must map every ToolMode to a QCursor.""" + src = _overlay_src() + assert "_cursor_for_tool" in src, ( + "Expected a _cursor_for_tool helper that returns a QCursor per " + "ToolMode" + ) + # Ensure every ToolMode member is referenced in the helper. + helper_idx = src.find("def _cursor_for_tool") + assert helper_idx > 0 + helper_snippet = src[helper_idx:helper_idx + 2000] + for member in ("POINTER", "PEN", "HIGHLIGHTER", "ERASER", "LASER"): + assert f"ToolMode.{member}" in helper_snippet, ( + f"_cursor_for_tool must handle ToolMode.{member} explicitly" + ) + + def test_cursor_helper_uses_matching_pen_rotation(self): + """The pen cursor rotation must match the HUD icon rotation so the + on-screen cursor and toolbar icon stay visually consistent. + + Rotation may be applied either via qtawesome's ``rotated=`` kwarg or + via a Qt-native :class:`QTransform` (the module currently prefers + QTransform because qtawesome's ``rotated=`` does not always survive + the icon → pixmap round-trip used for cursor rendering). + """ + src = _overlay_src() + helper_idx = src.find("def _cursor_for_tool") + assert helper_idx > 0 + helper_snippet = src[helper_idx:helper_idx + 2000] + assert "fa5s.pen" in helper_snippet + assert "fa5s.highlighter" in helper_snippet + # A rotation directive must exist somewhere in the module — either + # inline (qtawesome) or via a QTransform helper. + has_rotation = ( + "rotated" in src + or "QTransform" in src + or "transform.rotate" in src.lower() + ) + assert has_rotation, ( + "Overlay module must apply a 90° rotation to the pen/highlighter " + "cursor icons (qtawesome rotated= or QTransform.rotate)" + ) + + def test_laser_cursor_is_blank(self): + """LASER cursor must be BlankCursor because the overlay draws the red + dot manually and a system cursor on top would be visually noisy.""" + src = _overlay_src() + helper_idx = src.find("def _cursor_for_tool") + helper_snippet = src[helper_idx:helper_idx + 2000] + laser_idx = helper_snippet.find("ToolMode.LASER") + assert laser_idx >= 0 + # Look at the branch body for BlankCursor. + laser_snippet = helper_snippet[laser_idx:laser_idx + 300] + assert "BlankCursor" in laser_snippet, ( + "LASER cursor should be BlankCursor - the red dot is drawn " + "manually so a system cursor on top is redundant" + ) + + +class TestIconOutline: + """Regression tests for the black-outline treatment applied to cursor + pixmaps (and the deliberate absence of that treatment on HUD icons). + + The outline (a 1 px dilation of the glyph rendered in ``#000000``) + keeps the pen/highlighter/eraser cursors legible on light-coloured + slides, where an otherwise-white glyph would blend into the page. + The HUD toolbar, by contrast, sits on the fixed dark band background + where the outline is redundant and visually noisy — so only the + cursor helper (``annotation_layer.py``) applies it. + """ + + def test_hud_icons_have_no_outline(self): + """Regression: HUD toolbar icons must NOT apply a black outline. + + The buttons sit on the fixed dark HUD band, so the outline + dilation used for cursors would just add clutter. This test + pins that decision so the outline can't silently creep back + into the HUD renderer. + """ + src = _hud_src() + # The old outline pipeline used QPainter to composite the dilated + # black glyph under the fill-coloured one; none of that machinery + # should exist in the HUD module anymore. + assert "QPainter" not in src, ( + "HUD module should not use QPainter — outline compositing " + "belongs in annotation_layer.py (cursor pixmaps) only" + ) + assert "_HUD_OUTLINE" not in src, ( + "HUD outline constants must be removed; the HUD renders icons " + "cleanly without a dilation pass" + ) + assert "#000000" not in src, ( + "HUD module should not reference the black outline colour — " + "only cursors need outlining" + ) + + def test_cursor_icons_have_black_outline(self): + """Cursor pixmaps must be composited with a black outline.""" + src = _overlay_src() + assert "#000000" in src, ( + "Overlay module must reference the black outline colour used for " + "the cursor icon dilation pass" + ) + assert "outline" in src.lower(), ( + "Overlay module must expose an outline compositing helper " + "(variable or function named *outline*)" + ) + assert "QPainter" in src, ( + "Overlay module must use QPainter to composite the outline + glyph" + ) + + def test_cursor_helper_uses_reliable_rotation(self): + """Regression: rotation must reach the final pixmap. + + Historically we relied on ``qta.icon(..., rotated=90).pixmap(...)`` + but that path did not always propagate the rotation to the pixmap + used for cursors, so the tip stayed in the wrong quadrant on some + Qt/qtawesome combos. The fix uses a Qt-native ``QTransform`` on the + rendered pixmap, which is deterministic. Accept either mechanism — + we just need one of them present so the rotation cannot silently + drop out. + """ + src = _overlay_src() + assert ( + "QTransform" in src + or "rotated=90" in src + or "rotated=90.0" in src + ), ( + "Overlay must apply the pen/highlighter rotation via QTransform " + "(preferred) or qtawesome rotated=" + )