From a7b17f54f175b137105a2613dd1b7fb30479511f Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Thu, 21 May 2026 16:39:26 +0200 Subject: [PATCH] :bug: Fix problem of path position on variant change (#9801) --- common/src/app/common/logic/libraries.cljc | 36 +++- common/test/common_tests/data_test.cljc | 2 +- .../logic/variants_switch_test.cljc | 177 +++++++++++++++++- common/test/common_tests/runner.cljc | 2 + 4 files changed, 208 insertions(+), 9 deletions(-) diff --git a/common/src/app/common/logic/libraries.cljc b/common/src/app/common/logic/libraries.cljc index 917f2afe72..848b837fc0 100644 --- a/common/src/app/common/logic/libraries.cljc +++ b/common/src/app/common/logic/libraries.cljc @@ -20,6 +20,7 @@ [app.common.logging :as log] [app.common.logic.shapes :as cls] [app.common.logic.variant-properties :as clvp] + [app.common.math :as mth] [app.common.path-names :as cpn] [app.common.types.component :as ctk] [app.common.types.components-list :as ctkl] @@ -2086,19 +2087,40 @@ displacement (x/y). For :selrect we compare width/height only; for :points we normalise each vector so the first point is the - origin before comparing." + origin before comparing. + For :content on path shapes we compare the bounding-box width/height + of the path segments, again ignoring absolute position. + + Comparisons use `mth/close?` (and `gpt/close?` for points) rather than + exact `=` because `previous-shape` here may carry sub-pixel drift from + interactive transform modifiers (e.g. an alt-drag duplicate of a + variant whose children are component instances). Without tolerance + this guard would miss equivalent geometries and let the `:else` branch + in `update-attrs-on-switch` carry stale `:selrect`/`:points` from the + pre-switch shape onto the freshly instantiated target." [shape origin-shape attr] (or (and (= attr :selrect) - (= (-> shape :selrect :width) (-> origin-shape :selrect :width)) - (= (-> shape :selrect :height) (-> origin-shape :selrect :height))) + (mth/close? (-> shape :selrect :width) (-> origin-shape :selrect :width)) + (mth/close? (-> shape :selrect :height) (-> origin-shape :selrect :height))) (and (= attr :points) (let [normalize-pts (fn [pts] (when (seq pts) (let [f (first pts)] - (mapv #(gpt/subtract % f) pts))))] - (= (normalize-pts (get shape :points)) - (normalize-pts (get origin-shape :points))))))) - + (mapv #(gpt/subtract % f) pts)))) + a (normalize-pts (get shape :points)) + b (normalize-pts (get origin-shape :points))] + (and (= (count a) (count b)) + (every? identity (map gpt/close? a b))))) + (and (= attr :content) + (cfh/path-shape? shape) + (let [ca (:content shape) + cb (:content origin-shape)] + (and (some? ca) (some? cb) + (let [selrect-a (segment/content->selrect ca) + selrect-b (segment/content->selrect cb)] + (and (some? selrect-a) (some? selrect-b) + (mth/close? (:width selrect-a) (:width selrect-b)) + (mth/close? (:height selrect-a) (:height selrect-b))))))))) (defn update-attrs-on-switch "Copy attributes that have changed in the shape previous to the switch diff --git a/common/test/common_tests/data_test.cljc b/common/test/common_tests/data_test.cljc index 56b43078dc..70f13e5119 100644 --- a/common/test/common_tests/data_test.cljc +++ b/common/test/common_tests/data_test.cljc @@ -888,7 +888,7 @@ (t/is (not (d/editable-collection? "hello"))) (t/is (not (d/editable-collection? 42)))) -(t/deftest num-predicate +(t/deftest num-predicate-2 (t/is (d/num? 1)) (t/is (d/num? 0)) (t/is (d/num? -3.14)) diff --git a/common/test/common_tests/logic/variants_switch_test.cljc b/common/test/common_tests/logic/variants_switch_test.cljc index 99784dade5..0384a95a89 100644 --- a/common/test/common_tests/logic/variants_switch_test.cljc +++ b/common/test/common_tests/logic/variants_switch_test.cljc @@ -7,6 +7,9 @@ (ns common-tests.logic.variants-switch-test (:require [app.common.files.changes-builder :as pcb] + [app.common.files.helpers :as cfh] + [app.common.geom.point :as gpt] + [app.common.geom.shapes :as gsh] [app.common.logic.shapes :as cls] [app.common.test-helpers.components :as thc] [app.common.test-helpers.compositions :as tho] @@ -2860,4 +2863,176 @@ ;; Both variants are identical in size (100x50), so the override IS preserved (t/is (= (:width rect02') 150)) ;; The guard must not have suppressed :selrect — it should be consistent - (t/is (= (get-in rect02' [:selrect :width]) 150)))) \ No newline at end of file + (t/is (= (get-in rect02' [:selrect :width]) 150)))) + + +(t/deftest test-switch-when-source-master-child-has-touched-geometry + ;; Regression: when the previous-shape's geometry has sub-pixel drift + ;; relative to its source master (a state produced by interactive transform + ;; modifiers, e.g. alt-drag duplicate of a variant whose children are + ;; component copies), the equal-geometry? guard in update-attrs-on-switch + ;; uses exact equality and fails. The :else branch then copies + ;; previous-shape's :selrect verbatim onto the freshly-instantiated target, + ;; leaving :y correct (the per-attr y skip catches that) but :selrect.y + ;; stale. The shape ends up internally inconsistent (:y disagrees with + ;; :selrect.y); the renderer reads :selrect, so the child appears at the + ;; source variant's position inside a parent that has resized to the + ;; target's dimensions — the visible "cut off" symptom. + (let [;; ==== Setup + ;; A self-contained Input/Button-like component, plus a variant + ;; container whose two variants each instance that component + ;; at different y positions. This mirrors the production setup + ;; where the dragged variant's children are themselves component + ;; copies (and thus carry :touched on geometry within the master). + file (-> (thf/sample-file :file1) + (tho/add-simple-component :btn-comp :btn-root :btn-rect) + (thv/add-variant-with-copy + :v01 :c01 :m01 :c02 :m02 :child1 :child2 :btn-comp)) + + ;; Position child1 at y=101 (in m01) and child2 at y=73 (in m02). + ;; Use gsh/absolute-move so :selrect/:points stay consistent with + ;; :y — a plain (assoc :y …) would leave them out of sync and + ;; produce a different (artificial) failure mode. + page (thf/current-page file) + child1 (ths/get-shape file :child1) + child2 (ths/get-shape file :child2) + changes (-> (pcb/empty-changes nil (:id page)) + (cls/generate-update-shapes + #{(:id child1)} + #(gsh/absolute-move % (gpt/point (:x %) 101)) + (:objects page) {}) + (cls/generate-update-shapes + #{(:id child2)} + #(gsh/absolute-move % (gpt/point (:x %) 73)) + (:objects page) {})) + file (thf/apply-changes file changes) + file (thc/instantiate-component file :c01 :copy01) + + ;; The copy carries an Input/Button instance (Frame1). Introduce + ;; sub-pixel drift in its :width and :selrect.width — the kind of + ;; floating-point error produced by the alt-drag modifier path in + ;; production. This drift is what defeats equal-geometry?'s + ;; exact-equality comparison and lets the bug surface. + page (thf/current-page file) + copy01 (ths/get-shape file :copy01) + copy-btn-id (->> (cfh/get-children-ids-with-self (:objects page) (:id copy01)) + (map #(get-in page [:objects %])) + (filter #(= "Frame1" (:name %))) + first :id) + drift 0.00001 + changes (cls/generate-update-shapes + (pcb/empty-changes nil (:id page)) + #{copy-btn-id} + (fn [shape] + (let [w (+ (:width shape) drift) + sr (:selrect shape)] + (-> shape + (assoc :width w) + (assoc :selrect (-> sr + (assoc :width w) + (assoc :x2 (+ (:x1 sr) w))))))) + (:objects page) {}) + file (thf/apply-changes file changes) + m02 (ths/get-shape file :m02) + child2 (ths/get-shape file :child2) + + target-rel-y (- (:y child2) (:y m02)) + + ;; ==== Action + file' (tho/swap-component-in-shape file :copy01 :c02 + {:new-shape-label :copy02 + :keep-touched? true}) + + page' (thf/current-page file') + copy02 (ths/get-shape file' :copy02) + post-btn (->> (cfh/get-children-ids-with-self (:objects page') (:id copy02)) + (map #(get-in page' [:objects %])) + (filter #(= "Frame1" (:name %))) + first) + post-btn-rel-y (- (:y post-btn) (:y copy02)) + post-btn-selrect-rel-y (- (get-in post-btn [:selrect :y]) + (get-in copy02 [:selrect :y]))] + + ;; The post-switch button must sit at the target master's relative y. + ;; Its :y field already does (the per-attr :y skip handles that + ;; correctly); the failure is on :selrect. + (t/is (= target-rel-y post-btn-rel-y) + (str "Child's :y should match target master layout (" target-rel-y ")")) + + ;; The bug: :selrect.y is overwritten with the previous shape's value, + ;; not regenerated from the target master's layout. After fix, this + ;; assertion should pass. + (t/is (= target-rel-y post-btn-selrect-rel-y) + (str ":selrect.y should match target master layout (expected " + target-rel-y " got " post-btn-selrect-rel-y ")")) + + ;; And :y must agree with :selrect.y — a shape whose :y disagrees with + ;; its :selrect.y is internally inconsistent and renders incorrectly. + (t/is (= post-btn-rel-y post-btn-selrect-rel-y) + ":y and :selrect.y must agree after switch"))) + + +(t/deftest test-switch-does-not-override-path-content-when-only-repositioned + ;; Regression: when a path shape inside a variant has :geometry-group touched + ;; (e.g. because auto-layout repositioned it after the copy's parent was + ;; resized), switching variants must NOT copy the old variant's path position + ;; to the new variant. The path should stay at the new variant's default position. + ;; + ;; Root cause: equal-geometry? did not handle the :content attr for path shapes, + ;; so switch-path-change-value was always invoked and placed the new path at the + ;; pre-switch absolute position instead of the target master's default position. + (let [;; A small closed triangle path whose bounding box is 24x14 px, + ;; anchored at absolute position (x0, y0). + triangle (fn [x0 y0] + [{:command :move-to :params {:x x0 :y y0}} + {:command :line-to :params {:x (+ x0 24) :y y0}} + {:command :line-to :params {:x (+ x0 12) :y (+ y0 14)}} + {:command :close-path}]) + + ;; V1 has the path at y=10; V2 has the same-shape path at y=30. + file (-> (thf/sample-file :file1) + (thv/add-variant :v01 :c01 :m01 :c02 :m02 + {:variant1-params {:width 100 :height 100} + :variant2-params {:width 100 :height 100}}) + (ths/add-sample-shape :path1 :type :path + :parent-label :m01 + :content (triangle 0 10)) + (ths/add-sample-shape :path2 :type :path + :parent-label :m02 + :content (triangle 0 30)) + (thc/instantiate-component :c01 :copy01)) + + ;; Simulate auto-layout repositioning the path inside the copy by + ;; moving it to y=50. This touches :geometry-group on the copy's path. + page (thf/current-page file) + copy01 (ths/get-shape file :copy01) + copy-path (->> (cfh/get-children-with-self (:objects page) (:id copy01)) + (filter #(= :path (:type %))) + first) + changes (cls/generate-update-shapes + (pcb/empty-changes nil (:id page)) + #{(:id copy-path)} + #(gsh/absolute-move % (gpt/point (:x %) 50)) + (:objects page) {}) + file (thf/apply-changes file changes) + + ;; Switch copy01 from V1 (c01) to V2 (c02). + file' (tho/swap-component-in-shape file :copy01 :c02 {:keep-touched? true}) + + page' (thf/current-page file') + copy01' (ths/get-shape file' :copy01) + copy-path' (->> (cfh/get-children-with-self (:objects page') (:id copy01')) + (filter #(= :path (:type %))) + first) + + ;; Expected: V2's path sits at y=30 (its master default), not y=50 + ;; (the pre-switch repositioned position). + m02 (ths/get-shape file :m02) + path2 (ths/get-shape file :path2) + target-rel-y (- (-> path2 :selrect :y) (-> m02 :selrect :y)) + actual-rel-y (- (-> copy-path' :selrect :y) (-> copy01' :selrect :y))] + + (t/is (some? copy-path') "path should exist in switched copy") + (t/is (= target-rel-y actual-rel-y) + (str "path :selrect.y should match target master layout (expected " + target-rel-y " got " actual-rel-y ")")))) diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 29540525db..9e1cd276a5 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -52,6 +52,7 @@ [common-tests.logic.swap-and-reset-test] [common-tests.logic.swap-as-override-test] [common-tests.logic.token-test] + [common-tests.logic.variants-switch-test] [common-tests.media-test] [common-tests.path-names-test] [common-tests.record-test] @@ -132,6 +133,7 @@ 'common-tests.logic.swap-and-reset-test 'common-tests.logic.swap-as-override-test 'common-tests.logic.token-test + 'common-tests.logic.variants-switch-test 'common-tests.media-test 'common-tests.path-names-test 'common-tests.record-test