From 4aa0f9754e01fb2b06cc302388baade2747d2c32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Moya?= Date: Fri, 24 Apr 2026 14:32:24 +0200 Subject: [PATCH] :bug: Fix loss of swap slot in some cases of variant switch --- CHANGES.md | 1 + .../src/app/common/files/changes_builder.cljc | 4 +- common/src/app/common/logic/libraries.cljc | 12 +- .../logic/variants_switch_test.cljc | 211 ++++++++++++++++-- 4 files changed, 206 insertions(+), 22 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index f1edeb5963..888246b514 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -144,6 +144,7 @@ - Fix plugin parse-point returning plain map instead of Point record (by @FairyPigDev) [Github #9129](https://github.com/penpot/penpot/pull/9129) - Fix `:heigth` typo in clipboard frame-same-size? (by @iot2edge) [Github #9250](https://github.com/penpot/penpot/pull/9250) - Fix Settings and Notifications "Update Settings" button enabled state when form has no changes (by @moorsecopers99) [Github #9090](https://github.com/penpot/penpot/issues/9090) +- Fix file crashing when switching a variant [Taiga #14014](https://tree.taiga.io/project/penpot/issue/14014) ## 2.15.0 (Unreleased) diff --git a/common/src/app/common/files/changes_builder.cljc b/common/src/app/common/files/changes_builder.cljc index 5a3838c8c1..49a78726f5 100644 --- a/common/src/app/common/files/changes_builder.cljc +++ b/common/src/app/common/files/changes_builder.cljc @@ -46,8 +46,8 @@ (with-meta changes {::page-id page-id}))) ([] - {:redo-changes [] - :undo-changes '()}) + {:redo-changes [] ;; redo-changes is a vector so that conj adds things at the end, in order of execution + :undo-changes '()}) ;; undo-changes is a list to conj things at the beginning, so they execute in the reverse order when undoing several changes ([origin] {:redo-changes [] :undo-changes '() diff --git a/common/src/app/common/logic/libraries.cljc b/common/src/app/common/logic/libraries.cljc index 39762ed139..08c40b5ee3 100644 --- a/common/src/app/common/logic/libraries.cljc +++ b/common/src/app/common/logic/libraries.cljc @@ -2119,8 +2119,8 @@ (contains? #{:auto-height :auto-width} (:grow-type current-shape)))] (loop [attrs updatable-attrs - roperations [{:type :set-touched :touched (:touched previous-shape)}] - uoperations (list {:type :set-touched :touched (:touched current-shape)})] + roperations [] + uoperations '()] (if-let [attr (first attrs)] (let [sync-group (ctk/resolve-sync-group (:type previous-shape) attr) @@ -2262,7 +2262,13 @@ (let [updated-attrs (into #{} (comp (filter #(= :set (:type %))) (map :attr)) - roperations)] + roperations) + updated-sync-groups (into #{} + (keep #(ctk/resolve-sync-group (:type previous-shape) %)) + updated-attrs) + new-touched (set/union (or (:touched current-shape) #{}) updated-sync-groups) + roperations (into [{:type :set-touched :touched new-touched}] roperations) + uoperations (into (list {:type :set-touched :touched (:touched current-shape)}) uoperations)] (cond-> changes (> (count roperations) 1) (-> (add-update-attr-changes current-shape container roperations uoperations) diff --git a/common/test/common_tests/logic/variants_switch_test.cljc b/common/test/common_tests/logic/variants_switch_test.cljc index c991f35ab6..207ae1e1b3 100644 --- a/common/test/common_tests/logic/variants_switch_test.cljc +++ b/common/test/common_tests/logic/variants_switch_test.cljc @@ -14,6 +14,7 @@ [app.common.test-helpers.ids-map :as thi] [app.common.test-helpers.shapes :as ths] [app.common.test-helpers.variants :as thv] + [app.common.types.component :as ctk] [clojure.test :as t])) (t/use-fixtures :each thi/test-fixture) @@ -35,13 +36,13 @@ copy01 (ths/get-shape file :copy01) ;; ==== Action - file' (tho/swap-component-in-shape file :copy01 :c02 {:new-shape-label :copy02 :keep-touched? true}) + file' (tho/swap-component-in-shape file :copy01 :c02 {:keep-touched? true}) - copy01' (ths/get-shape file' :copy02)] + copy01' (ths/get-shape file' :copy01)] (thf/dump-file file :keys [:width]) ;; The copy had width 5 before the switch (t/is (= (:width copy01) 5)) - ;; The rect has width 15 after the switch + ;; The copy has width 15 after the switch (t/is (= (:width copy01') 15)))) (t/deftest test-simple-switch @@ -61,15 +62,15 @@ rect01 (get-in page [:objects (-> copy01 :shapes first)]) ;; ==== Action - file' (tho/swap-component-in-shape file :copy01 :c02 {:new-shape-label :copy02 :keep-touched? true}) + file' (tho/swap-component-in-shape file :copy01 :c02 {:keep-touched? true}) page' (thf/current-page file') - copy02' (ths/get-shape file' :copy02) - rect02' (get-in page' [:objects (-> copy02' :shapes first)])] + copy01' (ths/get-shape file' :copy01) + rect01' (get-in page' [:objects (-> copy01' :shapes first)])] ;; The rect had width 5 before the switch (t/is (= (:width rect01) 5)) ;; The rect has width 15 after the switch - (t/is (= (:width rect02') 15)))) + (t/is (= (:width rect01') 15)))) ;; ============================================================ ;; SIMPLE ATTRIBUTE OVERRIDES (identical variants) @@ -100,9 +101,9 @@ copy01 (ths/get-shape file :copy01) ;; ==== Action - file' (tho/swap-component-in-shape file :copy01 :c02 {:new-shape-label :copy02 :keep-touched? true}) + file' (tho/swap-component-in-shape file :copy01 :c02 {:keep-touched? true}) - copy01' (ths/get-shape file' :copy02)] + copy01' (ths/get-shape file' :copy01)] (thf/dump-file file :keys [:width]) ;; The copy had width 25 before the switch (t/is (= (:width copy01) 25)) @@ -137,16 +138,16 @@ rect01 (get-in page [:objects (:id rect01)]) ;; ==== Action - file' (tho/swap-component-in-shape file :copy01 :c02 {:new-shape-label :copy02 :keep-touched? true}) + file' (tho/swap-component-in-shape file :copy01 :c02 {:keep-touched? true}) page' (thf/current-page file') - copy02' (ths/get-shape file' :copy02) - rect02' (get-in page' [:objects (-> copy02' :shapes first)])] + copy01' (ths/get-shape file' :copy01) + rect01' (get-in page' [:objects (-> copy01' :shapes first)])] ;; The rect had width 25 before the switch (t/is (= (:width rect01) 25)) ;; The override is keept: The rect still has width 25 after the switch - (t/is (= (:width rect02') 25)))) + (t/is (= (:width rect01') 25)))) ;; ============================================================ ;; SIMPLE ATTRIBUTE OVERRIDES (different variants) @@ -180,17 +181,193 @@ rect01 (get-in page [:objects (:id rect01)]) ;; ==== Action - file' (tho/swap-component-in-shape file :copy01 :c02 {:new-shape-label :copy02 :keep-touched? true}) + file' (tho/swap-component-in-shape file :copy01 :c02 {:keep-touched? true}) page' (thf/current-page file') - copy02' (ths/get-shape file' :copy02) - rect02' (get-in page' [:objects (-> copy02' :shapes first)])] + copy01' (ths/get-shape file' :copy01) + rect01' (get-in page' [:objects (-> copy01' :shapes first)])] ;; The rect had width 25 before the switch (t/is (= (:width rect01) 25)) ;; The override isn't keept, because the property is different in the mains ;; The rect has width 15 after the switch - (t/is (= (:width rect02') 15)))) + (t/is (= (:width rect01') 15)))) + +;; ============================================================ +;; NESTED COPY SWITCH (no overrides) +;; ============================================================ + +(t/deftest test-nested-switch-in-main + (let [;; ==== Setup + file (-> (thf/sample-file :file1) + (thv/add-variant + :v01 :c01 :m01 :c02 :m02 + {:variant1-params {:width 5} + :variant2-params {:width 15}}) + + (tho/add-frame :m03) + (thc/instantiate-component :c01 + :copy01 + :parent-label :m03) + (thc/make-component :c03 :m03)) + + copy01 (ths/get-shape file :copy01) + + ;; ==== Action + file' (tho/swap-component-in-shape file :copy01 :c02 {:keep-touched? true}) + + copy01' (ths/get-shape file' :copy01)] + + (thf/dump-file file :keys [:width]) + + ;; The copy had width 5 before the switch + (t/is (= (:width copy01) 5)) + ;; The copy has width 15 after the switch + (t/is (= (:width copy01') 15)) + ;; The copy is not touched but has swap slot + (t/is (= (count (:touched copy01')) 1)) + (t/is (= (ctk/get-swap-slot copy01') (thi/id :copy01))))) + +(t/deftest test-nested-switch-in-copy + (let [;; ==== Setup + file (-> (thf/sample-file :file1) + (thv/add-variant + :v01 :c01 :m01 :c02 :m02 + {:variant1-params {:width 5} + :variant2-params {:width 15}}) + + (tho/add-frame :m03) + (thc/instantiate-component :c01 + :nested01 + :parent-label :m03) + (thc/make-component :c03 :m03) + + (thc/instantiate-component :c03 + :nested02 + :children-labels [:child01])) + + child01 (ths/get-shape file :child01) + + ;; ==== Action + file' (tho/swap-component-in-shape file :child01 :c02 {:keep-touched? true}) + + child01' (ths/get-shape file' :child01)] + + (thf/dump-file file :keys [:width]) + + ;; The copy had width 5 before the switch + (t/is (= (:width child01) 5)) + ;; The copy has width 15 after the switch + (t/is (= (:width child01') 15)) + ;; The copy is not touched but has swap slot + (t/is (= (count (:touched child01')) 1)) + (t/is (= (ctk/get-swap-slot child01') (thi/id :nested01))))) + +;; ============================================================ +;; NESTED COPY SWITCH (with overrides) +;; ============================================================ + +(t/deftest test-nested-switch-in-main-with-override + (let [;; ==== Setup + file (-> (thf/sample-file :file1) + (thv/add-variant + :v01 :c01 :m01 :c02 :m02 + {:variant1-params {:width 5} + :variant2-params {:width 15}}) + + (tho/add-frame :m03) + (thc/instantiate-component :c01 + :copy01 + :parent-label :m03) + (thc/make-component :c03 :m03)) + + page (thf/current-page file) + fills (ths/sample-fills-color :fill-color "#fabada") + changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page)) + #{(thi/id :copy01)} + (fn [shape] + (assoc shape + :width 25 + :fills fills)) + (:objects page) + {}) + + file (thf/apply-changes file changes) + + copy01 (ths/get-shape file :copy01) + + ;; ==== Action + file' (tho/swap-component-in-shape file :copy01 :c02 {:keep-touched? true}) + + copy01' (ths/get-shape file' :copy01)] + + (thf/dump-file file :keys [:width :touched]) + + ;; The copy had fill color before the switch + (t/is (= (:fills copy01) fills)) + ;; The copy still has fill color after the switch + (t/is (= (:fills copy01') fills)) + ;; The copy had width 25 before the switch + (t/is (= (:width copy01) 25)) + ;; The copy gets the switched variant width 15, because this is the value changed in the variant + (t/is (= (:width copy01') 15)) + ;; The copy is fills touched and has swap slot + (t/is (= (count (:touched copy01')) 2)) + (t/is (= (ctk/get-swap-slot copy01') (thi/id :copy01))) + (t/is (contains? (:touched copy01') :fill-group)))) + +(t/deftest test-nested-switch-in-copy-with-override + (let [;; ==== Setup + file (-> (thf/sample-file :file1) + (thv/add-variant + :v01 :c01 :m01 :c02 :m02 + {:variant1-params {:width 5} + :variant2-params {:width 15}}) + + (tho/add-frame :m03) + (thc/instantiate-component :c01 + :nested01 + :parent-label :m03) + (thc/make-component :c03 :m03) + + (thc/instantiate-component :c03 + :copy02 + :children-labels [:nested02])) + + page (thf/current-page file) + fills (ths/sample-fills-color :fill-color "#fabada") + changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page)) + #{(thi/id :nested02)} + (fn [shape] + (assoc shape + :width 25 + :fills fills)) + (:objects page) + {}) + + file (thf/apply-changes file changes) + + nested02 (ths/get-shape file :nested02) + + ;; ==== Action + file' (tho/swap-component-in-shape file :nested02 :c02 {:keep-touched? true}) + + nested02' (ths/get-shape file' :nested02)] + + (thf/dump-file file :keys [:width]) + + ;; The copy had fill color before the switch + (t/is (= (:fills nested02) fills)) + ;; The copy still has fill color after the switch + (t/is (= (:fills nested02') fills)) + ;; The copy had width 5 before the switch + (t/is (not= (:width nested02) 5)) + ;; The copy gets the switched variant width 15, because this is the value changed in the variant + (t/is (= (:width nested02') 15)) + ;; The copy is fills touched and has swap slot + (t/is (= (count (:touched nested02')) 2)) + (t/is (= (ctk/get-swap-slot nested02') (thi/id :nested01))) + (t/is (contains? (:touched nested02') :fill-group)))) ;; ============================================================ ;; TEXT OVERRIDES (identical variants)