🐛 Improve sync of layout item attributes in components (#12851)

* 🐛 Preserve layout item touched groups when switching variant

* 🔧 Add systematic tests to check layout items component sync

* 🔧 Add small updates after comments

* 🔧 Add tests for partial overrides
This commit is contained in:
Andrés Moya 2026-11-06 09:51:12 +01:00 committed by GitHub
parent 1a80e98e62
commit 018b857de1
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 447 additions and 3 deletions

View File

@ -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

View File

@ -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?

View File

@ -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))))

View File

@ -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)
;; ============================================================

View File

@ -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