UX audit fixes - #4
Merged
Merged
Conversation
…bmodule The editor kept its own single-element JSON (de)serializer that had drifted lossy: it dropped data-change animations (data_anim_in/out) on copy/paste and delete-undo, silently discarded scale_mode "tile" (via an out-of-bounds string lookup) and text_align "justify". Replace elementToJson / insertElementFromJson with calls into the engine's now-public ogt::SerializeElement / ogt::ParseElement (single source of truth), keeping the editor-side parent resolution and local->world coordinate handling. Bump the engine submodule to pick up the exposed API plus the Title::Save asset-handling hardening. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PLAN.md ranks the audited UX/correctness findings by impact x fix cost with repro/root-cause/fix per item. TASK.md tracks in-flight progress. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Selection was stored as a raw index into title.elements and only range-checked on documentChanged. Deleting a lower-indexed element (which shifts the vector) left the index in range but pointing at a different element, so the ribbon and canvas silently edited the wrong one; deleting the selected element could also retarget selection to a stranger. EditorTitle now caches the selected element's pointer identity and re-resolves the index by identity in validateSelection: it re-points on reorder/shift and clears only when the element is truly gone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
RibbonFormatSection::onIdEditingFinished renamed elements via a bare
applyMutation inside catch(...){} with no undo command and no validation.
That made rename non-undoable and corrupted undo history: element commands
resolve their target by string id at undo/redo time, so a direct rename
orphaned every earlier stacked command (its getElement(oldId) threw and was
swallowed).
- Add SetElementIdCmd: undoable rename (redo old->new, undo new->old). Because
the stack replays in order, the rename is reversed before any earlier command
referencing the old id runs, so history stays intact.
- onIdEditingFinished now rejects empty and duplicate ids (reverts the field +
QToolTip feedback) and pushes SetElementIdCmd; the catch(...){} is gone.
- EditorTitle caches the selected element's id and re-emits selectionChanged
when that id changes with the index unchanged, so the ribbon re-syncs its
string handle on rename undo/redo (rename changes id without moving the
element, so selectionChanged would otherwise never fire).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The editor gave no feedback when an image path pointed at a missing or unreadable file, and swallowed the engine's exception text on save/load (showing a generic 'Could not save/open'). The engine now throws before writing when a referenced asset is missing, so the plugin never gets a silently-broken .ogt — the editor just needs to report it. - RibbonFormatSection::updateImagePathValidity: non-blocking inline check (QFileInfo + QImageReader::canRead). A missing/unreadable path gets a red border and a warning tooltip while still applying the value; called on edit (with a tooltip popup) and on selection refresh (state only). - TitleDocument::lastError() captures the engine's exception message; the saveAs catch is narrowed from catch(...) to catch(const std::exception&) (verified safe: every throw on the engine save/load path is std-derived and the zip library never throws). - MainWindow open/save dialogs append the real cause (e.g. which asset is missing) instead of a generic message. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Delete was only reachable via the ribbon button (no shortcut) and there was no Duplicate at all. - deleteSelectedElement() extracted from the deleteElementRequested lambda and bound to a Delete/Backspace QAction. - onDuplicate() (Ctrl+D) serializes the selected element and inserts an offset copy, reusing insertElementCopy() factored out of doPaste so paste and duplicate share one code path. - Both actions live in the Clipboard ribbon panel and enable only when an element is selected. Backspace/Delete are safe while editing text: QLineEdit/spinbox/plain-text widgets claim those keys via ShortcutOverride before the window shortcut. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Handles were hit-tested against their exact 8px rect while drawn ~13px, so near-edge grabs missed. hitTest now grows each handle rect by kHitSlack (4px) and, when several enlarged rects overlap (small elements), returns the handle whose center is nearest the click instead of the first match — so edge handles stay reachable next to corners. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The tree model did beginResetModel/endResetModel on every documentChanged, so a canvas move/resize drag (which mutates per mouse-move frame) reset the tree every frame — flicker, collapse/re-expand, lost scroll, lost selection. A move/resize never changes id/zOrder/parent, so those resets were pure waste. - CanvasWidget emits interactiveEditStarted/Finished around a drag. - TitleTreeModel suppresses resets between them and fires exactly one on release if anything changed. - TitleTreeView re-applies the current selection after every reset, fixing the separate bug where any document change (even a single spinbox edit) dropped the tree's selection highlight. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Text/QR content pushed one undo entry per keystroke (no merge tag), while spinbox fields used constant merge tags that collapsed every edit to a field — even edits minutes apart — into a single undo step. A shared idle QTimer bumps an edit-generation counter after 600ms of no edits; mergeTag(base) folds the generation into each command's merge id (and restarts the timer). Consecutive edits within a gesture share a generation and merge; a pause seals the step so the next edit is a fresh undo entry. Applied to every field/bounds/shear/rotation push and to text/QR content (new ElemMergeTag::TextContent; SetElementRotationCmd gained a merge-tag param). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every catch(...) in RibbonFormatSection wrapped getElement(), which throws std::runtime_error only when the selected id went stale (a benign, expected case) — but catch(...) also silently ate any genuinely unexpected exception. Each site now catches std::runtime_error silently (stale id, nothing to do) and logs anything else via qWarning() with the exception text, without letting it escape the slot. The load/save and rename paths were already fixed in the asset-validation and undoable-rename changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Section labels, the read-only ID field, the brand-colors caption, and the keyboard-shortcuts reference used hardcoded greys (#aaa/#888/gray/#444/#ddd/#ccc) that didn't derive from the theme. They now pull from palette roles (PlaceholderText for dimmed labels, Text/BrightText for body/keys, Mid for borders) so they stay consistent with the dark theme. No light-theme branch; functional colors (error flash, missing-asset warning, checkerboard, swatch rings, timeline accents) are left as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rebuildRing/rebuildTriangle allocated their backing images at logical size and paintEvent blitted them 1:1, so the wheel was upscaled and blurry on HiDPI displays. Both images are now allocated at size()*devicePixelRatioF() and tagged with setDevicePixelRatio; the ring's QPainter path auto-scales, and the triangle's direct-pixel loop scales its vertices and bounds into physical space (barycentric weights are scale-invariant, so colors are unchanged). At dpr 1 the output is identical to before. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The keyboard-shortcuts dialog didn't mention the newly added Ctrl+D (duplicate) and Delete/Backspace (delete) actions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…DE.md Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Route the selection box, 8 handles, hit-test, cursors, and resize/rotate math through a single element-local→widget QTransform that reproduces the engine's center-pivot render, so rotated/sheared elements no longer show an un-rotated box that fights the user. - elementToWidgetTransform / elementLinear / elementLocalToTitle helpers (numerically identical to the engine Cairo transform, incl. shear) - SelectionHandles: mapped-quad border, handles at mapped points, and a new 9th drag-to-rotate knob above the top edge - hitTest / cursor body-tests inverse-map into element-local space; direction-aware resize cursors; rotate knob → cross cursor - full rotation+shear-correct resize: project the drag into local axes and solve bounds so the opposite (anchor) corner stays fixed; Ctrl keeps the center fixed; snapping gated to identity transforms only - on-canvas rotate drag pivots about the element center, Shift snaps to 15°, result normalized to (-180,180], commits one SetElementRotationCmd - new ui/widgets/ResizeMath.h: pure resizeSolve / rotateSolve / normalizeDeg Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, green knob - rotateSolve: when Shift isn't held, magnetically snap to the nearest 45° increment within 4° (Shift still gives fine 15° snapping) - draw the 8 resize handles as circles (AA on) instead of squares - fill the rotate knob green so it reads distinctly from the resize handles Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…istribute (D-2) Additive design: EditorTitle::m_selection stays the active/anchor element (every single-select path untouched); a pointer-anchored selection SET layers on top for group operations, re-anchored across mutations like the existing single mechanism. Scope: move-only on multi (single-select keeps full resize/rotate handles, gated to selectionCount()<=1); ribbon shows the active element; includes align/distribute. - EditorTitle: selectedIndices/isSelected/selectionCount/setMultiSelection/toggle/ addToSelection + selectionSetChanged(); validateSelection re-anchors the whole set. - CanvasWidget: rubber-band marquee (QPainterPath intersection, rotated-quad correct), multi-element highlight, group-move drag committed as one macro, elementTitleAABB. - TitleTreeView: SingleSelection -> ExtendedSelection with guarded two-way sync; context-menu Remove is multi-aware. - MainWindow: group delete/duplicate as single macros; Ctrl+A Select All; "N selected" status hint; Home -> Arrange panel (6 align + 2 distribute), translation-only via SetElementBoundsCmd macros. - New AlignMath.h (pure, header-only) for align/distribute deltas. Known limitation: copy/cut of a multi-selection stays single-active (clipboard holds one element); multi-clipboard/paste deferred. Verified: build exit 0; deterministic align_test PASS (6 align modes, distribute H/V, edge cases); reviewer APPROVE on the selection model + marquee + group-move logic; live qt-auto-test GUI confirmed tree multi-select, Select All, align execution, and single-step group undo. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… (D-4) D-3 — TitleTreeView gains real layers-panel affordances: - D3-1 In-place rename: TitleTreeModel::flags() marks leaf element rows (no children) editable in column 0; data() serves Qt::EditRole; setData() validates (trim, non-empty, unique across title().elements) and pushes SetElementIdCmd. View enables DoubleClicked | EditKeyPressed (F2). - D3-2 Reorder: new header-only zorderops::applyReorder (ZOrderOps.h) renumbers VisualElement siblings (sorted desc by zOrder) for ToFront/Forward/Backward/ToBack as one undo macro, reusing SetElementFieldCmd<int>. Exposed via the tree's "Order" context submenu and a Home->Arrange ribbon group (Bring to Front/Forward, Send Backward/to Back; Ctrl+]/Ctrl+[/Ctrl+Shift+]/Ctrl+Shift+[), gated in updateToolBarState() on a single element with >=2 siblings. - D3-3 Duplicate + clipboard in the tree context menu: new duplicate/cut/copy/ paste/pasteInPlace signals wired to the existing MainWindow onDuplicate/onCut/ onCopy/doPaste slots (no new logic). - D3-4 Lock: a clickable lock column (column 1, fixed width). Lock state persists in title.metadata["locked_ids"] via TitleDocument::isElementLocked/ setElementLocked (applyMutation, intentionally NOT on the undo stack). A mousePressEvent override toggles the lock without disturbing row selection. CanvasWidget skips locked elements in hit-testing/marquee and blocks move/resize/rotate/keyboard-nudge on locked elements (multi-nudge and group-drag pre-filter to unlocked members). D-4 — editor consumes the new engine schema-versioning API (engine submodule bumped 7ec908f -> 7ad3032): - TitleDocument::lastLoadDiagnostic() mirrors the lastError() pattern, populated from Title::GetLoadDiagnostic() after load(). - MainWindow::onOpen shows a non-blocking QMessageBox (WA_DeleteOnClose) when a file was written by a newer schema version (Info for newer-minor, Warning for newer-major). Editor builds clean (cmake --build build -> exit 0). Engine D-4 verified deterministically (unknown-key preservation, save/load round-trip incl. locked_ids, diagnostic bands None/Info/Warning). D-3 rename/lock/select confirmed live. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rgin) and remove dead UI files
Closes out PLAN.md's P2 batch except P2-4 (AnimationTimingEditor handle-overlap,
flagged instead as a redesign candidate since the surrounding widget has deeper
structural issues than a single patch would fix):
- P2-1: WaitCursor RAII guard on file/image I/O (open/save/save-as/paste/drop),
scoped to never cover an interactive QFileDialog or blocking QMessageBox
- P2-2a: focus restoration on the Fill/Stroke gradient dialogs and the content
editor (Qt::Tool popups previously left focus wherever the OS defaulted it)
- P2-2b: keyboard tab order across the Element/Style/Text/Image ribbon tabs
- P2-3: fitToWindow() now leaves an 8% margin instead of fitting edge-to-edge
- P2-5: investigated, closed with no code change (RibbonFormatSection already
refreshes by element ID on every documentChanged, so it can't go stale)
Also deletes ui/graphicproperties.{h,cpp}, ui/titleproperties.{h,cpp},
ui/transformeditor.{h,cpp}, ui/animationeditor.{h,cpp} — four orphaned files
predating the ribbon UI, confirmed unreferenced anywhere in the live app.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replace the per-element-bar GraphicTimingEditor/AnimationTimingEditor/ ScrubRuler with a single custom-painted QAbstractScrollArea timeline plus a right-side clip inspector, composed in a new AnimationTimingPanel. - AnimationTimelineEditor: one row per VisualElement for the selected slot; time ruler + name gutter + clip bars + visual playhead. Zoom (Ctrl+wheel about cursor), Shift+wheel pan, wheel vscroll, infinite horizontal scroll (no more Max-Duration). Clip drag (body=delay, handles=duration, 0.01s snap) with a single deferred undo push on release; context menu for type/easing. None clips render as placeholders. - ClipPropertiesPanel: Type/Easing combos + Delay/Duration spinboxes for the selected clip; guarded setClip, full-def clipChanged. - AnimationTimingPanel: slot combo (In/Out/Data In/Data Out), composes the two widgets, subscribes to EditorTitle + documentChanged, pushes SetElementAnimCmd. Selecting an element focuses/scrolls to its row and populates the panel; multi-select disables the editor. Fixes the old selection-only refresh by reacting to documentChanged. Scrub preview and play controls are intentionally dropped this pass (playhead is a visual marker only). MainWindow/CMake updated; 6 old widget files removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow-up to the timeline rewrite based on review feedback. - Fix: dragging a clip now repaints live every mouse-move. paintEvent drew the dragged row from m_rows[r].def (only updated on release); it now reads the live m_dragDef for the row under drag. - Restore playback: AnimationTimingPanel top strip gains a Play/Stop button and a Speed spinbox (%); a QTimer advances a playhead across the timeline up to contentDuration() (total across all elements for the current slot) and drives the canvas preview (scrubTimeChanged -> CanvasWidget::previewAtTime). Stop / end-of-range returns the canvas to the visible state (previewStopped). Dragging the playhead also previews. Multi-select halts playback. - Zoom controls: editable Zoom spinbox (%) + "Fit" button. New timeline API zoomPercent/setZoomPercent/zoomToFit/contentDuration/setPlayhead + signals zoomChanged (kept in sync with Ctrl+wheel) and playheadMoved. - Styling: taller clip rows (kRowH 26->32) and non-bold clip labels. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…c, observed SIGSEGV Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Engine submodule 7ad3032 -> 1c82d27. The only editor-facing change is the Title identity split: `id` is now an auto-generated uuid and the new `name` field holds the human-readable name (pre-1.1 .ogt stored the name in "id"). Retarget TitleDocument's titleName()/setTitleName()/reset() onto `name` — setTitleName() in particular would otherwise clobber the uuid on every rename. The data-source -> dataSourceId/DataPool/Scene rework is deliberately not adopted: no editor TU includes data-source.h, so it is a compile no-op. The new unconditional engine sources (scene/uuid/data-pool) are Lua-free, and lua_json/lua_xml/pugixml stay behind ENABLE_LUA_SCRIPTING which the editor forces OFF. .ogt 1.0 files migrate silently (same-major/older-minor is SchemaCompat::Ok), so legacy templates open without a new dialog. Timeline toolbar: one addStretch instead of three, so Show/transport/Speed stay flush left and Zoom/Fit pin right, with 6px gaps and VLine separators. Themed icon buttons replace the glyph-text play/stop/Fit buttons. The morphing Play/Stop toggle splits into Play/Pause + Stop + a new Reset-to-Visible, which returns the canvas to the editable Visible state after scrubbing — previously only Stop could escape preview, and it was unreachable because the button read "Play" whenever playback wasn't running. Preview lifetime is now tracked by m_previewActive, with every scrubTimeChanged/previewStopped emission funnelled through emitScrub()/ emitPreviewStopped(). A document edit or slot-combo change releases the preview: CanvasWidget skips repaints while previewing and never refreshes its snapshot, so a held preview otherwise made later edits invisible. Data slots also latch m_dataAnimState, which only clears in TickData and so survived a slot switch as a corrupted overlay. Also: playhead drag pauses playback instead of fighting the timer, playback advances on wall-clock time via QElapsedTimer, Play is a no-op when there is nothing to play, and the zoom spinbox range is (2,1667) to match the timeline's real kMinPps/kMaxPps over kBasePps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes out PLAN.md's P2 batch (busy cursor on I/O, dialog focus restoration, ribbon tab order, canvas-fit margin, P2-5 investigated/closed) except P2-4, which is flagged as a redesign candidate rather than patched. Also removes four dead UI files predating the ribbon (graphicproperties, titleproperties, transformeditor, animationeditor). Full detail and verification evidence in TASK.md.
Note: the ribbon tab-order live GUI check (P2-2b) couldn't be run this session — qt-auto-test can't inject into a window from this sandbox — flagged for a manual spot-check.