From 018b857de1cb2ad16d37e061671eec3f2b2142b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Moya?= Date: Fri, 6 Nov 2026 09:51:12 +0100 Subject: [PATCH] :bug: Improve sync of layout item attributes in components (#12851) * :bug: Preserve layout item touched groups when switching variant * :wrench: Add systematic tests to check layout items component sync * :wrench: Add small updates after comments * :wrench: Add tests for partial overrides --- common/src/app/common/logic/libraries.cljc | 17 +- .../app/common/test_helpers/compositions.cljc | 5 +- .../logic/comp_sync_layout_items_test.cljc | 228 ++++++++++++++++++ .../logic/variants_switch_test.cljc | 198 +++++++++++++++ common/test/common_tests/runner.cljc | 2 + 5 files changed, 447 insertions(+), 3 deletions(-) create mode 100644 common/test/common_tests/logic/comp_sync_layout_items_test.cljc diff --git a/common/src/app/common/logic/libraries.cljc b/common/src/app/common/logic/libraries.cljc index 1b7f2488f0..fee7e718c2 100644 --- a/common/src/app/common/logic/libraries.cljc +++ b/common/src/app/common/logic/libraries.cljc @@ -2864,11 +2864,24 @@ page {:id (:id file) :data file} - libraries)))] + libraries))) + + ;; The attributes of `keep-props-values` get their value restored in the + ;; new shape. Keep too the touched groups they belong to, so the override + ;; stays marked as such (otherwise a later sync would overwrite it). + kept-touched-groups + (->> (keys keep-props-values) + (keep #(ctk/resolve-sync-group (:type new-shape) %)) + (filter #(contains? (:touched shape) %)) + (into #{})) + + new-shape (update new-shape :touched (fnil into #{}) kept-touched-groups)] [new-shape (-> changes ;; Restore the properties - (pcb/update-shapes [(:id new-shape)] #(d/patch-object % keep-props-values)) + (pcb/update-shapes [(:id new-shape)] + #(d/patch-object % keep-props-values) + {:ignore-touched true}) ;; Touched attribute has already been set above ;; We need to set the same index as the original shape (pcb/change-parent (:parent-id shape) [new-shape] index {:allow-altering-copies true diff --git a/common/src/app/common/test_helpers/compositions.cljc b/common/src/app/common/test_helpers/compositions.cljc index e2efb1a170..dcc51a2fc5 100644 --- a/common/src/app/common/test_helpers/compositions.cljc +++ b/common/src/app/common/test_helpers/compositions.cljc @@ -18,6 +18,7 @@ [app.common.test-helpers.files :as thf] [app.common.test-helpers.ids-map :as thi] [app.common.test-helpers.shapes :as ths] + [app.common.types.component :as ctk] [app.common.types.container :as ctn] [app.common.types.shape :as cts] [app.common.types.text :as txt])) @@ -394,6 +395,8 @@ orig-shapes (when keep-touched? (cfh/get-children-with-self (:objects page) (:id shape))) + keep-props-values (select-keys shape ctk/swap-keep-attrs) + [new-shape _all-parents changes] (cll/generate-component-swap (pcb/empty-changes) (:objects page) @@ -405,7 +408,7 @@ :id) 0 nil - {} + keep-props-values (true? keep-touched?)) [changes _] (if keep-touched? diff --git a/common/test/common_tests/logic/comp_sync_layout_items_test.cljc b/common/test/common_tests/logic/comp_sync_layout_items_test.cljc new file mode 100644 index 0000000000..0daeb4c77c --- /dev/null +++ b/common/test/common_tests/logic/comp_sync_layout_items_test.cljc @@ -0,0 +1,228 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns common-tests.logic.comp-sync-layout-items-test + (:require + [app.common.files.changes-builder :as pcb] + [app.common.logic.libraries :as cll] + [app.common.test-helpers.components :as thc] + [app.common.test-helpers.compositions :as tho] + [app.common.test-helpers.files :as thf] + [app.common.test-helpers.ids-map :as thi] + [app.common.test-helpers.shapes :as ths] + [clojure.set :as set] + [clojure.test :as t])) + +(t/use-fixtures :each thi/test-fixture) + +;; ---- Value sets + +;; `:layout-item-h-sizing` and `:layout-item-v-sizing` may be absent, or take +;; one of these values. Each test sets both attributes to the same value. +;; +;; :fix -> valid for a board with flex and for any direct child of a flex board. +;; :auto -> valid for a board with flex. +;; :fill -> valid for a direct child of a flex board. +;; +;; A board with flex that is a direct child of another flex board (:comp1, +;; :board1) accepts every value. A plain shape that is a direct child (:rect1) +;; accepts :fix and :fill. :container-main is a top-level flex board, so it +;; accepts :fix and :auto but not :fill. +(def ^:private sizing-values-container + #{nil :fix :auto}) + +(def ^:private sizing-values-child + #{nil :fix :fill}) + +(def ^:private sizing-values-both + (set/union sizing-values-container sizing-values-child)) + +;; Cases for the touched tests: [copy-value main-before main-after]. +;; `main-before` differs from `copy-value`, so setting the copy activates the +;; sync group in :touched. `main-after` is a value the root copy would receive +;; if it were not touched. +(def ^:private root-touched-cases + [[nil :fill :auto] + [:fix :fill :auto] + [:auto :fill :fix] + [:fill :auto :fix]]) + +(def ^:private child-touched-cases + [[nil :fill :fill] + [:fix :fill :fill] + [:fill nil :fix]]) + +;; ---- Setup + +(defn- add-flex-frame + [file label & {:keys [parent-label]}] + (tho/add-frame file label (cond-> {:layout :flex :layout-flex-dir :row} + (some? parent-label) (assoc :parent-label parent-label)))) + +(defn- set-sizing + "Sets both sizing attrs of the shape. A nil value removes them." + [file label value] + (-> file + (tho/change-shape + label + (fn [shape] + (cond-> shape + (some? value) (assoc :layout-item-h-sizing value :layout-item-v-sizing value) + (nil? value) (dissoc :layout-item-h-sizing :layout-item-v-sizing)))))) + +(defn- set-sizing-untouched + "Sets both sizing attrs of the shape without marking it as touched." + [file label value] + (tho/change-shape + file label + (fn [shape] + (cond-> shape + (some? value) (assoc :layout-item-h-sizing value :layout-item-v-sizing value) + (nil? value) (dissoc :layout-item-h-sizing :layout-item-v-sizing))) + :ignore-touched true)) + +(defn- setup-file + "Builds this tree (all boards are flex): + + :container-main + :comp1 # component main (component :comp1-component) + :board1 + :rect1 + :container-copy + :copy1 #--> component :comp1-component + :copy-board1 + :copy-rect1 + + The h/v sizing attrs of :container-main, :comp1 and :board1 are set to the + given values. The copy is created afterwards, without explicit sizing attrs." + [comp-value board-value rect-value] + (let [file (-> (thf/sample-file :file1) + (add-flex-frame :container-main) + (add-flex-frame :comp1 :parent-label :container-main) + (add-flex-frame :board1 :parent-label :comp1) + (tho/add-rect :rect1 :parent-label :board1) + (set-sizing :comp1 comp-value) + (set-sizing :board1 board-value) + (set-sizing :rect1 rect-value) + (thc/make-component :comp1-component :comp1) + (add-flex-frame :container-copy) + (thc/instantiate-component :comp1-component :copy1 + :parent-label :container-copy + :children-labels [:copy-board1]))] + (thi/set-id! :copy-rect1 (first (:shapes (ths/get-shape file :copy-board1)))) + file)) + +;; ---- Actions + +(defn- sync-component + "Propagates the main component changes to its copies." + [file] + (let [file-id (:id file) + component-id (:id (thc/get-component file :comp1-component))] + (thf/apply-changes + file + (cll/generate-sync-file-changes (pcb/empty-changes) + nil + :components + file-id + component-id + file-id + {file-id file} + file-id)))) + +(defn- sizing + [file label] + (let [shape (ths/get-shape file label)] + [(:layout-item-h-sizing shape) (:layout-item-v-sizing shape)])) + +(defn- touched + [file label] + (:touched (ths/get-shape file label))) + +(defn- normalize-sizing + "`:fix` is the default sizing, so it is equivalent to the attribute being + absent. Both forms are accepted in the checks." + [value] + (if (= value :fix) nil value)) + +(defn- check-sizing + [file label expected] + (t/is (= (mapv normalize-sizing [expected expected]) + (mapv normalize-sizing (sizing file label))) + (str label " must be " (pr-str expected)))) + +(defn- expected-root-copy-sizing + "Sizing the copy of the component root is expected to have after a sync. + The root copy only receives `:auto` and `:fix`. `:fill` is never propagated + from the root, and neither is the absence of the attribute, so in those + cases the copy keeps the value it had before the propagation." + [comp-value previous] + (case comp-value + :auto :auto + :fix :fix + previous)) + +(defn- run-propagation + "Changes the main shapes to every valid value and checks that the value + reaches the copies." + [file] + (let [initial (first (sizing file :copy1))] + (doseq [comp-value sizing-values-both + board-value sizing-values-both + rect-value sizing-values-child] + (let [file' (-> file + (set-sizing :comp1 comp-value) + (set-sizing :board1 board-value) + (set-sizing :rect1 rect-value) + (sync-component))] + (check-sizing file' :copy1 (expected-root-copy-sizing comp-value initial)) + (check-sizing file' :copy-board1 board-value) + (check-sizing file' :copy-rect1 rect-value))) + file)) + +(defn- run-touched + "Overrides the sizing in the copy with every valid value (which activates the + sync groups in :touched) and then changes the main. The main values must not + be propagated to the copy this time." + [file] + (doseq [[label main-label cases] + [[:copy1 :comp1 root-touched-cases] + [:copy-board1 :board1 root-touched-cases] + [:copy-rect1 :rect1 child-touched-cases]] + [value before after] cases] + (let [file' (-> file + ;; Align the copy with the main, then override it with a + ;; different value so the override activates :touched. + (set-sizing main-label before) + (set-sizing-untouched label before) + (set-sizing label value)) + _ (t/is (not= (normalize-sizing value) + (normalize-sizing (first (sizing file' main-label)))) + (str label " value must differ from " main-label)) + _ (t/is (= #{:layout-item-h-sizing :layout-item-v-sizing} (touched file' label)) + (str label " must be touched after setting " (pr-str value))) + ;; Changing the main must not reach the copy. + file'' (-> file' + (set-sizing main-label after) + (sync-component))] + (check-sizing file'' label value)))) + +(defn- run-case + [comp-value board-value rect-value] + (-> (setup-file comp-value board-value rect-value) + (run-propagation) + (run-touched))) + +;; ---- Tests + +(t/deftest test-sync-layout-item-sizing + (doseq [comp-value sizing-values-both + board-value sizing-values-both + rect-value sizing-values-child] + (t/testing (str "Testing case comp1=" comp-value + ", board1=" board-value + ", rect1=" rect-value) + (run-case comp-value board-value rect-value)))) diff --git a/common/test/common_tests/logic/variants_switch_test.cljc b/common/test/common_tests/logic/variants_switch_test.cljc index acaa4faf06..46effffac4 100644 --- a/common/test/common_tests/logic/variants_switch_test.cljc +++ b/common/test/common_tests/logic/variants_switch_test.cljc @@ -21,6 +21,8 @@ [app.common.test-helpers.variants :as thv] [app.common.types.component :as ctk] [app.common.types.modifiers :as ctm] + [clojure.set :as set] + [clojure.string :as string] [clojure.test :as t])) (t/use-fixtures :each thi/test-fixture) @@ -1818,6 +1820,202 @@ ;; Verify layout-item-h-sizing is still nil after switch (t/is (nil? (:layout-item-h-sizing rect02'))))) +;; ============================================================ +;; LAYOUT ITEM SIZING - TOUCHED GROUPS ON THE COPY ROOT +;; ============================================================ + +(def ^:private layout-item-attrs + ;; Every layout-child attribute that is kept when swapping a component. + (into #{} (filter #(string/starts-with? (name %) "layout-item-")) ctk/swap-keep-attrs)) + +(def ^:private layout-item-main-values + ;; Values on the component masters; the copy overrides every one of them. + {:layout-item-margin {:m1 1 :m2 1 :m3 1 :m4 1} + :layout-item-margin-type :simple + :layout-item-h-sizing :fix + :layout-item-v-sizing :fix + :layout-item-max-h 100 + :layout-item-min-h 10 + :layout-item-max-w 100 + :layout-item-min-w 10 + :layout-item-absolute false + :layout-item-z-index 0 + :layout-item-align-self :center}) + +(def ^:private layout-item-overrides + ;; User overrides on the copy root, all different from the masters. + {:layout-item-margin {:m1 10 :m2 10 :m3 10 :m4 10} + :layout-item-margin-type :multiple + :layout-item-h-sizing :fill + :layout-item-v-sizing :fill + :layout-item-max-h 500 + :layout-item-min-h 20 + :layout-item-max-w 500 + :layout-item-min-w 20 + :layout-item-absolute true + :layout-item-z-index 5 + :layout-item-align-self :start}) + +(def ^:private layout-item-override-groups + ;; The touched groups of all the overrides attributes. + [:layout-item-margin + :layout-item-h-sizing + :layout-item-v-sizing + :layout-item-max-h + :layout-item-min-h + :layout-item-max-w + :layout-item-min-w + :layout-item-absolute + :layout-item-z-index + :layout-item-align-self]) + +(defn- check-layout-item-attrs-kept + [shape label] + (doseq [[attr value] layout-item-overrides] + (t/is (= value (get shape attr)) + (str label ": value of " attr))) + (doseq [group layout-item-override-groups] + (t/is (contains? (:touched shape) group) + (str label ": touched group " group))) + (t/is (= (count layout-item-override-groups) (count (:touched shape))) + (str label ": number of touched groups"))) + +(def ^:private layout-item-attrs-partial + (set/difference layout-item-attrs #{:layout-item-margin-type + :layout-item-v-sizing + :layout-item-min-h + :layout-item-min-w + :layout-item-z-index})) + +(def ^:private layout-item-partial-overrides + ;; User overrides on the copy root, not all of them. + {:layout-item-margin {:m1 10 :m2 10 :m3 10 :m4 10} + :layout-item-h-sizing :fill + :layout-item-max-h 500 + :layout-item-max-w 500 + :layout-item-absolute true + :layout-item-align-self :start}) + +(def ^:private layout-item-partial-override-groups + ;; The touched groups of all the overrides attributes. + [:layout-item-margin + :layout-item-h-sizing + :layout-item-max-h + :layout-item-max-w + :layout-item-absolute + :layout-item-align-self]) + +(defn- check-layout-item-attrs-kept-partial + [shape label] + (doseq [[attr value] layout-item-partial-overrides] + (t/is (= value (get shape attr)) + (str label ": value of " attr))) + (doseq [group layout-item-partial-override-groups] + (t/is (contains? (:touched shape) group) + (str label ": touched group " group))) + (t/is (= (count layout-item-partial-override-groups) (count (:touched shape))) + (str label ": number of touched groups"))) + +(t/deftest test-switch-preserves-touched-layout-item-attrs-on-root + ;; Every layout-item override on the copy ROOT must survive a variant switch: + ;; `generate-new-shape-for-swap` restores the value from `keep-props-values`, + ;; so it must restore the touched group the value belongs to as well. Before + ;; the fix the groups were dropped, leaving the overrides unprotected. + (let [;; ==== Setup + file (-> (thf/sample-file :file1) + (thv/add-variant :v01 :c01 :m01 :c02 :m02 + {:variant1-params layout-item-main-values + :variant2-params layout-item-main-values}) + (thc/instantiate-component :c01 :copy01)) + file (tho/change-shape file :copy01 #(merge % layout-item-overrides)) + 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)] + + (t/is (= layout-item-attrs (set (keys layout-item-overrides)))) + (check-layout-item-attrs-kept copy01 "before switch") + ;; The values and their touched groups must both survive the switch + (check-layout-item-attrs-kept copy01' "after switch"))) + +(t/deftest test-switch-preserves-touched-layout-item-attrs-on-root-partial + ;; Same as above but with only some of the layout-item attributes overridden. The rest are left untouched. + (let [;; ==== Setup + file (-> (thf/sample-file :file1) + (thv/add-variant :v01 :c01 :m01 :c02 :m02 + {:variant1-params layout-item-main-values + :variant2-params layout-item-main-values}) + (thc/instantiate-component :c01 :copy01)) + file (tho/change-shape file :copy01 #(merge % layout-item-partial-overrides)) + 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)] + + (t/is (= layout-item-attrs-partial (set (keys layout-item-partial-overrides)))) + (check-layout-item-attrs-kept-partial copy01 "before switch") + ;; The values and their touched groups must both survive the switch + (check-layout-item-attrs-kept-partial copy01' "after switch"))) + +(t/deftest test-switch-preserved-layout-item-attrs-survive-main-sync + ;; End-to-end consequence of the bug: after the switch, editing the new master + ;; must not reset the copy. `update-flex-child-copy-attrs` overwrites the + ;; layout-item sizing whose group is not touched, so the preserved groups are + ;; what keep the overrides alive across later syncs. + (let [;; ==== Setup + file (-> (thf/sample-file :file1) + (thv/add-variant :v01 :c01 :m01 :c02 :m02 + {:variant1-params (assoc layout-item-main-values + :layout :flex :layout-flex-dir :row) + :variant2-params (assoc layout-item-main-values + :layout :flex :layout-flex-dir :row)}) + (thc/instantiate-component :c01 :copy01)) + file (tho/change-shape file :copy01 #(merge % layout-item-overrides)) + 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) + file'' (-> file' + (tho/change-shape :m02 #(assoc % :r1 10)) + (tho/propagate-component-changes :c02)) + copy01'' (ths/get-shape file'' :copy01)] + + (t/is (= layout-item-attrs (set (keys layout-item-overrides)))) + (check-layout-item-attrs-kept copy01 "before switch") + (check-layout-item-attrs-kept copy01' "after switch") + ;; The later sync must not overwrite the protected overrides + (check-layout-item-attrs-kept copy01'' "after main sync"))) + +(t/deftest test-switch-preserved-layout-item-attrs-survive-main-sync-partial + ;; Same as above but with only some of the layout-item attributes overridden. The rest are left untouched. + (let [;; ==== Setup + file (-> (thf/sample-file :file1) + (thv/add-variant :v01 :c01 :m01 :c02 :m02 + {:variant1-params (assoc layout-item-main-values + :layout :flex :layout-flex-dir :row) + :variant2-params (assoc layout-item-main-values + :layout :flex :layout-flex-dir :row)}) + (thc/instantiate-component :c01 :copy01)) + file (tho/change-shape file :copy01 #(merge % layout-item-partial-overrides)) + 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) + file'' (-> file' + (tho/change-shape :m02 #(assoc % :r1 10)) + (tho/propagate-component-changes :c02)) + copy01'' (ths/get-shape file'' :copy01)] + + (t/is (= layout-item-attrs-partial (set (keys layout-item-partial-overrides)))) + (check-layout-item-attrs-kept-partial copy01 "before switch") + (check-layout-item-attrs-kept-partial copy01' "after switch") + ;; The later sync must not overwrite the protected overrides + (check-layout-item-attrs-kept-partial copy01'' "after main sync"))) + ;; ============================================================ ;; LAYOUT ITEM SIZING - VERTICAL (fix, auto, fill, none) ;; ============================================================ diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 2b45369c35..e7d7275d84 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -65,6 +65,7 @@ [common-tests.logic.comp-main-edit-breaks-copy-slots-test] [common-tests.logic.comp-remove-swap-slots-test] [common-tests.logic.comp-reset-test] + [common-tests.logic.comp-sync-layout-items-test] [common-tests.logic.comp-sync-test] [common-tests.logic.comp-touched-test] [common-tests.logic.copying-and-duplicating-test] @@ -175,6 +176,7 @@ 'common-tests.logic.comp-main-edit-breaks-copy-slots-test 'common-tests.logic.comp-remove-swap-slots-test 'common-tests.logic.comp-reset-test + 'common-tests.logic.comp-sync-layout-items-test 'common-tests.logic.comp-sync-test 'common-tests.logic.comp-touched-test 'common-tests.logic.copying-and-duplicating-test