diff --git a/.serena/memories/common/decimals-and-coordinates.md b/.serena/memories/common/decimals-and-coordinates.md index 0a4e8992ad..1cc966f452 100644 --- a/.serena/memories/common/decimals-and-coordinates.md +++ b/.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. diff --git a/common/src/app/common/logic/libraries.cljc b/common/src/app/common/logic/libraries.cljc index acfbe6c2d1..1b928d407b 100644 --- a/common/src/app/common/logic/libraries.cljc +++ b/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 diff --git a/common/src/app/common/math.cljc b/common/src/app/common/math.cljc index 41e3047880..c924446c15 100644 --- a/common/src/app/common/math.cljc +++ b/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] diff --git a/common/src/app/common/types/path/segment.cljc b/common/src/app/common/types/path/segment.cljc index 561dd4e44b..dbfc1a44bc 100644 --- a/common/src/app/common/types/path/segment.cljc +++ b/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 diff --git a/common/test/common_tests/logic/comp_sync_test.cljc b/common/test/common_tests/logic/comp_sync_test.cljc index 02cd219779..f8ecbe336d 100644 --- a/common/test/common_tests/logic/comp_sync_test.cljc +++ b/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 diff --git a/common/test/common_tests/math_test.cljc b/common/test/common_tests/math_test.cljc index 524963df9b..a8c3188a13 100644 --- a/common/test/common_tests/math_test.cljc +++ b/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"}))))) diff --git a/common/test/common_tests/types/path_data_test.cljc b/common/test/common_tests/types/path_data_test.cljc index 6c1b957691..de9762a2fc 100644 --- a/common/test/common_tests/types/path_data_test.cljc +++ b/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))))))