Skip to content

Add comment sidebar and pin tool - #17

Closed
emlcpfx wants to merge 5 commits into
D-Mad:mainfrom
emlcpfx:pr1b-comment-panel
Closed

Add comment sidebar and pin tool#17
emlcpfx wants to merge 5 commits into
D-Mad:mainfrom
emlcpfx:pr1b-comment-panel

Conversation

@emlcpfx

@emlcpfx emlcpfx commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Builds on #15 (comment model). This is the user-facing half: a Comments panel and a pin tool.

What it adds

  • widgets/commentpanel.py - comment list grouped by frame. Click a row to seek to it, tick the box to mark it done, delete the selection, or type a frame-level note. Pin numbers in the list match the numbers drawn inside the on-frame markers.
  • Comment tool in the viewer toolbar. Click the frame, type the note, and it is pinned at that point.
  • View > Comments Panel (Ctrl+M). Opens to a usable width the first time it is shown; hides with the rest of the chrome in fullscreen.

The panel is a pure view over Sketch.comments and holds no state of its own, so the list and the markers drawn on the frame cannot drift apart. Every edit goes straight to the sketch and is persisted to the note sidecar from #10.

Two crashes this avoids

A {type: comment} stroke. Sketch.mousePressEvent now returns early for the comment tool. Without that, a click would create a stroke of an unknown type, which renders as nothing and is silently discarded by erase() - which only re-appends the stroke types it recognizes. Same trap the arrow tool hit in #13.

A use-after-free on the done checkbox. Toggling a comment done restyles its row in place. Rebuilding the tree from inside itemChanged destroys the very item Qt is still emitting the signal for, and takes the whole process down with an access violation. It surfaced as an intermittent crash in the test suite (~1 run in 3) before being tracked down. test_checkbox_toggles_done_on_the_sketch asserts the row is the same object afterwards, so a future refactor that reintroduces the rebuild fails loudly instead of crashing at random.

Tests

15 new tests, 48 on the branch, all passing headless:

  • grouping, ordering, counts, pin numbering, empty and unbound states
  • click-to-seek, done-toggle, delete, add-note (including blank text and no-current-frame)
  • refresh must not emit comments_changed (rebuilding sets check states; that is not a user edit)
  • the comment tool emits the normalized hit point and creates no stroke; the pencil tool still draws

ViewerWidget is a QOpenGLWidget and cannot create a context under the offscreen platform - constructing one crashes Qt on teardown intermittently, on Linux CI too. The real ViewerWidget.mousePressEvent is therefore exercised against a stand-in carrying only the attributes it reads. The code under test is the shipped code; only the GL shell is stubbed.

Needs a look in the running app

The logic is covered, but the visual result is not: open a clip, pick the Comment tool, click the frame, and confirm the pin lands where you clicked and the note shows up in the sidebar.

Depends on #15, #10, #9, #7.

emlcpfx and others added 5 commits July 14, 2026 08:17
Introduce the first automated test infrastructure for FrameDeck. There were
previously no tests; CI only ran compileall.

- tests/conftest.py forces Qt's offscreen platform plugin and a single shared
  QApplication so widgets can be constructed, rendered and pixel-probed with no
  display server. FRAMEDECK_PROFILE_ROOT is redirected to a temp dir so tests
  do not write into the user's Documents folder.
- tests/helpers.py provides render_widget_to_image / probe_pixel plus synthetic
  media generators (solid MP4 via PyAV, numbered PNG sequence, flat EXR via
  OIIO) so later tests need no external assets.
- tests/test_smoke.py verifies the harness end to end: offscreen render + pixel
  probe, FrameDeck package imports, and each media generator produces a
  decodable/readable file. 10 tests pass locally.
- requirements-dev.txt pins pytest and pypdf.
- pytest.ini sets testpaths.
- ci.yml gains a headless pytest job (installs Qt offscreen system libs plus
  runtime + dev requirements).

Also adds doc/PLAN-reviewapp-parity.md: the staged plan for porting reviewapp
features into FrameDeck across separate PRs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177bi2WkPjrGytrarheKFDc
Claude-Session-Id: 45a11a50-85fe-4349-b853-e1db9cd20276
The Sketch annotation store had undo (create/move/erase) but no redo. Add a
snapshot-based redo that reapplies the most recently undone action.

- widgets/annotations.py: add redo_history; route the four action-record sites
  through _record_action(), which appends to the undo stack and clears redo (a
  fresh edit invalidates redo, standard semantics). undo() now snapshots the
  full pre-undo stroke state onto the redo stack; redo() restores it in place
  (mutating the existing dict so held references stay valid) and re-pushes the
  action onto the undo stack.
- widgets/viewer.py: redo_strokes() mirrors undo_strokes().
- widgets/__init__.py: "Redo Note" edit-menu action bound to Ctrl+Shift+Z and
  Ctrl+Y.

Tests: tests/test_annotation_redo.py covers create redo, multi-step
undo-then-redo ordering, redo-stack invalidation on a new edit, erase
undo/redo, and empty-stack no-op. 5 tests pass (15 with the harness suite).

