🐛 Ignore float noise when syncing moved library components (#12976)

.serena/memories/common/decimals-and-coordinates.md

@@ -52,3 +52,8 @@ its own drift. Comparing `:selrect.width` from shape A to
 `:selrect.width` from shape B is comparing two values that
 "semantically" should be equal but were computed via different
 operation chains — exact equality will often be false.
+
+## Library sync comparison
+
+- `update-attrs` (`app.common.logic.libraries`) compares main vs copy attrs exactly first, then with tolerance for: `drifting-geometry-attrs` (`:x :y :width :height :selrect :points`, via `gsh/close-attrs?`); path and bool `:content` (`segment/content-close?`; bool shapes keep their path in `:content`); and text `:position-data` (`position-data-close?` over `mth/close-map?`: every numeric field of every entry within `mth/close?`, other fields such as fills, fonts and text exact). All other attrs (`:opacity`, radii, `:transform`, `:rotation`, text `:content`) stay exact: tolerance there hides real changes.
+- `close-attrs?` pairs lists with `d/zip`, which truncates to the shorter list and treats nil vs vector as equal for `:points`/`:position-data`. Do not use it for variable-length data. That is why `:position-data` has its own helper, and why `attr-equal?` checks the `:points` count before calling it.

common/src/app/common/logic/libraries.cljc

@@ -58,6 +58,44 @@
        ;; We don't do automatic update of the `layout-grid-cells` property.
        (remove #(= :layout-grid-cells %))))
 
+;; Attrs whose value drifts by float noise when a shape is moved; they
+;; are compared with tolerance during sync. The rest compare exactly.
+(def ^:private drifting-geometry-attrs
+  #{:x :y :width :height :selrect :points})
+
+(defn- position-data-close?
+  "Compares text position-data entry by entry: numbers with tolerance,
+  every other field (fills, fonts, text) exactly."
+  [a b]
+  (let [a (vec a)
+        b (vec b)]
+    (and (= (count a) (count b))
+         (every? true? (map mth/close-map? a b)))))
+
+(defn- attr-equal?
+  [attr origin-shape dest-shape]
+  (let [origin-val (get origin-shape attr)
+        dest-val   (get dest-shape attr)]
+    (or (= origin-val dest-val)
+        (cond
+          ;; close-attrs? zips :points and would ignore a length mismatch
+          (= :points attr)
+          (and (= (count origin-val) (count dest-val))
+               (gsh/close-attrs? attr origin-val dest-val))
+
+          (contains? drifting-geometry-attrs attr)
+          (gsh/close-attrs? attr origin-val dest-val)
+
+          (and (= :content attr)
+               (or (cfh/path-shape? origin-shape) (cfh/bool-shape? origin-shape)))
+          (segment/content-close? origin-val dest-val)
+
+          (= :position-data attr)
+          (position-data-close? origin-val dest-val)
+
+          :else
+          false))))
+
 ;; Flex-child layout attrs synced between main and copy (interactions are only kept on swap)
 (def ^:private flex-child-attrs
   (disj ctk/swap-keep-attrs :interactions))
@@ -1993,7 +2031,7 @@
                 reset-pos-data?
                 (and (cfh/text-shape? origin-shape)
                      (= attr :position-data)
-                     (not= (:position-data origin-shape) (:position-data dest-shape))
+                     (not (attr-equal? :position-data origin-shape dest-shape))
                      (touched :geometry-group))
 
                 ;; On texts, when we want to omit the touched attrs, both text (the actual letters)
@@ -2007,7 +2045,7 @@
                      (touched sync-group))
 
                 skip-operations?
-                (or (= (get origin-shape attr) (get dest-shape attr))
+                (or (attr-equal? attr origin-shape dest-shape)
                     (and (touched sync-group)
                          omit-touched?
                          ;; When it is a text-partial-change, we should generate operations

common/src/app/common/math.cljc

@@ -208,6 +208,19 @@
   ([num1 num2 precision]
    (<= (abs (- num1 num2)) precision)))
 
+(defn close-map?
+  "Equality for maps: same keys, numeric values within `close?`, any other
+  value compared exactly."
+  [a b]
+  (and (= (count a) (count b))
+       (every? (fn [[k v]]
+                 (and (contains? b k)
+                      (let [w (get b k)]
+                        (if (number? v)
+                          (and (number? w) (close? v w))
+                          (= v w)))))
+               a)))
+
 (defn lerp
   "Calculates a the linear interpolation between two values and a given percent"
   [v0 v1 t]

common/src/app/common/types/path/segment.cljc

@@ -1355,6 +1355,20 @@
       (grc/points->rect extremities)
       (grc/make-rect))))
 
+(defn- segment-close?
+  [a b]
+  (and (= (:command a) (:command b))
+       (mth/close-map? (:params a) (:params b))))
+
+(defn content-close?
+  "True when both path contents have the same segments and every param
+  is within `mth/close?` of its counterpart."
+  [content-a content-b]
+  (let [a (vec content-a)
+        b (vec content-b)]
+    (and (= (count a) (count b))
+         (every? true? (map segment-close? a b)))))
+
 (defn content-center
   [content]
   (-> content

common/test/common_tests/logic/comp_sync_test.cljc

@@ -19,6 +19,7 @@
    [app.common.test-helpers.shapes :as ths]
    [app.common.types.component :as ctk]
    [app.common.types.container :as ctn]
+   [app.common.types.modifiers :as ctm]
    [app.common.types.shape-tree :as ctst]
    [clojure.test :as t]))
 
@@ -552,6 +553,330 @@
                                   {(:id file) file}
                                   (:id file)))
 
+(defn- changes-attrs
+  "Set of attrs touched by the :mod-obj changes of a changes map."
+  [changes]
+  (->> (:redo-changes changes)
+       (filter #(= :mod-obj (:type %)))
+       (mapcat :operations)
+       (map :attr)
+       (into #{})))
+
+(defn- file-with-path-component
+  []
+  (let [content [{:command :move-to :params {:x 10.3 :y 10.7}}
+                 {:command :line-to :params {:x 24.3 :y 10.7}}
+                 {:command :line-to :params {:x 17.3 :y 24.7}}
+                 {:command :close-path}]]
+    (-> (thf/sample-file :file1)
+        (tho/add-frame :main-root {:width 107.3 :height 41.7})
+        (ths/add-sample-shape :main-path :type :path :parent-label :main-root :content content)
+        (thc/make-component :component1 :main-root)
+        (thc/instantiate-component :component1 :copy-root {:children-labels [:copy-path]}))))
+
+(t/deftest test-no-sync-changes-when-position-changes-with-float-drift
+  ;; Regression (#12719): geometry in production carries sub-pixel drift
+  ;; and non-integer sizes, so move(+delta) followed by reposition(-delta)
+  ;; does not cancel exactly. A position-only move of the main must still
+  ;; produce no sync changes, and so no "library updated" notification.
+  (let [;; ==== Setup
+        file       (-> (thf/sample-file :file1)
+                       (tho/add-simple-component-with-copy :component1
+                                                           :main-root
+                                                           :main-child
+                                                           :copy-root
+                                                           :main-root-params {:width 107.3 :height 41.7}
+                                                           :main-child-params {:width 33.3 :height 17.7}
+                                                           :copy-root-params {:children-labels [:copy-child]}))
+        page       (thf/current-page file)
+        main-root  (ths/get-shape file :main-root)
+        main-child (ths/get-shape file :main-child)
+        copy-child (ths/get-shape file :copy-child)
+
+        ;; Override on the copy, so it carries touched state
+        changes0   (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                               #{(:id copy-child)}
+                                               (fn [shape]
+                                                 (assoc shape :fills (ths/sample-fills-color :fill-color "#fabada")))
+                                               (:objects page)
+                                               {})
+        file       (thf/apply-changes file changes0)
+        page       (thf/current-page file)
+
+        ;; ==== Action
+        ;; Move the whole main by 1 px plus sub-pixel noise
+        delta      (gpt/point 1.0000123 0.0000071)
+        changes1   (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                               #{(:id main-root) (:id main-child)}
+                                               (fn [shape] (gsh/move shape delta))
+                                               (:objects page)
+                                               {})
+
+        updated-file (thf/apply-changes file changes1)
+
+        sync-changes (cll/generate-sync-file-changes (pcb/empty-changes)
+                                                     nil
+                                                     :components
+                                                     (:id updated-file)
+                                                     (thi/id :component1)
+                                                     (:id updated-file)
+                                                     {(:id updated-file) updated-file}
+                                                     (:id updated-file))]
+
+    ;; ==== Check
+    (t/is (empty? (:redo-changes sync-changes)))))
+
+(t/deftest test-sync-size-change-of-main-child
+  ;; The drift tolerance must not swallow real size edits (>> 0.001).
+  (let [;; ==== Setup
+        file         (-> (thf/sample-file :file1)
+                         (tho/add-simple-component-with-copy :component1
+                                                             :main-root
+                                                             :main-child
+                                                             :copy-root
+                                                             :main-root-params {:width 107.3 :height 41.7}
+                                                             :main-child-params {:width 33.3 :height 17.7}
+                                                             :copy-root-params {:children-labels [:copy-child]}))
+        page         (thf/current-page file)
+
+        ;; ==== Action
+        updated-file (thf/apply-changes file
+                                        (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                                                    #{(thi/id :main-child)}
+                                                                    #(gsh/transform-shape % (ctm/change-size % (+ (:width %) 1) nil))
+                                                                    (:objects page)
+                                                                    {}))
+        attrs        (changes-attrs (sync-file-changes updated-file))]
+
+    ;; ==== Check
+    (t/is (contains? attrs :width))
+    (t/is (contains? attrs :selrect))))
+
+(t/deftest test-sync-text-position-data-change-with-same-xy
+  ;; A change in a text run (here its fill) that keeps x/y must still be
+  ;; synced: position-data is not a drifting geometry attr.
+  (let [;; ==== Setup
+        run         (fn [color]
+                      [{:x 0 :y 0 :width 50 :height 20 :text "hello"
+                        :fills [{:fill-color color :fill-opacity 1}]}])
+        file        (-> (thf/sample-file :file1)
+                        (tho/add-frame-with-text :main-root :main-child "hello")
+                        (thc/make-component :component1 :main-root)
+                        (thc/instantiate-component :component1 :copy-root {:children-labels [:copy-child]}))
+        set-pd      (fn [file color]
+                      (let [page       (thf/current-page file)
+                            main-child (ths/get-shape file :main-child)]
+                        (thf/apply-changes
+                         file
+                         (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                                     #{(:id main-child)}
+                                                     #(assoc % :position-data (run color))
+                                                     (:objects page)
+                                                     {}))))
+
+        file1       (set-pd file "#000000")
+        file2       (thf/apply-changes file1 (sync-file-changes file1))
+        file3       (set-pd file2 "#ff0000")
+
+        ;; ==== Action
+        sync-changes (sync-file-changes file3)]
+
+    ;; ==== Check
+    (t/is (contains? (changes-attrs sync-changes) :position-data))))
+
+(t/deftest test-sync-small-opacity-change
+  ;; The drift tolerance must not swallow real non-geometry changes.
+  (let [;; ==== Setup
+        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]}))
+        set-opacity (fn [file opacity]
+                      (let [page       (thf/current-page file)
+                            main-child (ths/get-shape file :main-child)]
+                        (thf/apply-changes
+                         file
+                         (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                                     #{(:id main-child)}
+                                                     #(assoc % :opacity opacity)
+                                                     (:objects page)
+                                                     {}))))
+
+        file1      (set-opacity file 1)
+        file2      (thf/apply-changes file1 (sync-file-changes file1))
+        file3      (set-opacity file2 0.9995)
+
+        ;; ==== Action
+        sync-changes (sync-file-changes file3)]
+
+    ;; ==== Check
+    (t/is (contains? (changes-attrs sync-changes) :opacity))))
+
+(t/deftest test-no-sync-changes-when-path-moves-with-float-drift
+  ;; Path content stores absolute coordinates, so it drifts like the other
+  ;; geometry when the main is moved.
+  (let [;; ==== Setup
+        file       (file-with-path-component)
+        page       (thf/current-page file)
+        main-root  (ths/get-shape file :main-root)
+        main-path  (ths/get-shape file :main-path)
+
+        ;; ==== Action
+        changes1   (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                               #{(:id main-root) (:id main-path)}
+                                               #(gsh/move % (gpt/point 1.0000123 0.0000071))
+                                               (:objects page)
+                                               {})
+        updated-file (thf/apply-changes file changes1)]
+
+    ;; ==== Check
+    (t/is (empty? (:redo-changes (sync-file-changes updated-file))))))
+
+(t/deftest test-sync-path-moved-inside-main
+  ;; Moving only the path inside its main is a real change and must sync.
+  (let [;; ==== Setup
+        file       (file-with-path-component)
+        page       (thf/current-page file)
+        main-path  (ths/get-shape file :main-path)
+
+        ;; ==== Action
+        changes1   (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                               #{(:id main-path)}
+                                               #(gsh/move % (gpt/point 1 0))
+                                               (:objects page)
+                                               {})
+        updated-file (thf/apply-changes file changes1)]
+
+    ;; ==== Check
+    (t/is (contains? (changes-attrs (sync-file-changes updated-file)) :content))))
+
+(defn- file-with-bool-component
+  []
+  (let [content [{:command :move-to :params {:x 10.3 :y 10.7}}
+                 {:command :line-to :params {:x 24.3 :y 10.7}}
+                 {:command :line-to :params {:x 17.3 :y 24.7}}
+                 {:command :close-path}]]
+    (-> (thf/sample-file :file1)
+        (tho/add-frame :main-root {:width 107.3 :height 41.7})
+        (ths/add-sample-shape :main-bool :type :bool :bool-type :union :parent-label :main-root :content content)
+        (ths/add-sample-shape :main-p1 :type :path :parent-label :main-bool :content content)
+        (ths/add-sample-shape :main-p2 :type :path :parent-label :main-bool :content content)
+        (thc/make-component :component1 :main-root)
+        (thc/instantiate-component :component1 :copy-root {:children-labels [:copy-bool :copy-p1 :copy-p2]}))))
+
+(defn- move-shapes
+  [file labels delta]
+  (let [page (thf/current-page file)]
+    (thf/apply-changes file
+                       (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                                   (into #{} (map #(thi/id %)) labels)
+                                                   #(gsh/move % delta)
+                                                   (:objects page)
+                                                   {}))))
+
+(t/deftest test-no-sync-changes-when-text-moves-with-float-drift
+  ;; Text position-data moves with the shape and drifts like geometry, so
+  ;; a position-only move of the main must produce no sync changes.
+  (let [;; ==== Setup
+        position-data [{:x 10.3 :y 10.7 :width 33.3 :height 17.7 :text "hello"
+                        :fills [{:fill-color "#000000" :fill-opacity 1}]}]
+        file          (-> (thf/sample-file :file1)
+                          (tho/add-frame-with-text :main-root :main-child "hello")
+                          (thc/make-component :component1 :main-root)
+                          (thc/instantiate-component :component1 :copy-root {:children-labels [:copy-child]}))
+        page          (thf/current-page file)
+        file          (thf/apply-changes file
+                                         (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                                                     #{(thi/id :main-child)}
+                                                                     #(assoc % :position-data position-data)
+                                                                     (:objects page)
+                                                                     {}))
+        file          (thf/apply-changes file (sync-file-changes file))
+
+        ;; ==== Action
+        updated-file  (move-shapes file [:main-root :main-child] (gpt/point 1.0000123 0.0000071))
+        sync-changes  (sync-file-changes updated-file)]
+
+    ;; ==== Check
+    (t/is (= #{} (changes-attrs sync-changes)))
+    (t/is (empty? (:redo-changes sync-changes)))))
+
+(t/deftest test-sync-text-moved-inside-main
+  ;; Moving only the text inside its main is a real change beyond the
+  ;; tolerance and must sync its position-data.
+  (let [;; ==== Setup
+        position-data [{:x 10.3 :y 10.7 :width 33.3 :height 17.7 :text "hello"
+                        :fills [{:fill-color "#000000" :fill-opacity 1}]}]
+        file          (-> (thf/sample-file :file1)
+                          (tho/add-frame-with-text :main-root :main-child "hello")
+                          (thc/make-component :component1 :main-root)
+                          (thc/instantiate-component :component1 :copy-root {:children-labels [:copy-child]}))
+        page          (thf/current-page file)
+        file          (thf/apply-changes file
+                                         (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                                                     #{(thi/id :main-child)}
+                                                                     #(assoc % :position-data position-data)
+                                                                     (:objects page)
+                                                                     {}))
+        file          (thf/apply-changes file (sync-file-changes file))
+
+        ;; ==== Action
+        updated-file  (move-shapes file [:main-child] (gpt/point 5 0))]
+
+    ;; ==== Check
+    (t/is (contains? (changes-attrs (sync-file-changes updated-file)) :position-data))))
+
+(t/deftest test-sync-text-position-data-entry-count-change
+  ;; A text that gains a line keeps its first entry untouched but has one
+  ;; more entry: a length mismatch is a real change and must sync.
+  (let [;; ==== Setup
+        entry         {:x 10.3 :y 10.7 :width 33.3 :height 17.7 :text "hello"
+                       :fills [{:fill-color "#000000" :fill-opacity 1}]}
+        set-pd        (fn [file position-data]
+                        (let [page (thf/current-page file)]
+                          (thf/apply-changes file
+                                             (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                                                         #{(thi/id :main-child)}
+                                                                         #(assoc % :position-data position-data)
+                                                                         (:objects page)
+                                                                         {}))))
+        file          (-> (thf/sample-file :file1)
+                          (tho/add-frame-with-text :main-root :main-child "hello")
+                          (thc/make-component :component1 :main-root)
+                          (thc/instantiate-component :component1 :copy-root {:children-labels [:copy-child]}))
+        file          (set-pd file [entry])
+        file          (thf/apply-changes file (sync-file-changes file))
+
+        ;; ==== Action
+        updated-file  (set-pd file [entry (assoc entry :y 28.7 :text "world")])]
+
+    ;; ==== Check
+    (t/is (contains? (changes-attrs (sync-file-changes updated-file)) :position-data))))
+
+(t/deftest test-no-sync-changes-when-bool-moves-with-float-drift
+  ;; Bool content stores absolute coordinates, so it drifts like path content.
+  (let [;; ==== Action
+        file         (file-with-bool-component)
+        updated-file (move-shapes file
+                                  [:main-root :main-bool :main-p1 :main-p2]
+                                  (gpt/point 1.0000123 0.0000071))
+        sync-changes (sync-file-changes updated-file)]
+
+    ;; ==== Check
+    (t/is (= #{} (changes-attrs sync-changes)))
+    (t/is (empty? (:redo-changes sync-changes)))))
+
+(t/deftest test-sync-bool-content-change-inside-main
+  ;; Moving only the bool inside its main is a real change and must sync.
+  (let [;; ==== Action
+        file         (file-with-bool-component)
+        updated-file (move-shapes file [:main-bool :main-p1 :main-p2] (gpt/point 1 0))]
+
+    ;; ==== Check
+    (t/is (contains? (changes-attrs (sync-file-changes updated-file)) :content))))
+
 (t/deftest test-sync-many-updated-shapes
   ;; The undo changes of a sync used to grow one lazy level per updated
   ;; shape (#12576). Overflowing the stack needs thousands of shapes, which

common/test/common_tests/math_test.cljc

@@ -57,3 +57,21 @@
      :clj
      (t/testing "finite? returns false for Double/NaN (CLJ)"
        (t/is (false? (mth/finite? Double/NaN))))))
+
+(t/deftest close-map?-test
+  (t/testing "numbers within tolerance are close"
+    (t/is (mth/close-map? {:x 1.0 :y 2.0} {:x 1.0004 :y 2.0})))
+
+  (t/testing "numbers beyond tolerance are not close"
+    (t/is (not (mth/close-map? {:x 1.0} {:x 1.01}))))
+
+  (t/testing "non-numeric values compare exactly"
+    (t/is (mth/close-map? {:text "a"} {:text "a"}))
+    (t/is (not (mth/close-map? {:text "a"} {:text "b"}))))
+
+  (t/testing "different key sets are not close"
+    (t/is (not (mth/close-map? {:x 1.0} {:x 1.0 :y 1.0})))
+    (t/is (not (mth/close-map? {:x 1.0 :y nil} {:x 1.0 :z nil}))))
+
+  (t/testing "a number is not close to a non-number"
+    (t/is (not (mth/close-map? {:x 1.0} {:x "1.0"})))))

common/test/common_tests/types/path_data_test.cljc

@@ -2864,3 +2864,30 @@
         result  (path/splice-content content sub)]
     (t/is (path.impl/path-data? result))
     (t/is (= (into (vec content) sub) (vec result)))))
+
+(t/deftest content-close?
+  (let [content [{:command :move-to :params {:x 10.3 :y 10.7}}
+                 {:command :line-to :params {:x 24.3 :y 10.7}}
+                 {:command :close-path}]
+        shifted (fn [f] (update-in content [1 :params :x] f))]
+    (t/testing "params within tolerance are close"
+      (t/is (path.segment/content-close? content (shifted #(+ % 0.0004)))))
+
+    (t/testing "a param beyond tolerance is not close"
+      (t/is (not (path.segment/content-close? content (shifted #(+ % 0.01))))))
+
+    (t/testing "a different command is not close"
+      (t/is (not (path.segment/content-close? content (assoc-in content [1 :command] :move-to)))))
+
+    (t/testing "a different segment count is not close"
+      (t/is (not (path.segment/content-close? content (butlast content)))))
+
+    (t/testing "a different param count is not close"
+      (t/is (not (path.segment/content-close? content (assoc-in content [1 :params] {:x 24.3})))))
+
+    (t/testing "plain vector and PathData are comparable"
+      (t/is (path.segment/content-close? content (path.impl/path-data content))))
+
+    (t/testing "nil equals an empty content, but not a non-empty one"
+      (t/is (path.segment/content-close? nil []))
+      (t/is (not (path.segment/content-close? nil content))))))