Fix layer rename reverting to default; auto-index duplicate names (#103)#108
Merged
Conversation
Renaming an element in the Layers panel reverted to the default name. There were three distinct causes: 1. Editor torn down mid-edit (all types): a debounced LayerPanel refresh (model reset) or scene-selection sync fired while the inline editor was open — the double-click that starts editing itself changes selection and arms those timers — closing the editor before the typed name was committed. Guard _do_refresh (defer) and _do_sync_from_scene_selection (skip) while the tree is in EditingState. 2. Sources had nowhere to store a name: SourceParams had no `name` field and SourceItem no `display_name`, so RenameNodeCommand wrote nothing and the label fell back to the type name. Add SourceParams.name (round-trips automatically via the vars()-based serializer). 3. TextNoteItem.display_name was not serialized, so text renames were lost on save/reload. Persist it in to_dict/from_dict. Also adds the requested auto-indexing: newly placed or dropped elements whose default label collides get a numeric suffix (Lens -> Lens 2 -> Lens 3). Logic lives in the new objects/naming.py (item_label/set_item_label/assign_unique_name), which also centralizes the label-resolution rule shared with the layer model. Applied at the two element-creation sites (toolbar placement + library drop); autolabels and paste are intentionally excluded. Adds headless-Qt regression tests covering all three rename defects, the mid-edit refresh/sync survival, auto-indexing (including gap refill), and the source/text name serialization round-trips. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extend duplicate auto-indexing to paste and duplicate (which share the clone
path in Optiverse): a pasted/duplicated "Lens" becomes "Lens 2", and a batch
stays unique against the scene and within itself ("Lens 2"/"Lens 3"). Added
assign_unique_names() for batches and a `reserved` set on assign_unique_name();
autolabels (TextNoteItem with an owner) are excluded from indexing.
Review follow-ups:
- LayerItemModel._get_item_name now delegates to naming.item_label so the
layer-tree label and auto-index label share one resolution rule (removes the
duplicated logic the helper was meant to centralize).
- TextNoteItem.clone() now copies display_name, so duplicating a renamed note
indexes on its real name ("My Note" -> "My Note 2") instead of losing it.
Tests: moved the auto-index logic tests to tests/core/test_naming.py as
Qt-free unit tests (pure fakes) so they run in CI, which skips tests/ui; kept
the Qt model/view + serialization integration tests in tests/ui. Added a
clone()-preserves-display_name regression test.
Co-Authored-By: Claude Opus 4.8 (1M context) <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.
Fixes #103.
Part 1 — Renaming a layer no longer reverts
Renaming an element in the Layers panel (double-click → type → Enter) reverted to the default name. Three independent causes, which is why every element type was affected:
LayerPanelrefresh (beginResetModel/endResetModel) or scene-selection sync fired while the inline editor was open — the double-click that starts editing itself changes selection and arms those timers (collaboration / linked-assembly refreshes do the same) — closing the editor before the typed name committed. Fixed by guarding_do_refresh(defer) and_do_sync_from_scene_selection(skip) while the tree is inEditingState. Root cause was confirmed empirically with a headless-Qt harness.SourceParamshad nonamefield andSourceItemnodisplay_name, soRenameNodeCommandwrote nothing and the label fell back to"Source". AddedSourceParams.name(round-trips via thevars()-based serializer).TextNoteItem.display_namewasn't serialized. Now persisted into_dict/from_dict.Part 2 — Auto-indexing duplicate names (place, drop, paste & duplicate)
New/dropped elements whose default label already exists get a numeric suffix:
Lens → Lens 2 → Lens 3,Text → Text 2 → Text 3. This now also covers paste and duplicate (which share theclone()path in Optiverse): a batch stays unique against the scene and within itself (Lens 2/Lens 3). The first of a kind keeps its bare name; freed gaps are refilled; autolabels are excluded.Logic lives in
objects/naming.py(item_label/set_item_label/assign_unique_name/assign_unique_names), which is the single source of truth for label resolution —LayerItemModel._get_item_namenow delegates to it. Wired at the element-creation sites (toolbar placement, library drop) and both paste paths (same-instance clone + cross-instance).TextNoteItem.clone()now also copiesdisplay_name, so duplicating a renamed note indexes on its real name.Verification / tests
tests/core/test_naming.py— Qt-free unit tests (label resolution, single/batch auto-index, gap refill, autolabel exclusion). Runs in CI (which skipstests/ui).tests/ui/test_layer_rename_and_autoindex.py— Qt integration: rename survives a refresh/sync firing mid-edit (source & text), auto-index over real scene items, source/text name serialization round-trips, andclone()preservingdisplay_name.Review
Ran a high-effort self code-review over the full change. No correctness bugs in the diff; applied two cleanups it surfaced — the
_get_item_name→item_labelde-duplication andclone()preservingdisplay_name. Lower-severity notes (skip-sync-during-edit is an intentional trade-off to protect the edit; per-batch scene recomputation is negligible) were left as-is.🤖 Generated with Claude Code