Depends on the test harness (PR D-Mad#7); branched on top of it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177bi2WkPjrGytrarheKFDc
Claude-Session-Id: 45a11a50-85fe-4349-b853-e1db9cd20276
FrameDeck kept annotations only in memory, so pencil/text notes were lost when
switching sources or closing the app. Persist them to JSON sidecars so they
survive a reload -- the first piece of stronger session/project state, and the
storage foundation the comment sidebar (planned) will build on.

- widgets/annotations.py: Sketch.serialize() returns a JSON-safe snapshot
  (frame -> strokes, empty frames omitted); deserialize() restores it, converting
  list coordinates/colours back to tuples and resetting undo/redo history.
- widgets/notestore.py: read/write sidecars under the FrameDeck profile dir
  (<profile>/framedeck/notes/<stem>_<hash>.fdnotes.json), keyed by a hash of the
  absolute source path. Empty notes remove a stale sidecar; unreadable/foreign
  files load as empty.
- widgets/__init__.py: openMedia saves the outgoing source's notes before the
  viewer is cleared and loads the incoming source's notes after it opens;
  closeEvent saves on exit. All hooks are defensive (a notes error is logged,
  never breaks media loading).

Tests: tests/test_notes_persistence.py covers JSON serialize/deserialize
round-trip, history reset on load, sidecar save/load round-trip, empty-removes-
sidecar, missing-clears-sketch, and deterministic profile-scoped paths. 6 tests
pass (21 with the branch suite).

Depends on the test harness (PR D-Mad#7) and annotation redo (PR D-Mad#9, for the redo
stack reset in deserialize).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177bi2WkPjrGytrarheKFDc
Claude-Session-Id: 45a11a50-85fe-4349-b853-e1db9cd20276
Foundation for the Frame.io-style review workflow: per-frame text comments that
can optionally be pinned to a point on the image. This is the model, rendering,
persistence and navigation layer; the comment sidebar UI follows separately.

- widgets/annotations.py: Sketch gains a comments store (frame -> comments) with
  add/get/delete/toggle-done, commented_frames() and comment_count(). A comment
  holds text, a timestamp, a done flag and optional normalized x/y.
  draw_comment_pins() renders numbered markers for the current frame's pinned
  comments (numbering matches list order; done comments use a distinct fill).
  annotated_frames() now returns the union of stroke and comment frames, and
  clear()/clear_all() cover both.
- widgets/notestore.py: sidecars persist comments alongside strokes. A sketch
  holding only comments is still written; sidecars predating comments (no
  "comments" key) still load.
- widgets/__init__.py: jump_to_annotation() with "[" / "]" shortcuts and Edit
  menu actions, seeking to the previous/next annotated frame. Annotations are
  keyed by the player's local frame while the timeline is global during playlist
  playback, so the jump maps local -> timeline explicitly.
- constants: COMMENT_PIN_RADIUS and the pin colours.

Tests: tests/test_comments.py covers comment CRUD, blank-text rejection,
done-toggling, the stroke/comment frame union, per-frame and global clear, pin
rendering (pixel-probing the marker fill, and the distinct done colour),
unpinned comments drawing nothing, sidecar round-trip, a comment-only sketch
still writing a sidecar, legacy sidecars without a comments key, and a missing
sidecar clearing stale comments. 12 tests pass (33 with the branch suite).

Depends on annotation persistence (PR D-Mad#10), which is on PR D-Mad#9 -> PR D-Mad#7.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177bi2WkPjrGytrarheKFDc
Claude-Session-Id: 45a11a50-85fe-4349-b853-e1db9cd20276
Turns the comment model into a usable review workflow: a Comments panel
listing every note grouped by frame, and a viewer tool that pins a note to
a point on the frame.

- widgets/commentpanel.py: frame-grouped comment tree. Click a row to seek,
  tick to mark done, delete the selection, add a frame-level note. The panel
  is a pure view over Sketch.comments and owns no state, so it cannot drift
  from the markers drawn on the frame. Pin numbers in the list match the
  numbers inside the on-frame markers.
- Viewer gains a Comment tool. Clicking the frame emits comment_requested
  with the normalized hit point; the window prompts for text and pins it.
- View > Comments Panel (Ctrl+M) toggles the sidebar, which opens to a
  usable width the first time it is shown.
- Sketch.mousePressEvent now returns early for the comment tool. A
  {type: comment} stroke would render as nothing and be silently dropped
  by erase(), which only re-appends the stroke types it recognizes.

Note toggling a comment done restyles its row in place. Rebuilding the tree
from inside itemChanged destroys the item Qt is still emitting the signal
for, which is a use-after-free that takes the process down; the tests caught
it as an intermittent access violation.

15 tests, including the in-place-restyle regression guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TexnzYbmCjjTDB8zzZuUPb
@emlcpfx emlcpfx mentioned this pull request Jul 14, 2026
@emlcpfx

emlcpfx commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #18, which landed the same feature with a fuller UI (filters, resolve/reopen). Closing in favour of it — no point carrying two comment sidebars.

Two things from this branch worth folding into #18 if they aren't already covered, because both are real crashes rather than polish:

1. Sketch.mousePressEvent should return early for the comment tool. Without that guard, a comment-tool click falls through to stroke creation and stores a {"type": "comment"} stroke. It renders as nothing, and then erase() silently discards it — erase() rebuilds the frame's stroke list and only re-appends the stroke types it recognises, so any unhandled type vanishes on the first eraser stroke anywhere on the frame. This is the same trap the arrow tool hit in #13.

2. Never rebuild the comment list from inside a QTreeWidget.itemChanged handler. Clearing the tree there destroys the very item Qt is still emitting the signal for — a use-after-free that takes the whole process down with an access violation. It only reproduces intermittently (roughly one run in three), so it presents as a flaky test rather than a crash, which is exactly what makes it dangerous. The fix is to restyle the affected row in place; a done-toggle doesn't change the row count, so there's nothing to rebuild anyway.

If #18's resolve/reopen path refreshes its list inside that signal, it has bug 2. Worth a look either way.

The CSV export in #19 doesn't depend on this branch — it only needs the comment model from #15 and the timecode from #12, both already merged. I've rebased it onto main accordingly.

@emlcpfx emlcpfx closed this Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant