From d47f366c709629c061190fcb0b48975a7693b337 Mon Sep 17 00:00:00 2001 From: Pablo Alba Date: Thu, 29 Oct 2026 22:46:49 +0100 Subject: [PATCH] :bug: Fix Creating or combining a variant set saves half-finished steps --- .../src/app/main/data/workspace/variants.cljs | 224 +++++++------- .../data/workspace_variants_test.cljs | 274 +++++++++++++++++- 2 files changed, 392 insertions(+), 106 deletions(-) diff --git a/frontend/src/app/main/data/workspace/variants.cljs b/frontend/src/app/main/data/workspace/variants.cljs index 149a5edeff..f5f82a45b3 100644 --- a/frontend/src/app/main/data/workspace/variants.cljs +++ b/frontend/src/app/main/data/workspace/variants.cljs @@ -304,22 +304,36 @@ (dch/commit-changes changes) (dwu/commit-undo-transaction undo-id))))))))) -(defn- set-variant-id - "Sets the variant-id on a component" - [component-id variant-id] - (ptk/reify ::set-variant-id +(defn generate-define-variant-container + "One commit that turns an existing frame into a valid variant set: + container flag and name, the main's variant-id, the component's + variant-id (refreshing library data), then the first property." + [changes variant-id main-id component-id name] + (-> changes + (pcb/update-shapes [variant-id] + #(assoc % :is-variant-container true :name name)) + (pcb/update-shapes [main-id] + #(assoc % :name name :variant-id variant-id)) + (pcb/update-component component-id + #(assoc % :variant-id variant-id) + {:apply-changes-local-library? true}) + (clvp/generate-add-new-property variant-id {:fill-values? true}))) + +(defn- define-variant-container + "Read page objects after the artboard exists, then commit the defining + variant fields in one change." + [variant-id main-id component-id name] + (ptk/reify ::define-variant-container ptk/WatchEvent (watch [it state _] (let [page-id (:current-page-id state) data (dsh/lookup-file-data state) + objects (dsh/lookup-page-objects state page-id) changes (-> (pcb/empty-changes it page-id) + (pcb/with-objects objects) (pcb/with-library-data data) - (pcb/update-component component-id #(assoc % :variant-id variant-id))) - undo-id (js/Symbol)] - (rx/of - (dwu/start-undo-transaction undo-id) - (dch/commit-changes changes) - (dwu/commit-undo-transaction undo-id)))))) + (generate-define-variant-container variant-id main-id component-id name))] + (rx/of (dch/commit-changes changes)))))) (defn- focus-property [variant-id] @@ -397,6 +411,95 @@ (->> (rx/of (focus-property (:id variant-container))) (rx/delay 250)))))))))) +(defn- transform-in-variant-stream + "Event stream that turns `main-instance-id` into a variant set. + The defining fields share one commit (`define-variant-container`). + Combine concatenates this stream before relocating the other mains. + `page-id` is the page that holds the main; nil means the open page." + [state main-instance-id variant-id delta prefix add-wrapper? duplicate? flex? page-id] + (let [variant-id (or variant-id (uuid/next)) + variant-vec [variant-id] + file-id (:current-file-id state) + page-id (or page-id (:current-page-id state)) + objects (dsh/lookup-page-objects state file-id page-id) + main (get objects main-instance-id) + parent (get objects (:parent-id main)) + component-id (:component-id main) + name (if add-wrapper? + (str "Component/" (:name main)) + (:name main)) + ;; If there is a prefix, set is as first item of path + cpath (-> name + cpn/split-path + (cond-> + (seq prefix) + (->> (drop (count prefix)) + (cons (cpn/join-path prefix)) + vec))) + + name (first cpath) + num-props (max 1 (dec (count cpath))) + style-props (cond-> {:r1 20 + :r2 20 + :r3 20 + :r4 20 + :layout-item-absolute true} + flex? + (into {:layout-item-h-sizing :auto + :layout-item-v-sizing :auto + :layout-padding {:p1 30 :p2 30 :p3 30 :p4 30} + :layout-gap {:row-gap 0 :column-gap 20}})) + + stroke-props {:stroke-alignment :inner + :stroke-style :solid + :stroke-color "#bb97d8" ;; todo use color var? + :stroke-opacity 1 + :stroke-width 2} + + ;; Move the position of the variant container so the main shape doesn't + ;; change its position + delta (or delta + (if (ctsl/any-layout? parent) + (gpt/point 0 0) + (gpt/point -30 -30))) + undo-id (js/Symbol)] + + (rx/concat + (rx/of + (dwu/start-undo-transaction undo-id) + + (when (not= name (:name main)) + (dwl/rename-component component-id name)) + + ;; Create the frame while it is still a normal board + (dwsh/create-artboard-from-shapes [main-instance-id] variant-id nil nil nil delta flex?) + (cl/remove-all-fills variant-vec {:color clr/black :opacity 1}) + (when flex? (dwsl/create-layout-from-id variant-id :flex)) + (dwsh/update-shapes variant-vec #(merge % style-props)) + (cl/add-stroke variant-vec stroke-props)) + + (rx/of (define-variant-container variant-id main-instance-id component-id name)) + + ;; First property is in the defining commit + (when (> num-props 1) + (rx/from + (repeatedly (dec num-props) + #(add-new-property variant-id {:fill-values? true})))) + + ;; When the component has path, set the path items as properties values + (when (> (count cpath) 1) + (rx/from + (map + #(update-property-value component-id % (nth cpath (inc %))) + (range num-props)))) + + (rx/of + (when duplicate? (add-new-variant main-instance-id)) + (dwsh/update-shapes variant-vec #(dissoc % :layout-item-absolute)) + (dwu/commit-undo-transaction undo-id) + (when flex? + (ptk/data-event :layout/update {:ids [variant-id]})))))) + (defn transform-in-variant "Given the id of a main shape of a component, creates a variant structure for that component" @@ -406,98 +509,7 @@ (ptk/reify ::transform-in-variant ptk/WatchEvent (watch [_ state _] - (let [variant-id (or variant-id (uuid/next)) - variant-vec [variant-id] - file-id (:current-file-id state) - page-id (:current-page-id state) - objects (dsh/lookup-page-objects state file-id page-id) - main (get objects main-instance-id) - parent (get objects (:parent-id main)) - component-id (:component-id main) - name (if add-wrapper? - (str "Component/" (:name main)) - (:name main)) - ;; If there is a prefix, set is as first item of path - cpath (-> name - cpn/split-path - (cond-> - (seq prefix) - (->> (drop (count prefix)) - (cons (cpn/join-path prefix)) - vec))) - - name (first cpath) - num-props (max 1 (dec (count cpath))) - base-props {:is-variant-container true - :name name - :r1 20 - :r2 20 - :r3 20 - :r4 20 - :layout-item-absolute true} - flex-props {:layout-item-h-sizing :auto - :layout-item-v-sizing :auto - :layout-padding {:p1 30 :p2 30 :p3 30 :p4 30} - :layout-gap {:row-gap 0 :column-gap 20}} - cont-props (if flex? - (into base-props flex-props) - base-props) - main-props {:name name - :variant-id variant-id} - - stroke-props {:stroke-alignment :inner - :stroke-style :solid - :stroke-color "#bb97d8" ;; todo use color var? - :stroke-opacity 1 - :stroke-width 2} - - ;; Move the position of the variant container so the main shape doesn't - ;; change its position - delta (or delta - (if (ctsl/any-layout? parent) - (gpt/point 0 0) - (gpt/point -30 -30))) - undo-id (js/Symbol)] - - - ;;TODO Refactor all called methods in order to be able to - ;;generate changes instead of call the events - - - (rx/concat - (rx/of - (dwu/start-undo-transaction undo-id) - - (when (not= name (:name main)) - (dwl/rename-component component-id name)) - - ;; Create variant container - (dwsh/create-artboard-from-shapes [main-instance-id] variant-id nil nil nil delta flex?) - (cl/remove-all-fills variant-vec {:color clr/black :opacity 1}) - (when flex? (dwsl/create-layout-from-id variant-id :flex)) - (dwsh/update-shapes variant-vec #(merge % cont-props)) - (dwsh/update-shapes [main-instance-id] #(merge % main-props)) - (cl/add-stroke variant-vec stroke-props) - (set-variant-id component-id variant-id)) - - ;; Add the necessary number of new properties, with default values - (rx/from - (repeatedly num-props - #(add-new-property variant-id {:fill-values? true}))) - - ;; When the component has path, set the path items as properties values - (when (> (count cpath) 1) - (rx/from - (map - #(update-property-value component-id % (nth cpath (inc %))) - (range num-props)))) - - (rx/of - (when duplicate? (add-new-variant main-instance-id)) - (dwsh/update-shapes variant-vec #(dissoc % :layout-item-absolute)) - (dwu/commit-undo-transaction undo-id) - (when flex? - (ptk/data-event :layout/update {:ids [variant-id]}))))))))) + (transform-in-variant-stream state main-instance-id variant-id delta prefix add-wrapper? duplicate? flex? nil))))) (defn add-component-or-variant "Manage the shared shortcut, and do the pertinent action" @@ -696,9 +708,11 @@ (rx/of (dcm/go-to-workspace :page-id page-id)) (rx/empty)) - (rx/of (dwu/start-undo-transaction undo-id) - (transform-in-variant (first ids) variant-id delta prefix add-wrapper? false false) - (dwsh/relocate-shapes (into #{} (rest ids)) variant-id 0) + (rx/of (dwu/start-undo-transaction undo-id)) + + (transform-in-variant-stream state (first ids) variant-id delta prefix add-wrapper? false false current-page) + + (rx/of (dwsh/relocate-shapes (into #{} (rest ids)) variant-id 0) ;; relocate-shapes inserts the shapes in layer-tree ;; order, so restore the intended order afterwards (order-variant-children variant-id ids) diff --git a/frontend/test/frontend_tests/data/workspace_variants_test.cljs b/frontend/test/frontend_tests/data/workspace_variants_test.cljs index d96a7823b8..44e68d6745 100644 --- a/frontend/test/frontend_tests/data/workspace_variants_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_variants_test.cljs @@ -6,16 +6,24 @@ (ns frontend-tests.data.workspace-variants-test (:require + [app.common.files.changes-builder :as pcb] + [app.common.files.variant :as cfv] [app.common.path-names :as cpn] [app.common.test-helpers.components :as cthc] + [app.common.test-helpers.compositions :as tho] [app.common.test-helpers.files :as cthf] [app.common.test-helpers.ids-map :as cthi] [app.common.test-helpers.shapes :as cths] [app.common.test-helpers.variants :as thv] + [app.common.types.component :as ctk] [app.common.types.variant :as ctv] + [app.common.uuid :as uuid] + [app.main.data.workspace :as dw] [app.main.data.workspace.variants :as dwv] + [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] - [frontend-tests.helpers.state :as ths])) + [frontend-tests.helpers.state :as ths] + [potok.v2.core :as ptk])) (t/use-fixtures :each {:before cthi/reset-idmap!}) @@ -152,3 +160,267 @@ (t/is (= clean-name (:name main2))) (t/is (= clean-name (cpn/merge-path-item (:path comp1) (:name comp1)))) (t/is (= clean-name (cpn/merge-path-item (:path comp2) (:name comp2)))))))))) + +;; --------------------------------------------------------------------------- +;; Create-set / combine: every commit must be a valid file +;; --------------------------------------------------------------------------- + +(def ^:private apply-local? + (ptk/type? :app.main.data.changes/apply-changes-localy)) + +(defn- simple-main-file + [] + (-> (cthf/sample-file :file1) + (tho/add-simple-component :c01 :m01 :r01))) + +(defn- two-mains-file + [] + (-> (cthf/sample-file :file1) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:name "Alpha" :x 0 :y 0 :width 100 :height 80}) + (tho/add-simple-component :c02 :m02 :r02 + :root-params {:name "Beta" :x 150 :y 0 :width 100 :height 80}))) + +(defn- two-mains-on-page1-with-page2 + "Two mains on :page1; the open page is :page2 (Assets combine while elsewhere)." + [] + (-> (cthf/sample-file :file1 :page-label :page1) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:name "Alpha" :x 0 :y 0 :width 100 :height 80}) + (tho/add-simple-component :c02 :m02 :r02 + :root-params {:name "Beta" :x 150 :y 0 :width 100 :height 80}) + (cthf/add-sample-page :page2) + (cthf/switch-to-page :page2))) + +(defn- post-artboard-file + "A normal frame holding one main — the state after create-artboard-from-shapes + and before the container flag." + [] + (-> (cthf/sample-file :file1) + (tho/add-frame :v01 {:name "Set"}) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:parent-label :v01 :name "Set"}))) + +(defn- page-objects + [file] + (:objects (cthf/current-page file))) + +(defn- page-objects-on + [file page-label] + (:objects (cthf/get-page file page-label))) + +(defn- commit-error + [file] + (try + (cthf/validate-file! file) + nil + (catch :default cause + (or (ex-message cause) (str cause))))) + +(defn- watch-applied-commits + "Record the file after every local apply. Keep the subscription for the + whole run-store; assertions run in the completed callback." + [store snapshots] + (->> (ptk/input-stream store) + (rx/filter apply-local?) + (rx/subs! (fn [_] + (let [file (ths/get-file-from-state @store)] + (swap! snapshots conj {:file file + :error (commit-error file)})))))) + +(defn- after-commits-idle + "Stop once applied commits have been idle. Combine schedules an async + undo commit after the main stream." + [stream] + (->> stream + (rx/filter apply-local?) + (rx/debounce 100))) + +(defn- assert-valid-snapshots + [snapshots] + (doseq [{:keys [error]} snapshots] + (t/is (nil? error) error))) + +(defn- first-flagged + [snapshots container-id] + (first (filter #(ctk/is-variant-container? + (get (page-objects (:file %)) container-id)) + snapshots))) + +(t/deftest define-variant-container-commit-validates + (let [file (post-artboard-file) + page (cthf/current-page file) + variant-id (cthi/id :v01) + main-id (cthi/id :m01) + component (cthc/get-component file :c01) + component-id (:id component) + name (:name (cths/get-shape file :v01)) + changes (-> (pcb/empty-changes nil (:id page)) + (pcb/with-objects (:objects page)) + (pcb/with-library-data (:data file)) + (dwv/generate-define-variant-container + variant-id main-id component-id name)) + file' (cthf/apply-changes file changes) + container' (cths/get-shape file' :v01) + main' (cths/get-shape file' :m01) + comp' (cthc/get-component file' :c01)] + + (t/is (ctk/is-variant-container? container')) + (t/is (= [main-id] (:shapes container'))) + (t/is (= variant-id (:variant-id main'))) + (t/is (= variant-id (:variant-id comp'))) + (t/is (some? (:variant-id comp'))) + (t/is (= (ctv/properties-to-name (:variant-properties comp')) + (:variant-name main')))))) + +(t/deftest transform-in-variant-validates-after-every-commit + (t/async + done + (let [file (simple-main-file) + main-id (cthi/id :m01) + variant-id (uuid/next) + snapshots (atom []) + store (ths/setup-store file)] + (watch-applied-commits store snapshots) + (ths/run-store + store + done + [(dwv/transform-in-variant main-id variant-id nil [] false true true)] + (fn [state] + (let [snaps @snapshots + flagged (first-flagged snaps variant-id) + file' (ths/get-file-from-state state) + objects' (page-objects file') + container' (get objects' variant-id) + main' (get objects' main-id) + comp' (cthc/get-component file' :c01) + before-flag (take-while + #(not (ctk/is-variant-container? + (get (page-objects (:file %)) variant-id))) + snaps)] + (assert-valid-snapshots snaps) + (t/is (some? flagged) "a commit must set :is-variant-container") + (doseq [snap before-flag] + (t/is (not (ctk/is-variant-container? + (get (page-objects (:file snap)) variant-id))))) + (when flagged + (let [ffile (:file flagged) + fobjs (page-objects ffile) + fcont (get fobjs variant-id) + fmain (get fobjs main-id) + fcomp (cthc/get-component ffile :c01)] + (t/is (= [main-id] (:shapes fcont))) + (t/is (ctk/main-instance? fmain)) + (t/is (= variant-id (:variant-id fmain))) + (t/is (= variant-id (:variant-id fcomp))) + (t/is (= (ctv/properties-to-name (:variant-properties fcomp)) + (:variant-name fmain))))) + (t/is (ctk/is-variant-container? container')) + (t/is (= variant-id (:variant-id main'))) + (t/is (= variant-id (:variant-id comp'))))) + after-commits-idle)))) + +(t/deftest add-new-variant-on-existing-set-still-validates + (t/async + done + (let [file (-> (two-property-file-on-other-page) + (cthf/switch-to-page :page1)) + main-id (cthi/id :m01) + c01-before (cthc/get-component file :c01) + c02-before (cthc/get-component file :c02) + last-c01 (-> c01-before :variant-properties last) + last-c02 (-> c02-before :variant-properties last)] + (run file + [(dwv/add-new-variant main-id)] + done + (fn [state] + (let [file' (ths/get-file-from-state state) + container' (cths/get-shape file' :v01 :page-label :page1) + c01' (cthc/get-component file' :c01) + c02' (cthc/get-component file' :c02) + new-id (first (:shapes container')) + new-shape (cths/get-shape-by-id file' new-id :page-label :page1) + new-comp (cthc/get-component-by-id file' (:component-id new-shape))] + (t/is (= 3 (count (:shapes container')))) + (t/is (= last-c01 (-> c01' :variant-properties last))) + (t/is (= last-c02 (-> c02' :variant-properties last))) + (t/is (not= last-c01 (-> new-comp :variant-properties last))) + (t/is (= (ctv/properties-to-name (:variant-properties new-comp)) + (:variant-name new-shape))))))))) + +(t/deftest combine-as-variants-validates-after-every-commit + (t/async + done + (let [file (two-mains-file) + id1 (cthi/id :m01) + id2 (cthi/id :m02) + variant-id (uuid/next) + snapshots (atom []) + store (ths/setup-store file)] + (watch-applied-commits store snapshots) + (ths/run-store + store + done + [(dwv/combine-as-variants [id1 id2] {:variant-id variant-id :trigger "test"})] + (fn [state] + (let [snaps @snapshots + file' (ths/get-file-from-state state) + objects (page-objects file') + container (get objects variant-id) + m01' (get objects id1) + m02' (get objects id2) + c01' (cthc/get-component file' :c01) + c02' (cthc/get-component file' :c02) + names1 (mapv :name (:variant-properties c01')) + names2 (mapv :name (:variant-properties c02')) + ordered (mapv :main-instance-id + (cfv/find-variant-components (:data file') objects variant-id))] + (assert-valid-snapshots snaps) + (doseq [{:keys [file]} snaps] + (let [objs (page-objects file) + cont (get objs variant-id) + kids (set (:shapes cont))] + (when (contains? kids id2) + (t/is (ctk/is-variant-container? cont) + "other mains must not enter the frame before it is a variant container")))) + (t/is (ctk/is-variant-container? container)) + (t/is (= names1 names2)) + (t/is (= [id1 id2] ordered)) + (t/is (= variant-id (:variant-id m01'))) + (t/is (= variant-id (:variant-id m02'))) + (t/is (= variant-id (:parent-id m01'))) + (t/is (= variant-id (:parent-id m02'))))) + after-commits-idle)))) + +(t/deftest combine-as-variants-from-another-page-validates-after-every-commit + (t/async + done + (let [file (two-mains-on-page1-with-page2) + id1 (cthi/id :m01) + id2 (cthi/id :m02) + page1 (cthi/id :page1) + variant-id (uuid/next) + snapshots (atom []) + store (ths/setup-store file)] + (watch-applied-commits store snapshots) + (ths/run-store + store + done + [(dwv/combine-as-variants [id1 id2] {:page-id page1 :variant-id variant-id :trigger "test"}) + (dw/initialize-page (:id file) page1)] + (fn [state] + (let [snaps @snapshots + file' (ths/get-file-from-state state) + objects (page-objects-on file' :page1) + container (get objects variant-id) + m01' (get objects id1) + m02' (get objects id2) + c01' (cthc/get-component file' :c01) + c02' (cthc/get-component file' :c02)] + (assert-valid-snapshots snaps) + (t/is (ctk/is-variant-container? container)) + (t/is (= variant-id (:variant-id m01'))) + (t/is (= variant-id (:variant-id m02'))) + (t/is (= variant-id (:variant-id c01'))) + (t/is (= variant-id (:variant-id c02'))))) + after-commits-idle))))