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

🐛 Fix how changes touch copies and mark components

Browse Source 
Fixes in how a change touches a copy shape and marks its component modified
(Phase 3c).

- Give a copy shape back :remote-synced when its touch is undone.

  Touching a copy shape clears its :remote-synced (the mark that it was last
  synced from a remote main, which update main reads). The undo that
  pcb/update-shapes writes set the touched groups back but not that mark, so
  undoing an edit left the shape without it (F25).

  The undo now sets :remote-synced back when the shape had it.

  comp-touched-test: undoing an edit of a remote-synced copy shape gives the
  mark back and leaves the shape untouched.

- Apply an update's touched groups before the attrs it changes.

  When the function given to update-shapes set a copy shape's :touched and
  changed an attribute that touches, the order of the two operations
  followed the order of the shape's keys: a record field such as :name came
  before :touched, so the :set of :touched ran last and dropped the group
  the name had just touched (F24); keys in the shape's map came in hash
  order.

  The :set of :touched now always goes first, so the update's touched groups
  are the base and each attribute it changes touches on top.

  comp-touched-test: an update that sets the touched groups and changes the
  fills, opacity, strokes, visibility, blocking, a radius or the name keeps
  both.

- Count an :assign on a main as a change of its component.

  components-changed, which decides which components a commit changed and so
  need their copies synced, looked only at :set operations, so a :mod-obj
  with an :assign of synced attributes on a main never started a sync (F28).
  Nothing in the code writes :assign today, so no user hit it. An :assign
  now counts when any of its attributes is one that component sync reads.

  comp-sync-test: a :set and an :assign of opacity on a main child report
  its component; an :assign of interactions reports none.

- Mark a component modified only by changes copies can show.

  A change to a main marks its component modified, which tells the files
  that use the library to offer an update. set-shape-attr left in the
  shape's metadata whether its attribute is one that sync reads and changed,
  and the change was judged by that flag (F21):
  - :position-data, which a text re-measures from its content, counted, so
    re-measuring a main text prompted every file to update; decision (e) of
    the component contract says it never does;
  - each :set overwrote the flag, so a change that set a synced attribute
    and then the position data counted only the last one.

  :position-data no longer counts, and the flag is cleared at the start of
  each change and set by any of its :set operations.

  comp-sync-test: a fill change marks the component, a re-measure does not,
  and a fill change followed by a re-measure does.

- Compare transforms and paths of a copy with a tolerance.

  A change to a copy touches the group of the attribute when the value
  differs from the one it has. Numbers, selection rects, points and position
  data are compared with a tolerance, but the transform, its inverse and the
  content of a path were compared exactly, so float noise (a transform
  recomputed, a path moved there and back) touched :geometry-group (F23).
  The matrices are now compared term by term with the precision of the
  matrix helpers, as their scale and rotation terms need, and the paths
  segment by segment, with the precision the caller gives.

  container-test: float noise in a transform, its inverse or a path touches
  nothing; a rotation or a moved point still touches.

Benchmark (lower quantile, ms; baseline / now / ratio):
  deepest-edit-through-three-levels    35.2 /  19.0 / 0.54
  edit-through-a-chain-of-dependents   68.9 /  34.6 / 0.50
  library-update-into-copies           83.1 /  43.8 / 0.53
  main-edit-among-many-shapes           4.8 /   2.5 / 0.51
  main-edit-into-10-copies              1.8 /   0.9 / 0.48
  main-edit-into-100-copies            15.4 /   9.7 / 0.63
  main-edit-into-1000-copies          153.1 /  85.6 / 0.56
  variant-switch-into-copies           32.1 /  20.5 / 0.64

AI-assisted-by: claude-opus-5-5
This commit is contained in:
alonso.torres 2026-10-15 15:10:57 +02:00
parent 956df575f7
commit c7ef7b0088
7 changed files with 161 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
common/src/app/common/files/changes.cljc
View File

