mirror of
https://github.com/penpot/penpot.git
synced 2026-07-30 18:06:15 +00:00
🐛 Fix system crash on grid element deletion of a component (#10956)
* 🐛 Add regression test: main-side component edits break copy swap slots Reproduces the :missing-slot referential-integrity failure ("Shape has been swapped, should have swap slot") that crashes files with component copies. Root cause: reordering or deleting a nested sub-head IN THE MAIN of a component, while copies exist, does not propagate swap slots to the copies. find-near-match matches a copy's sub-heads to the main's children by POSITION, so once the main's order changes the copies' shape-refs no longer match their position and, lacking a swap slot, fail referential-integrity validation. - Copy-side edits are handled correctly (characterization tests, pass today). - The two main-side tests fail today with :missing-slot and go green once the sync assigns swap slots to copies on a main reorder/delete. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * 🐛 Fix integrity crashes from copy/main child-order divergence Copy sub-heads were matched to their main's children purely by position (find-near-match), while several code paths reorder or mutilate one side only. Any of them made file validation fail with :missing-slot ("Shape has been swapped, should have swap slot"), crashing the workspace on the next validated commit, or persisting a corrupt file whose later edits crash. Reproduced live: deleting a sub-head inside a copy (which only hides it) and then reflowing the copy's grid moved the hidden (cell-less) child to the front of :shapes, knocking every sibling out of its positional slot. Four fixes, one per divergence path: - validate/check-required-swap-slot: a sub-head whose shape-ref is still a child of the near main parent is a REORDER (the component sync realigns it), not a swap; a swap slot is required only when the ref points outside the near main parent (a real swap). This matches how the sync engine itself pairs children (by shape-ref, not by position). comp-processors/fix-missing-swap-slots (migration 0019) is aligned: adding slots to merely-reordered sub-heads would freeze them out of normal synchronization. - changes/:reorder-children now refuses to alter the child structure of component copies unless allow-altering-copies is set, mirroring the is-valid-move? rule of :mov-objects; that structure is owned by the component sync engine. Grid reflows emitted this change type with no guard. pcb/reorder-grid-children also skips copy grids producer-side. - layout/reorder-grid-children keeps children that participate in no cell (hidden or absolute positioned) at their original index instead of lumping them at the front: moving them gratuitously changed their z-order and, in copies, broke the positional matching. Note :shapes stays reversed relative to the sorted cell order for in-cell children. - logic/generate-delete-shapes: deleting shapes from inside a main (without deleting the main root, whose copies keep working against the deleted component) now also deletes the copy shapes that reference them, transitively (copies of copies) and across all pages of the file, so no dangling shape-refs remain. Skipped for allow-altering-copies flows (component swap replaces the shape and the sync reconciles copies via swap slots). Cross-page removals build redo/undo changes against that page's objects directly, since the changes-builder mounts only the current page; their undo mov-objects carry allow-altering-copies so restoring inside a copy is not rejected by the new guard. The regression tests assert the fixed semantics: main-side reorders and deletes keep copies valid, the previously crashing full chain (unvalidated main reorder + later copy edit) stays healthy, :reorder-children cannot scramble copies, and reorder-grid-children keeps cell-less children in place. The namespace is now also registered in the JS test runner. AI-assisted-by: Claude Opus 4.8 (1M context) * ✨ Extend composable slot cases to the grid-reflow crash sweep Case D (CopySubheadDeletePreservesSlots) now sweeps which copy sub-head is deleted (first or last) and whether the copy root is resized afterwards, forcing a grid reflow: the reflow used to move the hidden (cell-less) child to the front of the copy's children, shifting every sibling out of its positional slot. Deleting the FIRST sub-head masked the bug (moving it to the front is a no-op), which is why the case passed before this sweep. The foundation layout is grid accordingly. Case E (MainReorderKeepsCopySlots) no longer hangs the app now that a main-side reorder leaves a valid file, so its warning docstring is replaced: it runs as a routine test (verified headless, passing) and is safe in a "run all". SlotIntegrity's doc is updated to the new validator semantics (a slot is required only for real swaps, not reorders); the positional alignment it asserts remains the correct, stronger steady-state invariant for these cases. The lockfile change materializes the playwright devDependency already declared in package.json. AI-assisted-by: Claude Opus 4.8 (1M context) * 📚 Record copy/main order-divergence invariants in memories Swap-slot semantics (membership, not positional; slots only for real swaps), the copy-structure guards on :mov-objects/:reorder-children, the grid reorder stability for cell-less children, and the main-side delete propagation in generate-delete-shapes. AI-assisted-by: Claude Opus 4.8 (1M context) * 🐛 Add adjustements to the code --------- Co-authored-by: Michael Panchenko <michael.panchenko@oraios-ai.de> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
e705b6e12c
commit
014ec34249
@ -24,6 +24,12 @@ Variant masters are main instances and component roots. Their descendants may th
|
||||
|
||||
Masters are not normally touched through `set-shape-attr`, but touched flags can appear on master shapes through cloning/duplication paths. `add-touched-from-ref-chain` in `app.common.logic.variants` unions touched flags from ancestors into the copy being processed, so upstream/master touched state can affect downstream switch behavior.
|
||||
|
||||
## Swap slots and positional matching
|
||||
|
||||
- A swap slot (stored via `ctk/set-swap-slot`, a `:touched` group `swap-slot-<uuid>`) marks a copy sub-head that was SWAPPED to another component; `compare-children` then pairs it to the main child by slot instead of by `shape-ref`.
|
||||
- Copy sub-heads without a slot are paired to main children by `shape-ref` (seek, not index). `find-near-match` (positional) is only a validator/repair heuristic; validity requires membership of the ref among the near-main parent's children, not index equality (`mem:common/file-change-validation-migration-subtleties`).
|
||||
- Copy child ORDER converges to the main's via the async sync (`moved` branch of `compare-children`); local code must never reorder copy children directly (guards in `:mov-objects`/`:reorder-children`).
|
||||
|
||||
## Cloning paths
|
||||
|
||||
`make-component-instance` in `app.common.types.container` produces a clean component copy through `update-new-shape`, dissociating attrs such as `:touched`, `:variant-id`, and `:variant-name` on cloned shapes.
|
||||
|
||||
@ -7,6 +7,8 @@
|
||||
- `set-shape-attr` treats `:position-data` as derived and never touched. Geometry/content-path changes use approximate equality; geometry differences under about 1px can be ignored for touched purposes.
|
||||
- Width/height are excluded from the `is-geometry?` branch in `set-shape-attr`; do not assume all geometry-group attrs follow identical ignore-geometry behavior.
|
||||
- `process-touched-change` marks the owning component modified when a touched shape belongs to a main instance; component-data changes can come from shape ops through this second pass.
|
||||
- Copy structure is guarded at change application: `:mov-objects` (`is-valid-move?`) and `:reorder-children` both refuse to alter children of shapes inside component copies unless the change carries `allow-altering-copies` (sync/swap flows set it). New structural change types must follow the same rule.
|
||||
- `cls/generate-delete-shapes` propagates deletions from INSIDE a component main to the copy shapes referencing them (transitively, all pages of the file) so no dangling `shape-ref`s remain; skipped when the main root itself is deleted (copies then resolve into the deleted component) and for `allow-altering-copies` flows (swap replaces the shape; sync reconciles).
|
||||
|
||||
## Shape tree edits
|
||||
|
||||
@ -19,6 +21,7 @@
|
||||
- Full referential/semantic validation currently runs only when file features contain `"components/v2"`.
|
||||
- Validation starts at root plus orphan shapes, then validates component records. `validate-file!` raises `:validation :referential-integrity` with collected details.
|
||||
- `repair-file` does not mutate data directly; it reduces validation errors into redo changes using `changes-builder`. Callers must apply or persist those changes.
|
||||
- `:missing-slot` fires only for a REAL swap: a copy sub-head whose `shape-ref` is no longer a child of the near main parent. A pure positional mismatch (ref still a sibling elsewhere) is a reorder — valid, realigned by the async component sync; do not "repair" it by assigning swap slots (a slot freezes the child out of normal sync). `fix-missing-swap-slots` (migration 0019) follows the same membership rule.
|
||||
|
||||
## Migrations
|
||||
|
||||
|
||||
@ -8,6 +8,9 @@
|
||||
## Grid assignment
|
||||
|
||||
- Grid `assign-cells` ensures at least one column and row, skips absolute-position children, creates non-tracked rows/cols when children exceed tracked cells, and asserts that assigned cells do not overlap.
|
||||
- `position-absolute?` counts HIDDEN shapes as absolute: hiding a grid child frees its cell on the next `assign-cells`.
|
||||
- `reorder-grid-children` rewrites the parent's `:shapes` to the REVERSE of the sorted cell order, but children with no cell (hidden/absolute) keep their original index — do not "fix" this into moving them to an end; that broke copy/main positional slot alignment (referential-integrity crash).
|
||||
- The `:reorder-children` change it emits is refused on parents inside component copies unless `allow-altering-copies` (same rule as `:mov-objects`); `pcb/reorder-grid-children` also skips copy grids producer-side. Copy child order is owned by the component sync engine.
|
||||
- Grid deassignment removes cells for shapes that are no longer direct children or have become absolute-positioned.
|
||||
- Auto-positioning is not just sorting: some auto cells are converted to manual when empty/manual/span state would break the auto sequence, then auto single-span items can be compacted.
|
||||
- `fix-overlaps` is marked dev-only and removes one overlapping cell, preferring empty cells first. Avoid depending on it as normal production repair.
|
||||
@ -244,6 +244,7 @@
|
||||
[:page-id {:optional true} ::sm/uuid]
|
||||
[:component-id {:optional true} ::sm/uuid]
|
||||
[:ignore-touched {:optional true} :boolean]
|
||||
[:allow-altering-copies {:optional true} :boolean]
|
||||
[:parent-id ::sm/uuid]
|
||||
[:shapes ::sm/any]]]
|
||||
|
||||
@ -633,22 +634,26 @@
|
||||
(d/update-in-when data [:components component-id :objects] process-operations change)))
|
||||
|
||||
(defn- process-children-reordering
|
||||
[objects {:keys [parent-id shapes] :as change}]
|
||||
[objects {:keys [parent-id shapes allow-altering-copies] :as change}]
|
||||
(if-let [old-shapes (dm/get-in objects [parent-id :shapes])]
|
||||
(let [id->idx
|
||||
(update-vals
|
||||
(->> (d/enumerate shapes)
|
||||
(group-by second))
|
||||
(comp first first))
|
||||
;; Component sync owns copy child ordering.
|
||||
(if (and (not allow-altering-copies)
|
||||
(ctk/in-component-copy? (get objects parent-id)))
|
||||
objects
|
||||
(let [id->idx
|
||||
(update-vals
|
||||
(->> (d/enumerate shapes)
|
||||
(group-by second))
|
||||
(comp first first))
|
||||
|
||||
new-shapes
|
||||
(vec (sort-by #(d/nilv (id->idx %) -1) < old-shapes))]
|
||||
new-shapes
|
||||
(vec (sort-by #(d/nilv (id->idx %) -1) < old-shapes))]
|
||||
|
||||
(if (not= old-shapes new-shapes)
|
||||
(do
|
||||
(some-> *touched-changes* (vswap! conj change))
|
||||
(update objects parent-id assoc :shapes new-shapes))
|
||||
objects))
|
||||
(if (not= old-shapes new-shapes)
|
||||
(do
|
||||
(some-> *touched-changes* (vswap! conj change))
|
||||
(update objects parent-id assoc :shapes new-shapes))
|
||||
objects)))
|
||||
|
||||
objects))
|
||||
|
||||
|
||||
@ -198,6 +198,20 @@
|
||||
::applied-changes-count (count redo-changes)))
|
||||
changes))
|
||||
|
||||
(defn- without-changes-local
|
||||
"Append changes through `f` without applying them to the mounted page's
|
||||
working state."
|
||||
[changes f]
|
||||
(if (contains? (meta changes) ::file-data)
|
||||
(let [changes (-> changes (apply-changes-local) (f))]
|
||||
(vary-meta changes assoc ::applied-changes-count (count (:redo-changes changes))))
|
||||
(f changes)))
|
||||
|
||||
(defn concat-changes-without-local
|
||||
"Append `other` without applying it to the mounted page's working state."
|
||||
[changes other]
|
||||
(without-changes-local changes #(concat-changes % other)))
|
||||
|
||||
;; Page changes
|
||||
|
||||
(defn add-empty-page
|
||||
@ -603,68 +617,72 @@
|
||||
(-> (reduce update-shape changes ids)
|
||||
(apply-changes-local)))))
|
||||
|
||||
(defn- add-remove-objects-changes
|
||||
[changes page-id objects ids {:keys [ignore-touched allow-altering-copies]
|
||||
:or {ignore-touched false
|
||||
allow-altering-copies false}}]
|
||||
(let [add-redo-change
|
||||
(fn [change-set id]
|
||||
(conj change-set
|
||||
(cond-> {:type :del-obj
|
||||
:page-id page-id
|
||||
:id id}
|
||||
ignore-touched
|
||||
(assoc :ignore-touched true))))
|
||||
|
||||
add-undo-change-shape
|
||||
(fn [change-set id]
|
||||
(let [shape (get objects id)]
|
||||
(cond-> change-set
|
||||
(some? shape)
|
||||
(conj {:type :add-obj
|
||||
:id id
|
||||
:page-id page-id
|
||||
:parent-id (:parent-id shape)
|
||||
:frame-id (:frame-id shape)
|
||||
:index (cfh/get-position-on-parent objects id)
|
||||
:obj (cond-> shape
|
||||
(contains? shape :shapes)
|
||||
(assoc :shapes []))}))))
|
||||
|
||||
add-undo-change-parent
|
||||
(fn [change-set id]
|
||||
(let [shape (get objects id)
|
||||
prev-sibling (cfh/get-prev-sibling objects (:id shape))]
|
||||
(cond-> change-set
|
||||
(some? shape)
|
||||
(conj (cond-> {:type :mov-objects
|
||||
:page-id page-id
|
||||
:parent-id (:parent-id shape)
|
||||
:shapes [id]
|
||||
:after-shape prev-sibling
|
||||
:index 0
|
||||
:ignore-touched true}
|
||||
allow-altering-copies
|
||||
(assoc :allow-altering-copies true))))))]
|
||||
|
||||
(-> changes
|
||||
(update :redo-changes #(reduce add-redo-change % ids))
|
||||
(update :undo-changes #(as-> % $
|
||||
(reduce add-undo-change-parent $ ids)
|
||||
(reduce add-undo-change-shape $ ids))))))
|
||||
|
||||
(defn remove-objects
|
||||
([changes ids] (remove-objects changes ids nil))
|
||||
([changes ids {:keys [ignore-touched] :or {ignore-touched false}}]
|
||||
([changes ids options]
|
||||
(assert-page-id! changes)
|
||||
(assert-objects! changes)
|
||||
(let [page-id (::page-id (meta changes))
|
||||
objects (lookup-objects changes)
|
||||
|
||||
add-redo-change
|
||||
(fn [change-set id]
|
||||
(conj change-set
|
||||
(cond-> {:type :del-obj
|
||||
:page-id page-id
|
||||
:id id}
|
||||
ignore-touched
|
||||
(assoc :ignore-touched true))))
|
||||
|
||||
add-undo-change-shape
|
||||
(fn [change-set id]
|
||||
(let [shape (get objects id)]
|
||||
(cond-> change-set
|
||||
(some? shape)
|
||||
(conj {:type :add-obj
|
||||
:id id
|
||||
:page-id page-id
|
||||
:parent-id (:parent-id shape)
|
||||
:frame-id (:frame-id shape)
|
||||
:index (cfh/get-position-on-parent objects id)
|
||||
:obj (cond-> shape
|
||||
(contains? shape :shapes)
|
||||
(assoc :shapes []))}))))
|
||||
|
||||
add-undo-change-parent
|
||||
(fn [change-set id]
|
||||
(let [shape (get objects id)
|
||||
prev-sibling (cfh/get-prev-sibling objects (:id shape))]
|
||||
(cond-> change-set
|
||||
(some? shape)
|
||||
(conj {:type :mov-objects
|
||||
:page-id page-id
|
||||
:parent-id (:parent-id shape)
|
||||
:shapes [id]
|
||||
:after-shape prev-sibling
|
||||
:index 0
|
||||
:ignore-touched true}))))]
|
||||
|
||||
(-> changes
|
||||
(update :redo-changes #(reduce add-redo-change % ids))
|
||||
(update :undo-changes #(as-> % $
|
||||
(reduce add-undo-change-parent $ ids)
|
||||
(reduce add-undo-change-shape $ ids)))
|
||||
(apply-changes-local)))))
|
||||
(-> changes
|
||||
(add-remove-objects-changes (::page-id (meta changes))
|
||||
(lookup-objects changes)
|
||||
ids
|
||||
options)
|
||||
(apply-changes-local))))
|
||||
|
||||
;; FIXME: PERFORMANCE
|
||||
(defn resize-parents
|
||||
[changes ids]
|
||||
(assert-page-id! changes)
|
||||
(assert-objects! changes)
|
||||
(let [page-id (::page-id (meta changes))
|
||||
|
||||
objects (lookup-objects changes)
|
||||
xform (comp
|
||||
(defn- add-resize-parents-changes
|
||||
[changes page-id objects ids]
|
||||
(let [xform (comp
|
||||
(mapcat #(cons % (cfh/get-parent-ids objects %)))
|
||||
(map (d/getf objects))
|
||||
(filter #(contains? #{:group :bool} (:type %)))
|
||||
@ -698,9 +716,8 @@
|
||||
(update :uops conj {:type :set :attr attr :val old-val :ignore-touched true})))))
|
||||
|
||||
resize-parent
|
||||
(fn [changes parent]
|
||||
(let [objects (lookup-objects changes)
|
||||
children (->> parent :shapes (map (d/getf objects)))
|
||||
(fn [[changes objects] parent]
|
||||
(let [children (->> parent :shapes (map (d/getf objects)))
|
||||
resized-parent (cond
|
||||
(empty? children) ;; a parent with no children will be deleted,
|
||||
nil ;; so it does not need resize
|
||||
@ -727,14 +744,24 @@
|
||||
:id (:id parent)}]
|
||||
|
||||
(if (seq rops)
|
||||
(-> changes
|
||||
(update :redo-changes conj (assoc change :operations rops))
|
||||
(update :undo-changes conj (assoc change :operations uops))
|
||||
(apply-changes-local))
|
||||
changes))
|
||||
changes)))]
|
||||
[(-> changes
|
||||
(update :redo-changes conj (assoc change :operations rops))
|
||||
(update :undo-changes conj (assoc change :operations uops)))
|
||||
(assoc objects (:id parent) resized-parent)]
|
||||
[changes objects]))
|
||||
[changes objects])))]
|
||||
|
||||
(reduce resize-parent changes all-parents)))
|
||||
(first (reduce resize-parent [changes objects] all-parents))))
|
||||
|
||||
(defn resize-parents
|
||||
[changes ids]
|
||||
(assert-page-id! changes)
|
||||
(assert-objects! changes)
|
||||
(-> changes
|
||||
(add-resize-parents-changes (::page-id (meta changes))
|
||||
(lookup-objects changes)
|
||||
ids)
|
||||
(apply-changes-local)))
|
||||
|
||||
;; Library changes
|
||||
|
||||
@ -1148,6 +1175,8 @@
|
||||
(->> ids
|
||||
(map (d/getf objects))
|
||||
(filter ctl/grid-layout?)
|
||||
;; Component sync owns copy child ordering.
|
||||
(remove ctk/in-component-copy?)
|
||||
(reduce reorder-grid changes))]
|
||||
|
||||
changes))
|
||||
|
||||
@ -63,8 +63,8 @@
|
||||
file-data)))
|
||||
|
||||
(defn fix-missing-swap-slots
|
||||
"Locate shapes that have been swapped (i.e. their shape-ref does not point to the near match) but
|
||||
they don't have a swap slot. In this case, add one pointing to the near match."
|
||||
"Locate shapes that have been swapped (see `ctf/swapped-subhead?`) but don't have a swap slot.
|
||||
In this case, add one pointing to the near match."
|
||||
[file-data libraries]
|
||||
(try
|
||||
(ctf/update-all-shapes
|
||||
@ -73,7 +73,11 @@
|
||||
(if (ctk/subcopy-head? shape)
|
||||
(let [container (:container (meta shape))
|
||||
file {:id (:id file-data) :data file-data}
|
||||
near-match (ctf/find-near-match file container libraries shape :include-deleted? true :with-context? false)]
|
||||
swapped? (ctf/swapped-subhead?
|
||||
shape container
|
||||
#(ctf/find-ref-shape file container libraries % :include-deleted? true))
|
||||
near-match (when swapped?
|
||||
(ctf/find-near-match file container libraries shape :include-deleted? true :with-context? false))]
|
||||
(if (and (some? near-match)
|
||||
(not= (:shape-ref shape) (:id near-match))
|
||||
(nil? (ctk/get-swap-slot shape)))
|
||||
|
||||
@ -438,15 +438,15 @@
|
||||
shape file page)))
|
||||
|
||||
(defn- check-required-swap-slot
|
||||
"Validate that the shape has swap-slot if it's a subinstance head and the ref shape is not the
|
||||
matching shape by position in the near main."
|
||||
"Validate that the shape has a swap slot if it's a subinstance head that has been
|
||||
swapped (see `ctf/swapped-subhead?`)."
|
||||
[shape file page libraries]
|
||||
;; Guard first: if the shape already has a swap slot the invariant is satisfied
|
||||
;; and we can avoid the expensive `find-near-match` call entirely.
|
||||
(when (nil? (ctk/get-swap-slot shape))
|
||||
;; and we can avoid the ref-shape lookups entirely.
|
||||
(when (and (nil? (ctk/get-swap-slot shape))
|
||||
(ctf/swapped-subhead? shape page #(find-ref-shape* file page libraries %)))
|
||||
(let [near-match (ctf/find-near-match file page libraries shape :include-deleted? true :with-context? false)]
|
||||
(when (and (some? near-match)
|
||||
(not= (:shape-ref shape) (:id near-match)))
|
||||
(when (some? near-match)
|
||||
(report-error :missing-slot
|
||||
"Shape has been swapped, should have swap slot"
|
||||
shape file page
|
||||
|
||||
@ -113,6 +113,116 @@
|
||||
(-> changes
|
||||
(pcb/update-shapes ids update-fn {:attrs #{:blocked :hidden}}))))
|
||||
|
||||
;; Deleting shapes inside a surviving component main transitively removes their
|
||||
;; copies across all pages. Whole-main and component-swap deletions are excluded.
|
||||
|
||||
(defn- mutilates-main?
|
||||
"Whether deleting `id` takes a shape out of a component main that survives.
|
||||
`deleted-ids` are all the ids the deletion removes from the page."
|
||||
[objects deleted-ids id]
|
||||
(->> (cfh/get-parent-ids objects id)
|
||||
(some (fn [parent-id]
|
||||
(let [parent (get objects parent-id)]
|
||||
(and (:main-instance parent)
|
||||
(not (contains? deleted-ids parent-id))))))))
|
||||
|
||||
(defn- build-shape-ref-index
|
||||
"Index every referencing shape by shape-ref and page."
|
||||
[pages-index page-objects]
|
||||
(reduce (fn [index page-id]
|
||||
(reduce (fn [index shape]
|
||||
(if-let [shape-ref (:shape-ref shape)]
|
||||
(update index shape-ref (fnil conj []) [page-id (:id shape)])
|
||||
index))
|
||||
index
|
||||
(vals (page-objects page-id))))
|
||||
{}
|
||||
(keys pages-index)))
|
||||
|
||||
(defn- collect-copy-deletions
|
||||
"Collect dangling copy roots and their subtree ids by page.
|
||||
`scheduled` prevents duplicate deletions during transitive traversal."
|
||||
[ref-index descendants-of dangling scheduled]
|
||||
(loop [dangling dangling
|
||||
scheduled scheduled
|
||||
result {}]
|
||||
(if (empty? dangling)
|
||||
result
|
||||
(let [hits
|
||||
(into #{}
|
||||
(comp (mapcat ref-index)
|
||||
(remove (fn [[_ id]] (contains? scheduled id))))
|
||||
dangling)
|
||||
|
||||
;; Ancestor subtrees already contain nested hits.
|
||||
nested
|
||||
(into #{}
|
||||
(mapcat (fn [[page-id id]] (descendants-of page-id id)))
|
||||
hits)
|
||||
|
||||
subtrees
|
||||
(into []
|
||||
(comp (remove (fn [[_ id]] (contains? nested id)))
|
||||
(map (fn [[page-id id]]
|
||||
[page-id id (-> (descendants-of page-id id)
|
||||
(set)
|
||||
(conj id))])))
|
||||
hits)
|
||||
|
||||
deleted
|
||||
(into #{} (mapcat #(nth % 2)) subtrees)]
|
||||
(recur deleted
|
||||
(into scheduled deleted)
|
||||
(reduce (fn [result [page-id root-id ids]]
|
||||
(-> result
|
||||
(update-in [page-id :roots]
|
||||
(fnil conj (d/ordered-set))
|
||||
root-id)
|
||||
(update-in [page-id :ids] (fnil into #{}) ids)))
|
||||
result
|
||||
subtrees))))))
|
||||
|
||||
(defn- propagated-copy-deletions
|
||||
"Collect propagated copy deletions unless component sync owns the operation."
|
||||
[objects pages-index page-objects deleted-ids allow-altering-copies]
|
||||
(if (or allow-altering-copies (nil? pages-index))
|
||||
{}
|
||||
(let [dangling (into #{}
|
||||
(filter #(mutilates-main? objects deleted-ids %))
|
||||
deleted-ids)]
|
||||
(if (empty? dangling)
|
||||
{}
|
||||
(collect-copy-deletions (build-shape-ref-index pages-index page-objects)
|
||||
;; Cache subtrees shared by multiple references.
|
||||
(memoize (fn [page-id id]
|
||||
(cfh/get-children-ids (page-objects page-id) id)))
|
||||
dangling
|
||||
deleted-ids)))))
|
||||
|
||||
(declare generate-delete-shapes)
|
||||
|
||||
(defn- generate-copy-deletions
|
||||
"Delete propagated copy roots through each page's normal deletion workflow."
|
||||
[changes data page pages-index copy-deletions]
|
||||
(reduce-kv (fn [changes pid {:keys [roots]}]
|
||||
(let [options {:ignore-touched true
|
||||
:allow-altering-copies true}
|
||||
target-page (if (= pid (:id page))
|
||||
page
|
||||
(get pages-index pid))]
|
||||
(if (= pid (:id page))
|
||||
(second (generate-delete-shapes changes roots options))
|
||||
(let [[_ target-changes]
|
||||
(generate-delete-shapes (pcb/empty-changes nil pid)
|
||||
data
|
||||
target-page
|
||||
(:objects target-page)
|
||||
roots
|
||||
options)]
|
||||
(pcb/concat-changes-without-local changes target-changes)))))
|
||||
changes
|
||||
copy-deletions))
|
||||
|
||||
(defn generate-delete-shapes
|
||||
([changes file page objects ids options]
|
||||
(generate-delete-shapes (-> changes
|
||||
@ -185,17 +295,19 @@
|
||||
ids-to-delete)
|
||||
[])
|
||||
|
||||
interacting-shapes
|
||||
(filter (fn [shape]
|
||||
;; If any of the deleted shapes is the destination of
|
||||
;; some interaction, this must be deleted, too.
|
||||
(let [interactions (:interactions shape)]
|
||||
(some #(and (ctsi/has-destination %)
|
||||
(contains? ids-to-delete (:destination %)))
|
||||
interactions)))
|
||||
(vals objects))
|
||||
|
||||
id-to-delete? (set ids-to-delete)
|
||||
|
||||
interacting-shapes
|
||||
(into []
|
||||
(filter (fn [shape]
|
||||
;; If any of the deleted shapes is the destination of
|
||||
;; some interaction, this must be deleted, too.
|
||||
(let [interactions (:interactions shape)]
|
||||
(some #(and (ctsi/has-destination %)
|
||||
(id-to-delete? (:destination %)))
|
||||
interactions))))
|
||||
(vals objects))
|
||||
|
||||
changes
|
||||
(->> (:flows page)
|
||||
(reduce
|
||||
@ -261,19 +373,43 @@
|
||||
[]
|
||||
(into ids-to-delete descendants-to-delete))
|
||||
|
||||
;; Empty main parents also leave their copies dangling.
|
||||
all-deleted-ids
|
||||
(-> (set ids-to-delete)
|
||||
(into descendants-to-delete)
|
||||
(into empty-parents))
|
||||
|
||||
ids-set (set ids-to-delete)
|
||||
pages-index
|
||||
(when data
|
||||
(or (:pages-index data)
|
||||
(dm/get-in data [:data :pages-index])))
|
||||
|
||||
page-objects
|
||||
(fn [id]
|
||||
(if (= id (:id page))
|
||||
objects
|
||||
(dm/get-in pages-index [id :objects])))
|
||||
|
||||
copy-deletions
|
||||
(propagated-copy-deletions objects pages-index page-objects
|
||||
all-deleted-ids allow-altering-copies)
|
||||
|
||||
;; Propagated copy deletions supersede hiding the same shapes.
|
||||
ids-to-hide
|
||||
(if-let [deleted (seq (get-in copy-deletions [(:id page) :ids]))]
|
||||
(into [] (remove (set deleted)) ids-to-hide)
|
||||
ids-to-hide)
|
||||
|
||||
guides-to-delete
|
||||
(->> (:guides page)
|
||||
(vals)
|
||||
(filter #(contains? ids-set (:frame-id %)))
|
||||
(filter #(id-to-delete? (:frame-id %)))
|
||||
(map :id))
|
||||
|
||||
changes (reduce (fn [changes guide-id]
|
||||
(-> changes
|
||||
(pcb/with-page page)
|
||||
(pcb/set-flow guide-id nil)))
|
||||
(pcb/set-guide guide-id nil)))
|
||||
changes
|
||||
guides-to-delete)
|
||||
|
||||
@ -289,6 +425,7 @@
|
||||
(pcb/remove-objects descendants-to-delete {:ignore-touched true})
|
||||
(pcb/remove-objects ids-to-delete {:ignore-touched ignore-touched})
|
||||
(pcb/remove-objects empty-parents)
|
||||
(generate-copy-deletions data page pages-index copy-deletions)
|
||||
(pcb/resize-parents all-parents)
|
||||
(pcb/update-shapes groups-to-unmask
|
||||
(fn [shape]
|
||||
@ -299,7 +436,7 @@
|
||||
(fn [interactions]
|
||||
(into []
|
||||
(remove #(and (ctsi/has-destination %)
|
||||
(contains? ids-to-delete (:destination %))))
|
||||
(id-to-delete? (:destination %))))
|
||||
interactions))))))]
|
||||
[all-parents changes])))
|
||||
|
||||
|
||||
@ -429,6 +429,17 @@
|
||||
(with-meta (meta parent-ref-shape))))]
|
||||
near-match))
|
||||
|
||||
(defn swapped-subhead?
|
||||
"Whether `shape` references outside its near main parent and needs a swap slot.
|
||||
Same-parent positional differences are synchronized as reorders."
|
||||
[shape container find-parent-ref-shape]
|
||||
(let [parent-shape (ctst/get-shape container (:parent-id shape))
|
||||
parent-ref-shape (when parent-shape
|
||||
(find-parent-ref-shape parent-shape))]
|
||||
(and (some? parent-ref-shape)
|
||||
(not-any? #(= % (:shape-ref shape))
|
||||
(:shapes parent-ref-shape)))))
|
||||
|
||||
(defn advance-shape-ref
|
||||
"Get the shape-ref of the near main of the shape, recursively repeated as many times
|
||||
as the given levels."
|
||||
|
||||
@ -1525,20 +1525,37 @@
|
||||
(some? target-cell)
|
||||
(add-children-to-cell ids objects [(:row target-cell) (:column target-cell)]))))
|
||||
|
||||
(defn- refill-slots
|
||||
"Fill matching positions in `shapes` from `ordered`, preserving other indices.
|
||||
`ordered` must contain exactly the ids accepted by `slot?`."
|
||||
[shapes slot? ordered]
|
||||
(loop [shapes (seq shapes)
|
||||
ordered (seq ordered)
|
||||
result (transient [])]
|
||||
(if (nil? shapes)
|
||||
(persistent! result)
|
||||
(let [id (first shapes)]
|
||||
(if (slot? id)
|
||||
(recur (next shapes) (next ordered) (conj! result (first ordered)))
|
||||
(recur (next shapes) ordered (conj! result id)))))))
|
||||
|
||||
(defn reorder-grid-children
|
||||
"Order cell children by grid position while preserving the indices of
|
||||
hidden and absolute-positioned children."
|
||||
[parent]
|
||||
(let [cells (get-cells parent {:sort? true})
|
||||
(let [cells (get-cells parent {:sort? true})
|
||||
child? (set (:shapes parent))
|
||||
new-shapes
|
||||
(into (d/ordered-set)
|
||||
|
||||
in-cell-ids
|
||||
(into []
|
||||
(comp (keep (comp first :shapes))
|
||||
(filter child?))
|
||||
cells)
|
||||
|
||||
;; Add the children that are not in cells (absolute positioned for example)
|
||||
new-shapes (into new-shapes (:shapes parent))]
|
||||
|
||||
(assoc parent :shapes (into [] (reverse new-shapes)))))
|
||||
(filter child?)
|
||||
(distinct))
|
||||
cells)]
|
||||
;; :shapes is ordered in reverse relative to the visual cell order
|
||||
(assoc parent :shapes (refill-slots (:shapes parent)
|
||||
(set in-cell-ids)
|
||||
(reverse in-cell-ids)))))
|
||||
|
||||
(defn cells-by-row
|
||||
([parent index]
|
||||
|
||||
@ -0,0 +1,221 @@
|
||||
;; This Source Code Form is subject to the terms of the Mozilla Public
|
||||
;; License, v. 2.0. If a copy of the MPL was not distributed with this
|
||||
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
|
||||
;;
|
||||
;; Copyright (c) KALEIDOS INC Sucursal en España SL
|
||||
|
||||
(ns common-tests.logic.comp-main-edit-breaks-copy-slots-test
|
||||
(:require
|
||||
[app.common.files.changes :as cpc]
|
||||
[app.common.files.changes-builder :as pcb]
|
||||
[app.common.logic.shapes :as cls]
|
||||
[app.common.test-helpers.components :as thc]
|
||||
[app.common.test-helpers.compositions :as tho]
|
||||
[app.common.test-helpers.files :as thf]
|
||||
[app.common.test-helpers.ids-map :as thi]
|
||||
[app.common.test-helpers.shapes :as ths]
|
||||
[clojure.test :as t]))
|
||||
|
||||
(t/use-fixtures :each thi/test-fixture)
|
||||
|
||||
;; Main-side reorder and deletion preserve copy reference integrity and support
|
||||
;; exact undo across pages.
|
||||
(defn- setup-main
|
||||
[file]
|
||||
(-> file
|
||||
(tho/add-simple-component :icon :icon-main :icon-child)
|
||||
(tho/add-frame :row-main :name "Row")
|
||||
(thc/instantiate-component :icon :icon-1 :parent-label :row-main)
|
||||
(thc/instantiate-component :icon :icon-2 :parent-label :row-main)
|
||||
(thc/instantiate-component :icon :icon-3 :parent-label :row-main)
|
||||
(thc/make-component :row :row-main)))
|
||||
|
||||
(defn- setup-file
|
||||
[]
|
||||
(-> (thf/sample-file :file1)
|
||||
(setup-main)
|
||||
(thc/instantiate-component :row :row-copy :children-labels [:copy-1 :copy-2 :copy-3])))
|
||||
|
||||
(defn- add-copy-page-metadata
|
||||
[file]
|
||||
(let [page-id (thi/id :page2)
|
||||
copy-id (thi/id :copy-1)
|
||||
flow-id (thi/new-id! :copy-flow)
|
||||
guide-id (thi/new-id! :copy-guide)]
|
||||
(-> file
|
||||
(ths/add-interaction :copy-2 :copy-1)
|
||||
(assoc-in [:data :pages-index page-id :flows flow-id]
|
||||
{:id flow-id :name "Copy flow" :starting-frame copy-id})
|
||||
(assoc-in [:data :pages-index page-id :guides guide-id]
|
||||
{:id guide-id :axis :x :position 10 :frame-id copy-id}))))
|
||||
|
||||
;; Same structure, but with :row-copy on a second page: the propagated
|
||||
;; deletions then leave the page mounted in the changes builder.
|
||||
(defn- setup-cross-page-file
|
||||
[]
|
||||
(-> (thf/sample-file :file1 :page-label :page1)
|
||||
(setup-main)
|
||||
(thf/add-sample-page :page2)
|
||||
(thc/instantiate-component :row :row-copy :children-labels [:copy-1 :copy-2 :copy-3])
|
||||
(add-copy-page-metadata)
|
||||
(thf/switch-to-page :page1)))
|
||||
|
||||
(defn- delete-changes
|
||||
[file shape-label]
|
||||
(let [page (thf/current-page file)]
|
||||
(second (cls/generate-delete-shapes (pcb/empty-changes nil (:id page))
|
||||
file
|
||||
page
|
||||
(:objects page)
|
||||
#{(:id (ths/get-shape file shape-label))}
|
||||
{}))))
|
||||
|
||||
(defn- deleted-ids
|
||||
[changes]
|
||||
(->> (:redo-changes changes)
|
||||
(filter #(= :del-obj (:type %)))
|
||||
(map :id)))
|
||||
|
||||
;; Deleting a nested sub-head of a COPY only hides it (deleted-subinstance).
|
||||
(t/deftest deleting-a-copy-subhead-only-hides-it
|
||||
(let [file (setup-file)
|
||||
file' (tho/delete-shape file :copy-1)
|
||||
copy-1' (ths/get-shape file' :copy-1)]
|
||||
(t/is (some? copy-1'))
|
||||
(t/is (true? (:hidden copy-1')))))
|
||||
|
||||
;; Reordering a nested sub-head within a COPY keeps referential integrity.
|
||||
(t/deftest reordering-a-copy-subhead-keeps-referential-integrity
|
||||
(let [file (setup-file)
|
||||
page (thf/current-page file)
|
||||
copy-1 (ths/get-shape file :copy-1)
|
||||
row-copy (ths/get-shape file :row-copy)
|
||||
changes (cls/generate-relocate (-> (pcb/empty-changes nil)
|
||||
(pcb/with-page-id (:id page))
|
||||
(pcb/with-objects (:objects page)))
|
||||
(:id row-copy) 2 #{(:id copy-1)})
|
||||
file' (thf/apply-changes file changes)]
|
||||
(t/is (some? (ths/get-shape file' :copy-2)))
|
||||
(t/is (some? (ths/get-shape file' :copy-3)))))
|
||||
|
||||
;; Main-side reorders remain valid while component sync realigns copy children.
|
||||
(t/deftest reordering-a-main-subhead-must-not-break-copies
|
||||
(let [file (setup-file)
|
||||
page (thf/current-page file)
|
||||
row-main (ths/get-shape file :row-main)
|
||||
icon-1 (ths/get-shape file :icon-1)
|
||||
;; move the main's first sub-head to the end (index 2)
|
||||
changes (cls/generate-relocate (-> (pcb/empty-changes nil)
|
||||
(pcb/with-page-id (:id page))
|
||||
(pcb/with-objects (:objects page)))
|
||||
(:id row-main) 2 #{(:id icon-1)})
|
||||
file' (thf/apply-changes file changes)]
|
||||
(t/is (some? (ths/get-shape file' :copy-1)))
|
||||
(t/is (some? (ths/get-shape file' :copy-2)))
|
||||
(t/is (some? (ths/get-shape file' :copy-3)))))
|
||||
|
||||
;; Main-side deletions remove corresponding copy shapes and dangling refs.
|
||||
(t/deftest deleting-a-main-subhead-must-not-break-copies
|
||||
(let [file (setup-file)
|
||||
file' (tho/delete-shape file :icon-1)]
|
||||
(t/is (nil? (ths/get-shape file' :copy-1)))
|
||||
(t/is (some? (ths/get-shape file' :copy-2)))
|
||||
(t/is (some? (ths/get-shape file' :copy-3)))))
|
||||
|
||||
;; Each propagated shape is deleted once so undo restores a valid tree.
|
||||
(t/deftest propagated-deletions-are-emitted-once
|
||||
(let [file (setup-file)
|
||||
changes (delete-changes file :icon-1)
|
||||
ids (deleted-ids changes)]
|
||||
(t/is (= (count ids) (count (distinct ids))))))
|
||||
|
||||
;; Empty main groups propagate deletion to their corresponding copy groups.
|
||||
(t/deftest emptied-main-groups-propagate-to-copies
|
||||
(let [file (-> (thf/sample-file :file1)
|
||||
(tho/add-simple-component :icon :icon-main :icon-child)
|
||||
(tho/add-frame :row-main :name "Row")
|
||||
(tho/add-group :grp-main :parent-label :row-main)
|
||||
(thc/instantiate-component :icon :icon-1 :parent-label :grp-main)
|
||||
(thc/make-component :row :row-main)
|
||||
(thc/instantiate-component :row :row-copy :children-labels [:grp-copy])
|
||||
(tho/delete-shape :icon-1))]
|
||||
(t/is (nil? (ths/get-shape file :grp-main)))
|
||||
(t/is (nil? (ths/get-shape file :grp-copy)))))
|
||||
|
||||
;; Propagated deletion supersedes hiding the same selected copy shape.
|
||||
(t/deftest propagated-deletions-are-not-hidden-first
|
||||
(let [file (setup-file)
|
||||
page (thf/current-page file)
|
||||
icon-1 (ths/get-shape file :icon-1)
|
||||
copy-1 (ths/get-shape file :copy-1)
|
||||
[_ changes] (cls/generate-delete-shapes (pcb/empty-changes nil (:id page))
|
||||
file page (:objects page)
|
||||
#{(:id icon-1) (:id copy-1)}
|
||||
{})
|
||||
file' (thf/apply-changes file changes)]
|
||||
(t/is (nil? (ths/get-shape file' :copy-1)))
|
||||
(t/is (not (contains? (->> (:redo-changes changes)
|
||||
(filter #(= :mod-obj (:type %)))
|
||||
(map :id)
|
||||
(set))
|
||||
(:id copy-1))))))
|
||||
|
||||
;; Cross-page propagation restores the exact original structure on undo.
|
||||
(t/deftest deleting-a-main-subhead-propagates-across-pages
|
||||
(let [file (setup-cross-page-file)
|
||||
changes (delete-changes file :icon-1)
|
||||
ids (deleted-ids changes)
|
||||
file' (thf/apply-changes file changes)
|
||||
file'' (thf/apply-undo-changes file' changes)
|
||||
page2-id (thi/id :page2)
|
||||
copy-2' (ths/get-shape file' :copy-2 :page-label :page2)]
|
||||
(t/is (= (count ids) (count (distinct ids))))
|
||||
(t/is (nil? (ths/get-shape file' :copy-1 :page-label :page2)))
|
||||
(t/is (some? (ths/get-shape file' :copy-2 :page-label :page2)))
|
||||
(t/is (some? (ths/get-shape file' :copy-3 :page-label :page2)))
|
||||
(t/is (empty? (:interactions copy-2')))
|
||||
(t/is (nil? (get-in file' [:data :pages-index page2-id :flows (thi/id :copy-flow)])))
|
||||
(t/is (nil? (get-in file' [:data :pages-index page2-id :guides (thi/id :copy-guide)])))
|
||||
(t/is (= (:pages-index (:data file))
|
||||
(:pages-index (:data file''))))))
|
||||
|
||||
;; A persisted main reorder remains valid before and after a later copy edit.
|
||||
(t/deftest main-reorder-keeps-copies-valid-for-later-edits
|
||||
(let [file (setup-file)
|
||||
page (thf/current-page file)
|
||||
row-main (ths/get-shape file :row-main)
|
||||
icon-1 (ths/get-shape file :icon-1)
|
||||
reorder (cls/generate-relocate (-> (pcb/empty-changes nil)
|
||||
(pcb/with-page-id (:id page))
|
||||
(pcb/with-objects (:objects page)))
|
||||
(:id row-main) 2 #{(:id icon-1)})
|
||||
;; Apply without validation to model persisted intermediate state.
|
||||
file' (thf/apply-changes file reorder :validate? false)]
|
||||
(thf/validate-file! file')
|
||||
(let [file'' (tho/delete-shape file' :copy-2)
|
||||
copy-2' (ths/get-shape file'' :copy-2)]
|
||||
(t/is (some? copy-2'))
|
||||
(t/is (true? (:hidden copy-2'))))))
|
||||
|
||||
;; Copy ordering rejects local reorders unless component sync owns the operation.
|
||||
(t/deftest reorder-children-change-cannot-alter-copies
|
||||
(let [file (setup-file)
|
||||
page (thf/current-page file)
|
||||
row-copy (ths/get-shape file :row-copy)
|
||||
scrambled (vec (reverse (:shapes row-copy)))
|
||||
change {:type :reorder-children
|
||||
:page-id (:id page)
|
||||
:parent-id (:id row-copy)
|
||||
:shapes scrambled}
|
||||
get-order (fn [data]
|
||||
(get-in data [:pages-index (:id page)
|
||||
:objects (:id row-copy) :shapes]))]
|
||||
;; without allow-altering-copies the reorder is rejected
|
||||
(t/is (= (:shapes row-copy)
|
||||
(get-order (cpc/process-changes (:data file) [change] false))))
|
||||
;; the sync engine can still restructure copies explicitly
|
||||
(t/is (= scrambled
|
||||
(get-order (cpc/process-changes
|
||||
(:data file)
|
||||
[(assoc change :allow-altering-copies true)]
|
||||
false))))))
|
||||
@ -47,6 +47,7 @@
|
||||
[common-tests.logic.chained-propagation-test]
|
||||
[common-tests.logic.comp-creation-test]
|
||||
[common-tests.logic.comp-detach-with-nested-test]
|
||||
[common-tests.logic.comp-main-edit-breaks-copy-slots-test]
|
||||
[common-tests.logic.comp-remove-swap-slots-test]
|
||||
[common-tests.logic.comp-reset-test]
|
||||
[common-tests.logic.comp-sync-test]
|
||||
@ -122,6 +123,7 @@
|
||||
'common-tests.logic.chained-propagation-test
|
||||
'common-tests.logic.comp-creation-test
|
||||
'common-tests.logic.comp-detach-with-nested-test
|
||||
'common-tests.logic.comp-main-edit-breaks-copy-slots-test
|
||||
'common-tests.logic.comp-remove-swap-slots-test
|
||||
'common-tests.logic.comp-reset-test
|
||||
'common-tests.logic.comp-sync-test
|
||||
|
||||
@ -1211,7 +1211,29 @@
|
||||
;; so shape-id-2 before shape-id-1 in new order; reorder-grid-children reverses
|
||||
(let [result (layout/reorder-grid-children parent)]
|
||||
(t/is (vector? (:shapes result)))
|
||||
(t/is (= 2 (count (:shapes result))))))))
|
||||
(t/is (= [shape-id-1 shape-id-2] (:shapes result)))))))
|
||||
|
||||
(t/deftest reorder-grid-children-keeps-cell-less-children-in-place-test
|
||||
;; Cell-less children keep their index while cell children follow grid order.
|
||||
(let [shape-id-1 (uuid/next)
|
||||
shape-id-2 (uuid/next)
|
||||
hidden-id (uuid/next)
|
||||
;; Grid storage reverses the visual cell order.
|
||||
cell-a (make-cell :row 1 :column 1 :shapes [shape-id-2])
|
||||
cell-b (make-cell :row 1 :column 2 :shapes [shape-id-1])
|
||||
parent {:layout-grid-dir :row
|
||||
:shapes [shape-id-2 shape-id-1 hidden-id]
|
||||
:layout-grid-cells {(:id cell-a) cell-a
|
||||
(:id cell-b) cell-b}}]
|
||||
|
||||
(t/testing "cell-less child at the end stays at the end"
|
||||
(let [result (layout/reorder-grid-children parent)]
|
||||
(t/is (= [shape-id-1 shape-id-2 hidden-id] (:shapes result)))))
|
||||
|
||||
(t/testing "cell-less child in the middle stays in the middle"
|
||||
(let [parent (assoc parent :shapes [shape-id-2 hidden-id shape-id-1])
|
||||
result (layout/reorder-grid-children parent)]
|
||||
(t/is (= [shape-id-1 hidden-id shape-id-2] (:shapes result)))))))
|
||||
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
;; merge-cells
|
||||
|
||||
@ -2,9 +2,12 @@ import { Board, Shape } from "@penpot/plugin-types";
|
||||
import { TestCase } from "../test-suite/TestCase.ts";
|
||||
import { Situation } from "../core/Situation";
|
||||
import { Color } from "../model/Color";
|
||||
import { ShapePropHeight } from "../model/ShapeProp.ts";
|
||||
import { OpAssert } from "../operations/OpAssert";
|
||||
import { OpSequence } from "../operations/OpSequence.ts";
|
||||
import { OpOneOf } from "../operations/OpOneOf.ts";
|
||||
import { OpOptional } from "../operations/OpOptional.ts";
|
||||
import { OpChangeProperty } from "../operations/OpChangeProperty";
|
||||
import { OpCreateNestableComponent } from "../operations/OpCreateNestableComponent";
|
||||
import { OpDeleteShape } from "../operations/OpDeleteShape";
|
||||
import { ContentCreationStrategySiblingInstances } from "../content-creation/ContentCreationStrategySiblingInstances";
|
||||
@ -13,47 +16,37 @@ import { SlotIntegrity } from "../util/SlotIntegrity";
|
||||
const BASELINE = new Color("#aaaaaa");
|
||||
const NESTED_COUNT = 3;
|
||||
const LAYOUT = "grid" as const;
|
||||
const REFLOW_HEIGHT = 500; // any size change of the grid root forces a reflow
|
||||
|
||||
/**
|
||||
* Case D — CHARACTERIZATION: deleting a nested sub-head of a copy is well-behaved.
|
||||
*
|
||||
* Builds a component whose main holds several nested component instances, plus a
|
||||
* copy of it, then SWEEPS whether one of the copy's nested sub-heads is deleted,
|
||||
* asserting every remaining sub-head still references its positional slot in the
|
||||
* main (see {@link SlotIntegrity}).
|
||||
*
|
||||
* This case PASSES for every layout (none / flex / grid): deleting a sub-head of a
|
||||
* COPY only hides it (a deleted-subinstance), so the copy stays aligned and valid.
|
||||
* We initially suspected the layout (flex, then grid) was the trigger; it is not —
|
||||
* the copy-side delete is correct. The actual :missing-slot crash comes from a
|
||||
* MAIN-side edit; see {@link ../cases/caseE} (and the clj regression test
|
||||
* `common/test/.../comp_main_edit_breaks_copy_slots_test.cljc`). This case is kept
|
||||
* as a characterization guard that copy-side deletes never corrupt the copy.
|
||||
*/
|
||||
/** Sweeps copy sub-head deletion and grid reflow while preserving alignment with the main. */
|
||||
export function createTestCaseCopySubheadDeletePreservesSlots(): TestCase {
|
||||
const foundation = new OpCreateNestableComponent(
|
||||
new ContentCreationStrategySiblingInstances(NESTED_COUNT, BASELINE, LAYOUT)
|
||||
);
|
||||
const content = new ContentCreationStrategySiblingInstances(NESTED_COUNT, BASELINE, LAYOUT);
|
||||
const foundation = new OpCreateNestableComponent(content);
|
||||
// domain vocabulary for the generic roles: the outer component's main and its copy
|
||||
const outerMain = foundation.roles.mainInstance;
|
||||
const outerCopy = foundation.roles.copyInstance;
|
||||
|
||||
// the copy's first nested sub-head, resolved at apply-time
|
||||
const firstSubhead = (s: Situation): Shape => (s.get(outerCopy).children ?? [])[0];
|
||||
// Resolve the copy's boundary sub-heads at apply time.
|
||||
const firstSubhead = (s: Situation): Shape => content.getSibling(s.get(outerCopy), 0);
|
||||
const lastSubhead = (s: Situation): Shape => content.getSibling(s.get(outerCopy), NESTED_COUNT - 1);
|
||||
const deleteFirstSubhead = new OpDeleteShape(firstSubhead, "first copy sub-head");
|
||||
const deleteLastSubhead = new OpDeleteShape(lastSubhead, "last copy sub-head");
|
||||
|
||||
// Resizing the copy root reflows its grid children.
|
||||
const reflowCopy = new OpChangeProperty(outerCopy, new ShapePropHeight(), REFLOW_HEIGHT, "copy root");
|
||||
|
||||
return new TestCase(
|
||||
"CopySubheadDeletePreservesSlots",
|
||||
"A component whose main holds several nested component instances is created, plus a copy " +
|
||||
"of it. One of the copy's nested sub-heads is optionally deleted. Every remaining " +
|
||||
"sub-head of the copy must still reference the slot at its position in the main — a " +
|
||||
"copy-side delete must not corrupt the positional slot matching that Penpot's file " +
|
||||
"validation enforces.",
|
||||
"A grid component with nested instances and a copy is created; a boundary copy sub-head " +
|
||||
"is optionally deleted and the copy optionally reflowed; remaining copy sub-heads " +
|
||||
"must stay positionally aligned with the main.",
|
||||
new OpSequence(
|
||||
foundation,
|
||||
foundation.createOpInstantiate(),
|
||||
// sweep with/without the deletion (the delete variant is the repro)
|
||||
new OpOptional(deleteFirstSubhead),
|
||||
// Optionally delete the first or last copy sub-head.
|
||||
new OpOptional(new OpOneOf(deleteFirstSubhead, deleteLastSubhead)),
|
||||
// Optionally reflow the grid after deletion.
|
||||
new OpOptional(reflowCopy),
|
||||
new OpAssert("every copy sub-head still references its positional slot in the main", (s) => {
|
||||
SlotIntegrity.assertAligned(s.get(outerCopy) as Board, s.get(outerMain) as Board);
|
||||
})
|
||||
|
||||
@ -12,53 +12,26 @@ import { SlotIntegrity } from "../util/SlotIntegrity";
|
||||
const BASELINE = new Color("#aaaaaa");
|
||||
const NESTED_COUNT = 3;
|
||||
|
||||
/**
|
||||
* Case E — reordering a sub-head IN THE MAIN must not break copies.
|
||||
*
|
||||
* This is the ACTUAL cause of the referential-integrity crash (:missing-slot).
|
||||
* `find-near-match` matches a copy's nested sub-heads to the main's children BY
|
||||
* POSITION. Reordering a sub-head inside the MAIN changes that order, but the
|
||||
* copies keep their shape-refs and are NOT given swap slots — so every copy is
|
||||
* now "swapped" relative to its position without a slot -> validation fails.
|
||||
*
|
||||
* The layout is irrelevant (the equivalent clj test reproduces it with no layout);
|
||||
* the trigger is purely the main-side reorder. The copy-side edits in case D are
|
||||
* correct — it's this main-side edit that corrupts.
|
||||
*
|
||||
* ⚠ WARNING — this case reproduces the bug through the LIVE app, and the app
|
||||
* currently CHOKES on the corrupt state: reordering a main sub-head via the Plugin
|
||||
* API hangs (and in the workspace UI it crashes the document). Because the test
|
||||
* runner lives inside that same app, running this case will HANG the panel — it
|
||||
* cannot report a normal pass/fail until the underlying bug is fixed. It is
|
||||
* included as an executable, documented reproduction, NOT as a routine test; do
|
||||
* NOT include it in a "run all". The reliable, non-hanging capture of this exact
|
||||
* bug is the clj test `common/test/.../comp_main_edit_breaks_copy_slots_test.cljc`.
|
||||
* Once Penpot propagates swap slots to copies on a main reorder, this case will
|
||||
* complete and the assertion will pass.
|
||||
*/
|
||||
/** Verifies that main-side reordering realigns copies without swap slots. */
|
||||
export function createTestCaseMainReorderKeepsCopySlots(): TestCase {
|
||||
// layout "none": the bug does not depend on the layout, only on the main reorder
|
||||
const foundation = new OpCreateNestableComponent(
|
||||
new ContentCreationStrategySiblingInstances(NESTED_COUNT, BASELINE)
|
||||
);
|
||||
// domain vocabulary for the generic roles: the outer component's main and its copy
|
||||
// Reorder behavior is independent of layout.
|
||||
const content = new ContentCreationStrategySiblingInstances(NESTED_COUNT, BASELINE);
|
||||
const foundation = new OpCreateNestableComponent(content);
|
||||
// Bind the generic roles to the outer component instances.
|
||||
const outerMain = foundation.roles.mainInstance;
|
||||
const outerCopy = foundation.roles.copyInstance;
|
||||
|
||||
// the MAIN's first nested sub-head, resolved at apply-time
|
||||
const firstMainSubhead = (s: Situation): Shape => (s.get(outerMain).children ?? [])[0];
|
||||
// Resolve the main's first nested sub-head at apply time.
|
||||
const firstMainSubhead = (s: Situation): Shape => content.getSibling(s.get(outerMain), 0);
|
||||
|
||||
return new TestCase(
|
||||
"MainReorderKeepsCopySlots",
|
||||
"A component whose main holds several nested component instances is created, plus a copy " +
|
||||
"of it. A nested sub-instance is then reordered INSIDE THE MAIN, which changes the " +
|
||||
"positional matching that copies rely on. Penpot must keep the copies' slot references " +
|
||||
"valid (assigning swap slots where needed); every copy sub-head must still reference " +
|
||||
"its positional slot in the main afterwards.",
|
||||
"A component with nested instances and a copy is created; the first main sub-head is moved " +
|
||||
"to the end; component sync must restore positional alignment without swap slots.",
|
||||
new OpSequence(
|
||||
foundation,
|
||||
foundation.createOpInstantiate(),
|
||||
// move the main's first sub-head to the end — the corrupting operation
|
||||
// Move the main's first sub-head to the end.
|
||||
new OpReorderShape(firstMainSubhead, NESTED_COUNT - 1, "first MAIN sub-head"),
|
||||
new OpAssert("every copy sub-head still references its positional slot in the main", (s) => {
|
||||
SlotIntegrity.assertAligned(s.get(outerCopy) as Board, s.get(outerMain) as Board);
|
||||
|
||||
@ -1,4 +1,4 @@
|
||||
import { Board } from "@penpot/plugin-types";
|
||||
import { Board, Shape } from "@penpot/plugin-types";
|
||||
import { Situation } from "../core/Situation";
|
||||
import { Color } from "../model/Color";
|
||||
import { ContentCreationStrategy } from "./ContentCreationStrategy";
|
||||
@ -23,6 +23,8 @@ export type SiblingLayout = "none" | "flex" | "grid";
|
||||
* is offered to keep the sweep space available, not because it is known to break.
|
||||
*/
|
||||
export class ContentCreationStrategySiblingInstances extends ContentCreationStrategy {
|
||||
static readonly SIBLING_NAME = "Icon";
|
||||
|
||||
/**
|
||||
* @param count - how many sibling instances of the inner component to append
|
||||
* @param baselineColor - the inner rectangle's fill colour
|
||||
@ -39,7 +41,7 @@ export class ContentCreationStrategySiblingInstances extends ContentCreationStra
|
||||
createContent(_situation: Situation, board: Board): void {
|
||||
// inner component: a small board with a single rectangle
|
||||
const innerBoard = penpot.createBoard();
|
||||
innerBoard.name = "Icon";
|
||||
innerBoard.name = ContentCreationStrategySiblingInstances.SIBLING_NAME;
|
||||
innerBoard.resize(24, 24);
|
||||
const rect = penpot.createRectangle();
|
||||
rect.name = "rect";
|
||||
@ -64,4 +66,16 @@ export class ContentCreationStrategySiblingInstances extends ContentCreationStra
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/** Returns the sibling instance at `index` inside a component instance. */
|
||||
getSibling(instance: Board, index: number): Shape {
|
||||
const siblings = (instance.children ?? []).filter(
|
||||
(shape) => shape.name === ContentCreationStrategySiblingInstances.SIBLING_NAME
|
||||
);
|
||||
const sibling = siblings[index];
|
||||
if (sibling === undefined) {
|
||||
throw new Error(`No sibling instance at index ${index} inside "${instance.name}"`);
|
||||
}
|
||||
return sibling;
|
||||
}
|
||||
}
|
||||
|
||||
@ -1,31 +1,11 @@
|
||||
import { Board, Shape } from "@penpot/plugin-types";
|
||||
import { Assert } from "./Assert";
|
||||
|
||||
/**
|
||||
* Checks the positional swap-slot invariant that Penpot's file validator enforces
|
||||
* for component copies.
|
||||
*
|
||||
* Background: in a copy of a component, each nested sub-instance head must, by
|
||||
* position, reference (its `shape-ref`) the child at the SAME index in the
|
||||
* component's main. Penpot finds the "near match" purely by position
|
||||
* (`find-near-match` in `common/.../types/file.cljc`) and then requires a swap
|
||||
* slot whenever a sub-head's `shape-ref` is not that positional near match
|
||||
* (`check-required-swap-slot` in `common/.../files/validate.cljc`, error
|
||||
* `:missing-slot` — "Shape has been swapped, should have swap slot").
|
||||
*
|
||||
* When the copy's child order diverges from the main's (e.g. after deleting one
|
||||
* sub-head, which shifts the rest) while their shape-refs still point to their
|
||||
* original slots, the invariant breaks and — with no swap slot recorded — the file
|
||||
* fails referential-integrity validation, crashing the project.
|
||||
*
|
||||
* This helper replicates that positional check through the Plugin API so a test
|
||||
* can assert the invariant holds.
|
||||
*/
|
||||
/** Checks steady-state positional alignment between a component copy and its main. */
|
||||
export class SlotIntegrity {
|
||||
/**
|
||||
* Returns the indices of `copyRoot`'s children whose `shape-ref` does not point
|
||||
* to the child at the same index in `mainRoot` (the positional near match) —
|
||||
* i.e. the sub-heads that would require a swap slot.
|
||||
* Returns copy-child indices whose references differ from the main child at
|
||||
* the same index.
|
||||
*/
|
||||
static misalignedIndices(copyRoot: Board, mainRoot: Board): number[] {
|
||||
const copyKids: Shape[] = copyRoot.children ?? [];
|
||||
@ -49,7 +29,7 @@ export class SlotIntegrity {
|
||||
Assert.that(
|
||||
bad.length === 0,
|
||||
`copy sub-heads must reference their positional slot in the main; ` +
|
||||
`mismatch (missing swap slot) at indices [${bad.join(", ")}]`
|
||||
`misaligned indices [${bad.join(", ")}]`
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user