diff --git a/common/src/app/common/types/container.cljc b/common/src/app/common/types/container.cljc index 5e405a0995..9b569e04a7 100644 --- a/common/src/app/common/types/container.cljc +++ b/common/src/app/common/types/container.cljc @@ -414,6 +414,13 @@ true (has-any-copy-parent? objects (:parent-id shape)))))) +(defn detachable? + "Whether the shape can be detached from its component: a copy shape that + is not inside another copy." + [objects shape] + (and (ctk/in-component-copy? shape) + (not (has-any-copy-parent? objects shape)))) + (defn has-any-main? "Check if the shape is a main component or has any children or parent that is a main component." [objects shape] diff --git a/frontend/src/app/main/data/workspace/libraries.cljs b/frontend/src/app/main/data/workspace/libraries.cljs index d40d844379..0abab599fa 100644 --- a/frontend/src/app/main/data/workspace/libraries.cljs +++ b/frontend/src/app/main/data/workspace/libraries.cljs @@ -616,14 +616,14 @@ (watch [it state _] (let [page-id (:current-page-id state) file-id (:current-file-id state) + objects (dsh/lookup-page-objects state page-id) fdata (dsh/lookup-file-data state file-id) - libraries (dsh/lookup-libraries state) + libraries (dsh/lookup-libraries state)] - changes (-> (pcb/empty-changes it) - (cli/generate-detach-component id fdata page-id libraries))] - - (rx/of (dch/commit-changes changes)))))) + (when (ctn/detachable? objects (get objects id)) + (rx/of (dch/commit-changes (-> (pcb/empty-changes it) + (cli/generate-detach-component id fdata page-id libraries))))))))) (defn detach-components "Remove all references to components in the shapes with the given ids" diff --git a/frontend/src/app/main/data/workspace/variants.cljs b/frontend/src/app/main/data/workspace/variants.cljs index 9d5c3501af..3908a2dafc 100644 --- a/frontend/src/app/main/data/workspace/variants.cljs +++ b/frontend/src/app/main/data/workspace/variants.cljs @@ -109,13 +109,19 @@ (ev/event {::ev/name "variant-edit-property-name" ::ev/origin trigger}))))))) (defn update-property-value - "Updates the variant property value on the position pos in a component" - [component-id pos value] + "Set the value of the variant property at `pos` of the components + `component-ids`, and clear their errors." + [component-ids pos value] (ptk/reify ::update-property-value ptk/WatchEvent (watch [it state _] - (rx/of (dch/commit-changes (-> (variant-changes it state) - (clvp/generate-update-property-value component-id pos value))))))) + (rx/of (dch/commit-changes + (reduce (fn [changes id] + (-> changes + (clvp/generate-update-property-value id pos value) + (clvp/generate-set-variant-error id nil))) + (variant-changes it state) + component-ids)))))) (defn update-error "Sets or unsets an error for a component" @@ -442,7 +448,10 @@ (rx/of (combine-as-variants selected options)))))) (defn- variant-switch - "Switch the shape (that must be a variant copy head) for the closest one with the property value passed as parameter" + "Switch the shape (that must be a variant copy head) for the closest one + with the property value passed as parameter. A switch that would nest a + component in itself does nothing and calls the `:on-error` of the + metadata of `params`." [shape {:keys [pos val] :as params}] (ptk/reify ::variant-switch ptk/WatchEvent @@ -451,7 +460,7 @@ nearest-comp (clv/find-switch-target libraries shape pos val) {:keys [on-error] - :or {on-error rx/throw}} (meta params)] + :or {on-error (constantly nil)}} (meta params)] ;; Nothing to do when the value is already val or no variant has it (when nearest-comp (if (clv/swap-nesting-loop? (dsh/lookup-page-objects state) diff --git a/frontend/src/app/main/ui/workspace/sidebar/options/menus/component.cljs b/frontend/src/app/main/ui/workspace/sidebar/options/menus/component.cljs index 9d7bd0bc36..9e04782f1a 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/options/menus/component.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/options/menus/component.cljs @@ -391,11 +391,11 @@ (mf/deps component-ids) (fn [pos value] (let [value (ctv/normalize-property-text (d/nilv value ""))] - (doseq [id component-ids] - (st/emit! - (ev/event {::ev/name "variant-edit-property-value" ::ev/origin "workspace:combo-design-tab"}) - (dwv/update-property-value id pos value)) - (st/emit! (dwv/update-error id)))))) + (apply st/emit! + (dwv/update-property-value component-ids pos value) + (repeat (count component-ids) + (ev/event {::ev/name "variant-edit-property-value" + ::ev/origin "workspace:combo-design-tab"})))))) update-property-name (mf/use-fn diff --git a/frontend/src/app/plugins/library.cljs b/frontend/src/app/plugins/library.cljs index 56782eb1e2..9b36750b16 100644 --- a/frontend/src/app/plugins/library.cljs +++ b/frontend/src/app/plugins/library.cljs @@ -1029,7 +1029,7 @@ :else (st/emit! (se/event plugin-id "variant-edit-property-value") - (dwv/update-property-value id pos (ctv/normalize-property-text value)))))))) + (dwv/update-property-value [id] pos (ctv/normalize-property-text value)))))))) (defn library-proxy? [p] (obj/type-of? p "LibraryProxy")) diff --git a/frontend/src/app/plugins/shape.cljs b/frontend/src/app/plugins/shape.cljs index eb0a7df488..93fadaeb41 100644 --- a/frontend/src/app/plugins/shape.cljs +++ b/frontend/src/app/plugins/shape.cljs @@ -333,6 +333,10 @@ :else (st/emit! (dwsh/update-shapes [id] #(assoc % :exports value)))))) +(defn- copy-head? + [shape] + (and (ctk/instance-head? shape) (ctk/in-component-copy? shape))) + (defn shape-proxy? [p] (obj/type-of? p "ShapeProxy")) @@ -1556,6 +1560,9 @@ (not (u/page-active? page-id)) (u/not-valid plugin-id :detach "Cannot modify a page that is not currently active") + (not (ctn/detachable? (u/locate-objects file-id page-id) (u/locate-shape file-id page-id id))) + (u/not-valid plugin-id :detach "Only a copy that is not inside another copy can be detached") + :else (st/emit! (dwl/detach-component id)))) @@ -1566,7 +1573,7 @@ valid-component? (obj/type-of? component "LibraryComponentProxy") target-file (when valid-component? (obj/get component "$file")) target-id (when valid-component? (obj/get component "$id")) - target-data (some-> (u/locate-file target-file) :data)] + target-data (some-> target-file u/locate-file :data)] (cond (not (r/check-permission plugin-id "content:write")) (u/not-valid plugin-id :swapComponent "Plugin doesn't have 'content:write' permission") @@ -1574,12 +1581,12 @@ (not (u/page-active? page-id)) (u/not-valid plugin-id :swapComponent "Cannot modify a page that is not currently active") + (not (copy-head? shape)) + (u/not-valid plugin-id :swapComponent "The shape is not a component copy head") + (not valid-component?) (u/not-valid plugin-id :swapComponent "Component not valid") - (not (ctk/in-component-copy? shape)) - (u/not-valid plugin-id :swapComponent "The shape is not a component copy instance") - (clv/swap-nesting-loop? objects shape target-data target-id) (u/not-valid plugin-id :swapComponent "The swap would create a component nesting loop") @@ -1599,8 +1606,8 @@ (not (u/page-active? page-id)) (u/not-valid plugin-id :resetOverrides "Cannot modify a page that is not currently active") - (not (ctk/in-component-copy? shape)) - (u/not-valid plugin-id :resetOverrides "The shape is not a component copy instance") + (not (copy-head? shape)) + (u/not-valid plugin-id :resetOverrides "The shape is not a component copy head") :else (st/emit! (dwl/reset-component id))))) @@ -1847,7 +1854,10 @@ :else (let [shape (u/locate-shape file-id page-id id)] (if (clv/valid-switch? (dsh/lookup-libraries @st/state) shape pos value) - (st/emit! (-> (dwv/variants-switch {:shapes [shape] :pos pos :val value}) + (st/emit! (-> (dwv/variants-switch + (with-meta {:shapes [shape] :pos pos :val value} + {:on-error #(u/not-valid plugin-id :switchVariant + "The switch would create a component nesting loop")})) (se/add-event plugin-id))) (u/not-valid plugin-id :switchVariant "Shape, property, or value is not valid"))))) diff --git a/frontend/test/frontend_tests/composable_tests/interpreter.cljs b/frontend/test/frontend_tests/composable_tests/interpreter.cljs index fb6e3711be..5f55b4725e 100644 --- a/frontend/test/frontend_tests/composable_tests/interpreter.cljs +++ b/frontend/test/frontend_tests/composable_tests/interpreter.cljs @@ -237,7 +237,8 @@ :rename (let [[variant-id pos name] args] (dwv/update-property-name variant-id pos name {:trigger "workspace:design-tab"})) :reorder (apply dwv/reorder-variant-properties args) - :set-value (apply dwv/update-property-value args) + :set-value (let [[component-id pos value] args] + (dwv/update-property-value [component-id] pos value)) ;; the layers panel's rename of the member's main :rename-in-layers (let [{:keys [set-name params]} op] (dw/rename-shape-or-variant (:id (n/member-root situation set-name (:member params))) diff --git a/frontend/test/frontend_tests/data/workspace_variants_test.cljs b/frontend/test/frontend_tests/data/workspace_variants_test.cljs index 097ad663c7..c8c92054ce 100644 --- a/frontend/test/frontend_tests/data/workspace_variants_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_variants_test.cljs @@ -227,3 +227,19 @@ (let [data (:data (ths/get-file-from-state state))] (t/is (= (remove #{"Property 6"} (map :name (props "a"))) (map :name (:variant-properties (ctkl/get-component data (cthi/id :c01)))))))))))) + +(t/deftest set-a-property-value-on-several-members + (t/async + done + (let [file (-> (setup-file) + (cths/update-shape :m01 :variant-error "Duplicated") + (cths/update-shape :m02 :variant-error "Duplicated"))] + (ths/run-store + (ths/setup-store file) done + [(dwv/update-property-value [(cthi/id :c01) (cthi/id :c02)] 0 "Same")] + (fn [state] + (let [file' (ths/get-file-from-state state) + value #(get-in (ctkl/get-component (:data file') (cthi/id %)) [:variant-properties 0 :value])] + (t/is (= ["Same" "Same"] [(value :c01) (value :c02)])) + (t/is (nil? (:variant-error (cths/get-shape file' :m01)))) + (t/is (nil? (:variant-error (cths/get-shape file' :m02)))))))))) diff --git a/frontend/test/frontend_tests/plugins/shape_bugfixes_test.cljs b/frontend/test/frontend_tests/plugins/shape_bugfixes_test.cljs index 7dca8b7af8..59c688a3b4 100644 --- a/frontend/test/frontend_tests/plugins/shape_bugfixes_test.cljs +++ b/frontend/test/frontend_tests/plugins/shape_bugfixes_test.cljs @@ -7,7 +7,10 @@ (ns frontend-tests.plugins.shape-bugfixes-test (:require [app.common.data :as d] + [app.common.test-helpers.components :as cthc] [app.common.test-helpers.files :as cthf] + [app.common.test-helpers.ids-map :as cthi] + [app.common.test-helpers.variants :as cthv] [app.common.types.component :as ctk] [app.common.uuid :as uuid] [app.main.data.workspace :as dw] @@ -410,3 +413,85 @@ (t/is (= 1 (count @errors))) (t/is (= [plugin-id :combineAsVariants "Plugin doesn't have 'content:write' permission"] (first @errors))))))) + +(defn- plugin-errors-of + "The errors the plugin reports when `call` runs on the proxy of the shape + `shape-id` of `objects`, with every check passing but the shape's own." + [objects shape-id call] + (let [file-id (uuid/next) + page-id (uuid/next) + errors (atom []) + emitted (atom [])] + (with-redefs [u/locate-shape (fn [_ _ id] (get objects id)) + u/locate-objects (fn ([] objects) ([_ _] objects)) + u/page-active? (constantly true) + r/check-permission (constantly true) + u/not-valid (mock/stub (fn [_ prop msg] (swap! errors conj [prop msg]))) + st/emit! (mock/stub (fn [& events] (swap! emitted into events)))] + (call (shape/shape-proxy plugin-id file-id page-id shape-id))) + {:errors @errors :emitted @emitted})) + +(defn- nested-copy-objects + "A copy root holding a nested copy head, which holds a child." + [] + (let [root (uuid/next) + head (uuid/next) + child (uuid/next)] + {:root root + :head head + :child child + :objects {root {:id root :type :frame :parent-id uuid/zero :shapes [head] + :component-id (uuid/next) :component-file (uuid/next) + :component-root true :shape-ref (uuid/next)} + head {:id head :type :frame :parent-id root :shapes [child] + :component-id (uuid/next) :component-file (uuid/next) + :shape-ref (uuid/next)} + child {:id child :type :rect :parent-id head :shape-ref (uuid/next)}}})) + +(t/deftest swap-component-rejects-a-shape-that-is-no-copy-head + (let [{:keys [objects child]} (nested-copy-objects) + {:keys [errors emitted]} (plugin-errors-of objects child #(.swapComponent ^js % #js {}))] + (t/is (= [[:swapComponent "The shape is not a component copy head"]] errors)) + (t/is (empty? emitted)))) + +(t/deftest reset-overrides-rejects-a-shape-that-is-no-copy-head + (let [{:keys [objects child]} (nested-copy-objects) + {:keys [errors emitted]} (plugin-errors-of objects child #(.resetOverrides ^js %))] + (t/is (= [[:resetOverrides "The shape is not a component copy head"]] errors)) + (t/is (empty? emitted)))) + +(t/deftest detach-rejects-a-copy-nested-in-a-copy + (let [{:keys [objects head]} (nested-copy-objects) + {:keys [errors emitted]} (plugin-errors-of objects head #(.detach ^js %))] + (t/is (= [[:detach "Only a copy that is not inside another copy can be detached"]] errors)) + (t/is (empty? emitted)))) + +(t/deftest swap-component-rejects-a-value-that-is-no-component + (let [{:keys [objects head]} (nested-copy-objects) + {:keys [errors emitted]} (plugin-errors-of objects head #(.swapComponent ^js % #js {}))] + (t/is (= [[:swapComponent "Component not valid"]] errors)) + (t/is (empty? emitted)))) + +(t/deftest switch-variant-into-a-nesting-loop-reports-it + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (cthv/add-variant :v01 :c01 :m01 :c02 :m02) + (cthc/instantiate-component :c01 :nested :parent-label :m02)) + store (ths/setup-store file) + errors (atom [])] + (set! st/state store) + (mock/with-mocks + {u/not-valid (fn [_ prop msg] (swap! errors conj [prop msg])) + r/check-permission (constantly true) + u/page-active? (constantly true)} + (fn [done'] + (.switchVariant (shape/shape-proxy plugin-id (:id file) (cthf/current-page-id file) + (cthi/id :nested)) + 0 "Value2") + (js/setTimeout + (fn [] + (t/is (= [[:switchVariant "The switch would create a component nesting loop"]] @errors)) + (done')) + 50)) + done))))