github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-29 14:00:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

🔥 Remove dead code from the component helpers

Browse Source 
- generate-relocate set :component-root on the moved shapes twice. The
  second pass looked at the same copy heads again and wrote the values
  the first had written, and update-shapes had already applied them,
  so it emitted nothing. It goes. Delete and relocate found the masked
  groups whose mask leaves them with the same reduce;
  groups-losing-mask finds them for both. The comment "Remove the hide
  in viewer flag" sat above code that sets it.
- generate-duplicate-shape-change took unames, update-unames! and ids
  only to pass them to its own recursive call, assigned each shape the
  name it already had, and had a 13-argument arity that nothing
  called. It takes the values that stay the same along the subtree in
  one map, and it is private.
- make-component-instance collected the names used in the page and
  the new ids in two volatiles: the names were never read, and the ids
  only fed a second remap of the grid cells, which clone-shape already
  does.
- ctk/in-component-copy-not-root? had no caller, and
  ctn/get-container-root and ctk/valid-touched-group? only their
  tests.
- ctf/swapped-subhead? tells whether a copy head refers to a main
  shape that is not a child of the main shape its parent refers to;
  its docstring still spoke of swap slots. It is
  ref-outside-near-main?.
- dump-shape-component-info built the name of a nested head's
  component in a branch whose condition ended in `true`, so it never
  ran.

No behavior change: the golden masters and the suites are unchanged.

Benchmark (lower quantile, ms; baseline / now / ratio):
  deepest-edit-through-three-levels    35.2 /  16.1 / 0.46
  edit-through-a-chain-of-dependents   68.9 /  30.1 / 0.44
  library-update-into-copies           83.1 /  39.4 / 0.47
  main-edit-among-many-shapes           4.8 /   2.2 / 0.45
  main-edit-into-10-copies              1.8 /   0.8 / 0.44
  main-edit-into-100-copies            15.4 /   7.7 / 0.50
  main-edit-into-1000-copies          153.1 /  78.1 / 0.51
  variant-switch-into-copies           32.1 /  18.5 / 0.58

AI-assisted-by: claude-opus-5-5

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
alonso.torres 2026-10-14 16:04:16 +02:00
parent d0ffa6e411
commit ae22eaa296
10 changed files with 54 additions and 199 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
common/src/app/common/files/comp_processors.cljc
View File