@ -625,7 +625,7 @@
(defn- process-operations
[objects {:keys [page-id id operations] :as change}]
(if-let [shape (get objects id)]
(let [shape (reduce process-operation shape operations)
(let [shape (reduce process-operation (vary-meta shape dissoc ::ctn/touched) operations)
touched? (-> shape meta ::ctn/touched)]
;; NOTE: processing operation functions can assign
;; the ::ctn/touched metadata on shapes, in this case we
@ -1170,8 +1170,10 @@
(let [need-sync? (fn [operation]
; We need to trigger a sync if the shape has changed any
; attribute that participates in components synchronization.
(and (= (:type operation) :set)
(ctk/component-attr? (:attr operation))))
(case (:type operation)
:set (ctk/component-attr? (:attr operation))
:assign (some ctk/component-attr? (keys (:value operation)))
false))
any-sync? (some need-sync? operations)]
(when any-sync?
(if page-id

15
common/src/app/common/files/changes_builder.cljc
View File

@ -629,12 +629,21 @@
new-obj (if with-objects? (update-fn old-obj objects) (update-fn old-obj))]
(if (= old-obj new-obj)
changes
(let [[rops uops] (-> (or attrs (d/concat-set (keys old-obj) (keys new-obj)))
(generate-operations old-obj new-obj))
(let [attrs (or attrs (d/concat-set (keys old-obj) (keys new-obj)))
;; the touched groups the update sets go first, so the
;; attributes it changes touch on top of them
attrs (if (some #{:touched} attrs)
(cons :touched (remove #{:touched} attrs))
attrs)
[rops uops] (generate-operations attrs old-obj new-obj)
;; a touch also clears :remote-synced
uops (cond-> uops
(seq uops)
(conj {:type :set-touched :touched (:touched old-obj)}))
(conj {:type :set-touched :touched (:touched old-obj)})
(and (seq uops) (:remote-synced old-obj))
(conj {:type :set-remote-synced :remote-synced true}))
change (cond-> {:type :mod-obj :id id}
(some? page-id)

29
common/src/app/common/geom/shapes.cljc
View File

@ -8,6 +8,7 @@
(:require
[app.common.data :as d]
[app.common.data.macros :as dm]
[app.common.geom.matrix :as gmt]
[app.common.geom.point :as gpt]
[app.common.geom.rect :as grc]
[app.common.geom.shapes.common :as gco]
@ -16,7 +17,8 @@
[app.common.geom.shapes.fit-frame :as gsff]
[app.common.geom.shapes.intersect :as gsi]
[app.common.geom.shapes.transforms :as gtr]
[app.common.math :as mth]))
[app.common.math :as mth]
[app.common.types.path :as path]))
;; --- Outer Rect
@ -145,6 +147,31 @@
(close-val? (:y (first %)) (:y (second %))))
(d/zip val1 val2))
;; a matrix holds scale and rotation terms, so it is compared with a
;; precision of its own
(and (or (= attr :transform) (= attr :transform-inverse))
(or (nil? val1) (gmt/matrix? val1))
(or (nil? val2) (gmt/matrix? val2)))
(gmt/close? (or val1 (gmt/matrix)) (or val2 (gmt/matrix)))
(and (= attr :content) (path/content? val1) (path/content? val2))
(let [segments1 (vec val1)
segments2 (vec val2)
close-params?
(fn [params1 params2]
(and (= (set (keys params1)) (set (keys params2)))
(every? (fn [[k v]]
(let [other (get params2 k)]
(if (and (number? v) (number? other))
(close-val? v other)
(= v other))))
params1)))]
(and (= (count segments1) (count segments2))
(every? (fn [[s1 s2]]
(and (= (:command s1) (:command s2))
(close-params? (:params s1) (:params s2))))
(d/zip segments1 segments2))))
:else
(= val1 val2)))))

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

@ -606,8 +606,9 @@
(defn set-shape-attr
"Assign attribute to shape with touched logic.
The returned shape will contain a metadata associated with it
indicating if shape is touched or not."
The returned shape carries in its metadata (`::touched`) whether this or
an earlier attribute set on it changed something component sync reads;
`:position-data`, re-measured from the content, never does."
[shape attr val & {:keys [ignore-touched ignore-geometry]}]
(let [type (get shape :type)
group (ctk/resolve-sync-group type attr)
@ -643,6 +644,7 @@
touched?
(and group
(not equal?)
(not= attr :position-data)
(not (and ignore-geometry is-geometry?)))
;; Depending on the origin of the attribute change, we need or not to
@ -686,4 +688,4 @@
(assoc attr val)
:always
(vary-meta assoc ::touched touched?))))
(vary-meta update ::touched #(or % touched?)))))

39
common/test/common_tests/logic/comp_sync_test.cljc
View File

@ -7,6 +7,7 @@
(ns common-tests.logic.comp-sync-test
(:require
[app.common.data :as d]
[app.common.files.changes :as ch]
[app.common.files.changes-builder :as pcb]
[app.common.geom.point :as gpt]
[app.common.geom.shapes :as gsh]
@ -19,6 +20,7 @@
[app.common.test-helpers.shapes :as ths]
[app.common.types.component :as ctk]
[app.common.types.container :as ctn]
[app.common.types.file :as ctf]
[app.common.types.shape-tree :as ctst]
[clojure.test :as t]))
@ -580,3 +582,40 @@
;; ==== Check
(t/is (true? (:hidden main-child2')))
(t/is (= [(thi/id :main-child2) (thi/id :main-child1)] (:shapes main-root')))))
(t/deftest test-an-assign-on-a-main-changes-its-component
(let [file (-> (thf/sample-file :file1)
(tho/add-simple-component-with-copy :component1 :main-root :main-child :copy-root))
page (thf/current-page file)
change (fn [operation]
{:type :mod-obj
:id (thi/id :main-child)
:page-id (:id page)
:operations [operation]})]
(t/is (= #{(thi/id :component1)}
(ch/components-changed (:data file) (change {:type :set :attr :opacity :val 0.5}))))
(t/is (= #{(thi/id :component1)}
(ch/components-changed (:data file) (change {:type :assign :value {:opacity 0.5}}))))
(t/is (nil? (ch/components-changed (:data file) (change {:type :assign :value {:interactions []}}))))))
(t/deftest test-which-changes-of-a-main-mark-its-component-modified
(let [file (-> (thf/sample-file :file1)
(tho/add-simple-component :component1 :main-root :main-child)
(ths/update-shape :main-child :position-data [{:x 0 :y 0 :width 3 :height 4 :text "a"}])
(ctf/update-file-data #(assoc-in % [:components (thi/id :component1) :modified-at] :old)))
page (thf/current-page file)
apply-op (fn [operations]
(-> (thf/apply-changes file {:redo-changes [{:type :mod-obj
:id (thi/id :main-child)
:page-id (:id page)
:operations operations}]}
:validate? false)
(thc/get-component :component1)
:modified-at))
fills {:type :set :attr :fills :val (ths/sample-fills-color :fill-color "#ff0000")}
pos-data {:type :set :attr :position-data :val [{:x 1 :y 2 :width 3 :height 4 :text "a"}]}]
(t/is (not= :old (apply-op [fills])))
(t/testing "a re-measured text is not a change of the component"
(t/is (= :old (apply-op [pos-data]))))
(t/testing "any synced attribute of the change counts, not only the last"
(t/is (not= :old (apply-op [fills pos-data]))))))

43
common/test/common_tests/logic/comp_touched_test.cljc
View File

@ -289,3 +289,46 @@
(t/is (= (:fill-opacity fill') 1))
(t/is (= (:touched copy2-root') nil))
(t/is (= (:touched copy2-child') #{:fill-group}))))
(t/deftest test-undo-of-a-touch-gives-back-remote-synced
;; a copy shape synced from a remote main (after a reset below the copy
;; root, say) loses :remote-synced when it is touched
(let [file (-> (thf/sample-file :file1)
(tho/add-simple-component-with-copy :component1 :main-root :main-child :copy-root
:copy-root-params {:children-labels [:copy-child]})
(ths/update-shape :copy-child :remote-synced true))
page (thf/current-page file)
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
#{(thi/id :copy-child)}
#(assoc % :opacity 0.5)
(:objects page)
{})
file' (thf/apply-changes file changes)
file'' (thf/apply-undo-changes file' changes)]
(t/is (nil? (:remote-synced (ths/get-shape file' :copy-child))))
(t/is (true? (:remote-synced (ths/get-shape file'' :copy-child))))
(t/is (nil? (:touched (ths/get-shape file'' :copy-child))))))
(t/deftest test-touched-set-by-an-update-then-its-attrs-touch
;; an update sets the touched groups of a copy shape and changes an
;; attribute: the attribute touches its group on top of them, whatever the
;; order of the shape's keys
(doseq [[attr value group] [[:fills (ths/sample-fills-color :fill-color "#ff0000") :fill-group]
[:opacity 0.5 :layer-effects-group]
[:strokes [{:stroke-color "#ff0000" :stroke-opacity 1 :stroke-width 2}] :stroke-group]
[:hidden true :visibility-group]
[:blocked true :modifiable-group]
[:r1 5 :radius-group]
[:name "Renamed" :name-group]]]
(let [file (-> (thf/sample-file :file1)
(tho/add-simple-component-with-copy :component1 :main-root :main-child :copy-root
:copy-root-params {:children-labels [:copy-child]}))
page (thf/current-page file)
changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
#{(thi/id :copy-child)}
#(assoc % attr value :touched #{:shadow-group})
(:objects page)
{})
file' (thf/apply-changes file changes)]
(t/is (= #{:shadow-group group} (:touched (ths/get-shape file' :copy-child))) (name attr)))))

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

@ -6,6 +6,7 @@
(ns common-tests.types.container-test
(:require
[app.common.geom.matrix :as gmt]
[app.common.geom.point :as gpt]
[app.common.test-helpers.components :as thc]
[app.common.test-helpers.compositions :as tho]
@ -16,6 +17,7 @@
[app.common.types.component :as ctk]
[app.common.types.components-list :as ctkl]
[app.common.types.container :as ctc]
[app.common.types.path :as path]
[app.common.types.shape :as cts]
[app.common.uuid :as uuid]
[clojure.test :as t]))
@ -423,3 +425,30 @@
(t/is (= #{:objects} (ctk/diff-components component changed)))
(t/is (= #{} (ctk/diff-components component component)))))
(defn- copy-shape
"A shape of a copy: one with a ref to its main."
[& {:as attrs}]
(assoc (make-shape (uuid/next) attrs) :shape-ref (uuid/next)))
(defn- touched-by
[shape attr val]
(:touched (ctc/set-shape-attr shape attr val)))
(t/deftest float-noise-in-a-transform-touches-nothing
(let [shape (copy-shape)
noisy (-> (gmt/matrix)
(assoc :a 1.0000000001 :b 1e-10))]
(t/is (nil? (touched-by shape :transform noisy)))
(t/is (nil? (touched-by shape :transform-inverse noisy)))
(t/is (= #{:geometry-group}
(touched-by shape :transform (gmt/rotate-matrix 30))))))
(t/deftest float-noise-in-a-path-touches-nothing
(let [content (fn [x]
(path/content [{:command :move-to :params {:x 0 :y 0}}
{:command :line-to :params {:x x :y 10}}]))
shape (copy-shape :type :path :content (content 10))]
(t/is (nil? (touched-by shape :content (content 10.01))))
(t/is (= #{:geometry-group}
(touched-by shape :content (content 30))))))