From c7ef7b0088071871141b99bc491c06ca99b96e99 Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Thu, 15 Oct 2026 15:10:57 +0200 Subject: [PATCH] :bug: Fix how changes touch copies and mark components 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 --- common/src/app/common/files/changes.cljc | 8 ++-- .../src/app/common/files/changes_builder.cljc | 15 +++++-- common/src/app/common/geom/shapes.cljc | 29 ++++++++++++- common/src/app/common/types/container.cljc | 8 ++-- .../common_tests/logic/comp_sync_test.cljc | 39 +++++++++++++++++ .../common_tests/logic/comp_touched_test.cljc | 43 +++++++++++++++++++ .../common_tests/types/container_test.cljc | 29 +++++++++++++ 7 files changed, 161 insertions(+), 10 deletions(-) diff --git a/common/src/app/common/files/changes.cljc b/common/src/app/common/files/changes.cljc index 8e4bbb67d9..84ec2f2b94 100644 --- a/common/src/app/common/files/changes.cljc +++ b/common/src/app/common/files/changes.cljc @@ -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 diff --git a/common/src/app/common/files/changes_builder.cljc b/common/src/app/common/files/changes_builder.cljc index 11c9377a42..eebae2e2ef 100644 --- a/common/src/app/common/files/changes_builder.cljc +++ b/common/src/app/common/files/changes_builder.cljc @@ -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) diff --git a/common/src/app/common/geom/shapes.cljc b/common/src/app/common/geom/shapes.cljc index 4f0ac90fe9..7cfa86338a 100644 --- a/common/src/app/common/geom/shapes.cljc +++ b/common/src/app/common/geom/shapes.cljc @@ -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))))) diff --git a/common/src/app/common/types/container.cljc b/common/src/app/common/types/container.cljc index 3757b6c692..6d160cb25e 100644 --- a/common/src/app/common/types/container.cljc +++ b/common/src/app/common/types/container.cljc @@ -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?))))) diff --git a/common/test/common_tests/logic/comp_sync_test.cljc b/common/test/common_tests/logic/comp_sync_test.cljc index ff7aa1ee0e..4da84286c9 100644 --- a/common/test/common_tests/logic/comp_sync_test.cljc +++ b/common/test/common_tests/logic/comp_sync_test.cljc @@ -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])))))) diff --git a/common/test/common_tests/logic/comp_touched_test.cljc b/common/test/common_tests/logic/comp_touched_test.cljc index ff8ec9181a..40bf69268e 100644 --- a/common/test/common_tests/logic/comp_touched_test.cljc +++ b/common/test/common_tests/logic/comp_touched_test.cljc @@ -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))))) + diff --git a/common/test/common_tests/types/container_test.cljc b/common/test/common_tests/types/container_test.cljc index 6857a0c0fc..65d64b920d 100644 --- a/common/test/common_tests/types/container_test.cljc +++ b/common/test/common_tests/types/container_test.cljc @@ -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))))))