@ -165,7 +165,8 @@
file-data)))
(defn fix-missing-swap-slots
"Locate shapes that have been swapped (see `ctf/swapped-subhead?`) but don't have a swap slot.
"Locate shapes that have been swapped (their ref is outside the near main, see
`ctf/ref-outside-near-main?`) but don't have a swap slot.
In this case, add one pointing to the near match."
[file-data libraries]
(try
@ -175,7 +176,7 @@
(if (ctk/subcopy-head? shape)
(let [container (:container (meta shape))
file {:id (:id file-data) :data file-data}
swapped? (ctf/swapped-subhead?
swapped? (ctf/ref-outside-near-main?
shape container
#(ctf/find-ref-shape file container libraries % :include-deleted? true))
near-match (when swapped?

4
common/src/app/common/files/validate.cljc
View File

@ -409,8 +409,8 @@
"Validate that a nested copy head refers to a child of the near main of
its parent: the shape whose position it takes."
[shape file page libraries]
(when (ctf/swapped-subhead? shape page #(ctf/find-content-ref-shape file page libraries %
:include-deleted? true))
(when (ctf/ref-outside-near-main? shape page #(ctf/find-content-ref-shape file page libraries %
:include-deleted? true))
(report-error :ref-not-in-near-main
(str/ffmt "Referenced shape % is not a child of the near main of the parent" (:shape-ref shape))
shape file page)))

59
common/src/app/common/logic/duplicate.cljc
View File

@ -309,13 +309,17 @@
{}))]
changes))
(defn generate-duplicate-shape-change
([changes objects page unames update-unames! ids ids-map obj delta level-delta libraries library-data file-id]
(generate-duplicate-shape-change changes objects page unames update-unames! ids ids-map obj delta level-delta libraries library-data file-id (:frame-id obj) (:parent-id obj) false false nil))
([changes objects page unames update-unames! ids ids-map obj delta level-delta libraries library-data file-id variant-props]
(generate-duplicate-shape-change changes objects page unames update-unames! ids ids-map obj delta level-delta libraries library-data file-id (:frame-id obj) (:parent-id obj) false false variant-props))
([changes objects page unames update-unames! ids ids-map obj delta level-delta libraries library-data file-id frame-id parent-id duplicating-component? child? variant-props]
(defn- generate-duplicate-shape-change
"Add to `changes` a duplicate of `obj` and its subtree in `objects`, under
`parent-id` and `frame-id`, and `level-delta` nesting levels shallower
(computed when nil). `ctx` holds what stays the same along the subtree:
the `:page`, the new id of each shape (`:ids-map`), the `:delta` they
move, the `:libraries`, the `:library-data`, the `:file-id` and the
`:variant-props` of a duplicated variant."
([changes ctx objects obj]
(generate-duplicate-shape-change changes ctx objects obj nil (:frame-id obj) (:parent-id obj) false false))
([changes {:keys [page ids-map delta libraries library-data file-id variant-props] :as ctx}
objects obj level-delta frame-id parent-id duplicating-component? child?]
(cond
(nil? obj)
changes
@ -330,7 +334,6 @@
new-id (ids-map (:id obj))
parent-id (or parent-id frame-id)
parent (get objects parent-id)
name (:name obj)
is-component-root? (or (:saved-component-root obj)
;; Backward compatibility
@ -365,7 +368,6 @@
new-obj
(-> obj
(assoc :id new-id
:name name
:parent-id parent-id
:frame-id frame-id)
@ -431,24 +433,15 @@
(reduce (fn [changes child]
(generate-duplicate-shape-change changes
ctx
(:objects page')
page
unames
update-unames!
ids
ids-map
child
delta
;; the children of a swapped head refer into its own component
(if (ctk/swapped? obj) 0 level-delta)
libraries
library-data
file-id
(if frame? new-id frame-id)
new-id
duplicating-component?
true
variant-props))
true))
changes
(map (d/getf objects) (:shapes obj)))))))
@ -502,12 +495,10 @@
(mapcat #(ctn/get-child-heads objects %) selected))))
(defn generate-duplicate-changes
"Prepare objects to duplicate: generate new id, give them unique names,
move to the desired position, and recalculate parents and frames as needed."
"Prepare objects to duplicate: generate new ids, move them to the desired
position, and recalculate parents and frames as needed."
[changes all-objects page ids delta libraries library-data file-id & {:keys [variant-props alt-duplication?]}]
(let [shapes (map (d/getf all-objects) ids)
unames (volatile! (cfh/get-used-names (:objects page)))
update-unames! (fn [new-name] (vswap! unames conj new-name))
all-ids (reduce #(into %1 (cons %2 (cfh/get-children-ids all-objects %2))) (d/ordered-set) ids)
;; We need ids-map for remapping the grid layout. But when duplicating the guides
@ -535,19 +526,15 @@
changes
(->> shapes
(reduce #(generate-duplicate-shape-change %1
{:page page
:ids-map ids-map
:delta delta
:libraries libraries
:library-data library-data
:file-id file-id
:variant-props variant-props}
all-objects
page
unames
update-unames!
ids
ids-map
%2
delta
nil
libraries
library-data
file-id
variant-props)
%2)
changes))
;; We need to check the changes to get the ids-map

79
common/src/app/common/logic/shapes.cljc
View File

@ -232,6 +232,17 @@
changes
copy-deletions))
(defn- groups-losing-mask
"The masked groups whose mask, their first child, is among `ids`."
[objects ids]
(into #{}
(keep (fn [id]
(let [parent (get objects (:parent-id (get objects id)))]
(when (and (:masked-group parent)
(= id (first (:shapes parent))))
(:id parent)))))
ids))
(defn generate-delete-shapes
([changes file page objects ids options]
(generate-delete-shapes (-> changes
@ -288,21 +299,10 @@
lookup (d/getf objects)
;; a group whose mask is deleted becomes a plain group
groups-to-unmask
(when-not ignore-mask
(reduce (fn [group-ids id]
;; When the shape to delete is the mask of a masked group,
;; the mask condition must be removed, and it must be
;; converted to a normal group.
(let [obj (lookup id)
parent (lookup (:parent-id obj))]
(if (and (:masked-group parent)
(= id (first (:shapes parent))))
(conj group-ids (:id parent))
group-ids)))
#{}
ids-to-delete)
[])
(groups-losing-mask objects ids-to-delete))
id-to-delete? (set ids-to-delete)
@ -495,46 +495,9 @@
removed-id?
result)))))
;; a group whose mask moves out of it becomes a plain group
groups-to-unmask
(reduce (fn [group-ids id]
;; When a masked group loses its mask shape, because it's
;; moved outside the group, the mask condition must be
;; removed, and it must be converted to a normal group.
(let [obj (get objects id)
parent (get objects (:parent-id obj))]
(if (and (:masked-group parent)
(= id (first (:shapes parent)))
(not= (:id parent) parent-id))
(conj group-ids (:id parent))
group-ids)))
#{}
ids)
;; TODO: Probably implementing this using loop/recur will
;; be more efficient than using reduce and continuous data
;; desturcturing.
;; Sets the correct components metadata for the moved copy heads
;; `shapes-to-deroot` Removes the root flag from a component instance moved inside another component
;; `shapes-to-reroot` Adds a root flag when a nested component instance is moved outside
[shapes-to-deroot shapes-to-reroot]
(reduce (fn [[shapes-to-deroot shapes-to-reroot] id]
(let [shape (get objects id)
parent (get objects parent-id)
component-shape-parent (ctn/get-component-shape objects parent {:allow-main? true})
root-parent (ctn/get-instance-root objects parent)
deroot? (and (ctk/instance-root? shape)
root-parent)
reroot? (and (ctk/subinstance-head? shape)
(not component-shape-parent))]
[(cond-> shapes-to-deroot deroot? (conj id))
(cond-> shapes-to-reroot reroot? (conj id))]))
[[] []]
(->> ids
(mapcat #(ctn/get-child-heads objects %))
(map :id)))
(disj (groups-losing-mask objects ids) parent-id)
index-cell-data (when to-index (ctl/get-cell-by-index parent to-index))
cell (or cell (and index-cell-data [(:row index-cell-data) (:column index-cell-data)]))
@ -557,7 +520,7 @@
(-> (pcb/update-shapes ids ctl/remove-layout-item-data)
(pcb/update-shapes ids cto/unapply-layout-item-tokens)))
;; Remove the hide in viewer flag
;; Hide in the viewer the boards moved into a board
(cond-> (and (not= uuid/zero parent-id) (cfh/frame-shape? parent))
(pcb/update-shapes ids #(cond-> % (cfh/frame-shape? %) (assoc :hide-in-viewer true))))
@ -590,16 +553,6 @@
(fn [shape]
(assoc shape :masked-group false)))
;; Make non root a component moved inside another one
(pcb/update-shapes shapes-to-deroot
(fn [shape]
(assoc shape :component-root nil)))
;; Make root a subcomponent moved outside its parent component
(pcb/update-shapes shapes-to-reroot
(fn [shape]
(assoc shape :component-root true)))
;; Reset constraints depending on the new parent
(pcb/update-shapes ids
(fn [shape]

21
common/src/app/common/types/component.cljc
View File

@ -6,7 +6,6 @@
(ns app.common.types.component
(:require
[app.common.exceptions :as ex]
[app.common.schema :as sm]
[app.common.time :as-alias ct]
[app.common.types.page :as ctp]
@ -220,13 +219,6 @@
(and (some? (:shape-ref shape))
(nil? (:component-id shape))))
(defn in-component-copy-not-root?
"Check if the shape is inside a component non-main instance and
is not the root shape."
[shape]
(and (some? (:shape-ref shape))
(nil? (:component-root shape))))
(defn main-instance-of?
"Check if this shape is the root of the main instance of the given component."
[shape-id page-id component]
@ -332,16 +324,3 @@
(or (not (instance-head? shape))
(not (in-component-copy? parent))))))
(def ^:private all-touched-groups
(reduce-kv (fn [acc _ v]
(if (map? v)
(into acc (vals v))
(conj acc v)))
#{}
sync-attrs))
(defn valid-touched-group?
[group]
(ex/ignoring
(or (contains? all-touched-groups group)
(= group :swap-group))))

35
common/src/app/common/types/container.cljc
View File

@ -94,10 +94,6 @@
[container shape-id f]
(update-in container [:objects shape-id] f))
(defn get-container-root
[container]
(d/seek #(or (nil? (:parent-id %)) (= (:parent-id %) uuid/zero)) (shapes-seq container)))
(defn get-direct-children
[container shape]
(map #(get-shape container %) (:shapes shape)))
@ -300,7 +296,7 @@
(defn make-component-instance
"Generate a new instance of the component inside the given container.
Clone the shapes of the component, generating new names and ids, and
Clone the shapes of the component, generating new ids, and
linking each new shape to the corresponding one of the
component. Place the new instance coordinates in the given
position.
@ -324,7 +320,6 @@
delta (gpt/subtract position orig-pos)
objects (:objects page)
unames (volatile! (cfh/get-used-names objects))
component-children
(d/index-by :id (cfh/get-children-with-self objects (:id component-shape)))
@ -346,18 +341,9 @@
frame (get-shape page frame-id)
component-frame (get-component-shape objects frame {:allow-main? true})
ids-map (volatile! {})
update-new-shape
(fn [new-shape original-shape]
(let [new-name (:name new-shape)
root? (ctk/instance-root? original-shape)]
(when root?
(vswap! unames conj new-name))
(vswap! ids-map assoc (:id original-shape) (:id new-shape))
(let [root? (ctk/instance-root? original-shape)]
(cond-> new-shape
:always
(-> (gsh/move delta)
@ -378,8 +364,7 @@
(nil? (:parent-id original-shape))
(assoc :component-id (:id component)
:component-file (:id library-data)
:component-root true
:name new-name)
:component-root true)
(or (some? (:parent-id original-shape)) ; On v2 we have removed the parent-id for component roots
(some? component-frame))
@ -395,17 +380,13 @@
:frame-id frame-id
:dest-objects (:objects page))
;; Fix empty parent-id and remap all grid cells to the new ids.
remap-ids
;; the root has no parent in the component; it goes to its frame
fix-parent-id
(fn [shape]
(as-> shape $
(update $ :parent-id #(or % (:frame-id $)))
(cond-> $
(ctl/grid-layout? shape)
(ctl/remap-grid-cells @ids-map))))]
(update shape :parent-id #(or % (:frame-id shape))))]
[(remap-ids new-shape)
(map remap-ids new-shapes)])))
[(fix-parent-id new-shape)
(map fix-parent-id new-shapes)])))
(defn get-first-valid-parent
"Go through the parents until we find a shape that is not a copy of a component nor

21
common/src/app/common/types/file.cljc
View File

@ -496,7 +496,7 @@
(ctk/remove-swap-group)))
(d/update-when shape :shape-ref #(get ref-map % %))))))
(defn swapped-subhead?
(defn ref-outside-near-main?
"Whether `shape` refers to a main shape that is not a child of the main
shape its parent refers to (`find-parent-ref-shape`). Same-parent
positional differences are synchronized as reorders."
@ -961,7 +961,7 @@
shape
:include-deleted? true)]
(str/format " %s--> %s%s%s%s%s"
(str/format " %s--> %s%s%s%s"
(cond (:component-root shape) "#"
(:component-id shape) "@"
:else "-")
@ -984,23 +984,6 @@
(when (and show-ids component-shape)
(str/format " %s" (:id component-shape)))
(if (or (:component-root shape)
(nil? (:component-id shape))
true)
""
(let [component-id (:component-id shape)
component-file-id (:component-file shape)
component-file (when component-file-id (get libraries component-file-id nil))
component (if component-file
(ctkl/get-component (:data component-file) component-id true)
(ctkl/get-component (:data file) component-id true))]
(str/format " (%s%s)"
(when component-file (str/format "<%s> "
(if (= (:id component-file) (:id file))
"local"
(:name component-file))))
(:name component))))
(when (and show-ids (:component-id shape))
(str/format " [Component %s]" (:component-id shape)))))))

2
common/test/common_tests/runner.cljc
View File

@ -106,7 +106,6 @@
[common-tests.types-shape-tree-test]
[common-tests.types.absorb-assets-test]
[common-tests.types.color-test]
[common-tests.types.components-test]
[common-tests.types.container-test]
[common-tests.types.file-test]
[common-tests.types.fill-test]
@ -227,7 +226,6 @@
'common-tests.types-shape-tree-test
'common-tests.types.absorb-assets-test
'common-tests.types.color-test
'common-tests.types.components-test
'common-tests.types.container-test
'common-tests.types.file-test
'common-tests.types.fill-test

21
common/test/common_tests/types/components_test.cljc
View File

@ -1,21 +0,0 @@
;; 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 SUBSIDIARY SL
(ns common-tests.types.components-test
(:require
[app.common.test-helpers.ids-map :as thi]
[app.common.types.component :as ctk]
[clojure.test :as t]))
(t/use-fixtures :each thi/test-fixture)
(t/deftest test-valid-touched-group
(t/is (ctk/valid-touched-group? :name-group))
(t/is (ctk/valid-touched-group? :geometry-group))
(t/is (ctk/valid-touched-group? :swap-group))
(t/is (not (ctk/valid-touched-group? :this-is-not-a-group)))
(t/is (not (ctk/valid-touched-group? :swap-slot-9cc181fa-5eef-8084-8004-7bb2ab45fd1f)))
(t/is (not (ctk/valid-touched-group? nil))))

6
common/test/common_tests/types/container_test.cljc
View File

@ -374,12 +374,6 @@
(t/is (not (ctc/page? container)))
(t/is (= (thi/id :main1) (:main-instance-id container)))))
(t/deftest container-root
(let [root-id (uuid/next)
root (assoc (make-shape root-id) :parent-id nil)
child (make-shape (uuid/next) :parent-id root-id)]
(t/is (= root (ctc/get-container-root {:objects (objects-map root child)})))))
(t/deftest swap-keep-attrs-keep-auto-sizing-of-a-layout
(let [shape (make-shape (uuid/next)
:type :frame