diff --git a/CHANGES.md b/CHANGES.md index ec9393b316..591612e6f5 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -15,6 +15,7 @@ - Fix pan cursor not disabling viewport guides [Github #6985](https://github.com/penpot/penpot/issues/6985) - Fix viewport resize on locked shapes [Taiga #11974](https://tree.taiga.io/project/penpot/issue/11974) - Fix variants not syncronizing tokens on switch [Taiga #12290](https://tree.taiga.io/project/penpot/issue/12290) +- Fix nested variant in a component doesn't keep inherited overrides [Taiga #12299](https://tree.taiga.io/project/penpot/issue/12299) ## 2.11.0 (Unreleased) diff --git a/common/src/app/common/logic/variants.cljc b/common/src/app/common/logic/variants.cljc index e8c14d5405..c0dc0409d0 100644 --- a/common/src/app/common/logic/variants.cljc +++ b/common/src/app/common/logic/variants.cljc @@ -11,7 +11,8 @@ [app.common.types.container :as ctn] [app.common.types.file :as ctf] [app.common.types.variant :as ctv] - [app.common.uuid :as uuid])) + [app.common.uuid :as uuid] + [clojure.set :as set])) (defn generate-add-new-variant [changes shape variant-id new-component-id new-shape-id prop-num] @@ -67,7 +68,6 @@ [[] {}] shapes)))) - (defn- keep-swapped-item "As part of the keep-touched process on a switch, given a child on the original copy that was swapped (orig-swapped-child), and its related shape on the new copy @@ -88,7 +88,6 @@ current-parent (get objects (:parent-id related-shape-in-new)) pos (d/index-of (:shapes current-parent) (:id related-shape-in-new))] - (-> (pcb/concat-changes before-changes changes) ;; Move the previous shape to the new parent @@ -122,6 +121,44 @@ (subvec (vec ancestors) 1 (dec num-ancestors)))] (some ctk/get-swap-slot ancestors))) +(defn- find-shape-ref-child-of + "Get the shape referenced by the shape-ref of the near main of the shape, + recursively repeated until find a shape-ref with parent-id as ancestor. + It will return the shape or nil if it doesn't found any" + [container libraries shape parent-id] + (let [ref-shape (ctf/find-ref-shape nil container libraries shape + :with-context? true) + + ref-shape-container (when ref-shape (:container (meta ref-shape))) + ref-shape-parents-set (when ref-shape + (->> (cfh/get-parents (:objects ref-shape-container) (:id ref-shape)) + (into #{} d/xf:map-id)))] + + (if (or (nil? ref-shape) (contains? ref-shape-parents-set parent-id)) + ref-shape + (find-shape-ref-child-of ref-shape-container libraries ref-shape parent-id)))) + +(defn- get-ref-chain + "Returns a vector with the shape ref chain including itself" + [container libraries shape] + (loop [chain [shape] + current shape] + (if-let [ref (ctf/find-ref-shape nil container libraries current :with-context? true)] + (recur (conj chain ref) ref) + chain))) + +(defn- add-touched-from-ref-chain + "Adds to the :touched attr of a shape the content of + the :touched of all its chain of ref shapes" + [container libraries shape] + (let [chain (get-ref-chain container libraries shape) + more-touched (->> chain + (map :touched) + (remove nil?) + (apply set/union) + (remove ctk/swap-slot?) + set)] + (update shape :touched #(set/union (or % #{}) more-touched)))) (defn generate-keep-touched "This is used as part of the switch process, when you switch from @@ -141,7 +178,10 @@ ;; Ignore children of swapped items, because ;; they will be moved without change when ;; managing their swapped ancestor - orig-touched (->> (filter (comp seq :touched) original-shapes) + orig-touched (->> original-shapes + ;; Add to each shape also the touched of its ref chain + (map #(add-touched-from-ref-chain container libraries %)) + (filter (comp seq :touched)) (remove #(child-of-swapped? % page-objects @@ -158,7 +198,7 @@ ;; The original-shape is in a copy. For the relation rules, we need the referenced ;; shape on the main component - orig-ref-shape (ctf/find-ref-shape nil container libraries original-shape {:with-context? true}) + orig-ref-shape (ctf/find-remote-shape container libraries original-shape {:with-context? true}) orig-ref-objects (:objects (:container (meta orig-ref-shape))) ;; Adds a :shape-path attribute to the children of the orig-ref-shape, @@ -171,7 +211,6 @@ ;; Creates a map to quickly find a child of the orig-ref-shape by its shape-path o-ref-shapes-p-map (into {} (map (juxt :id :shape-path)) o-ref-shapes-wp) - ;; Process each touched children of the original-shape [changes parents-of-swapped] (reduce @@ -182,8 +221,7 @@ ;; orig-child-touched is in a copy. Get the referenced shape on the main component ;; If there is a swap slot, we will get the referenced shape in another way orig-ref-shape (when-not swap-slot - ;; TODO Maybe just get it from o-ref-shapes-wp - (ctf/find-ref-shape nil container libraries orig-child-touched)) + (find-shape-ref-child-of container libraries orig-child-touched (:id orig-ref-shape))) orig-ref-id (if swap-slot ;; If there is a swap slot, find the referenced shape id @@ -196,6 +234,7 @@ ;; Get its related shape in the children of new-shape: the one that ;; has the same shape-path related-shape-in-new (get new-shapes-map shape-path) + parents-of-swapped (if related-shape-in-new (conj parent-of-swapped (:parent-id related-shape-in-new)) parent-of-swapped) diff --git a/common/src/app/common/types/component.cljc b/common/src/app/common/types/component.cljc index 70266435a3..130bece565 100644 --- a/common/src/app/common/types/component.cljc +++ b/common/src/app/common/types/component.cljc @@ -286,7 +286,7 @@ (fn [touched] (into #{} (remove #(str/starts-with? (name %) "swap-slot-") touched))))) -(defn get-component-root +(defn get-deleted-component-root [component] (if (some? (:main-instance-id component)) (get-in component [:objects (:main-instance-id component)]) diff --git a/common/src/app/common/types/file.cljc b/common/src/app/common/types/file.cljc index 2c65cc21f2..5a2deaabaa 100644 --- a/common/src/app/common/types/file.cljc +++ b/common/src/app/common/types/file.cljc @@ -276,7 +276,7 @@ (-> file-data (get-component-page component) (ctn/get-shape (:main-instance-id component))) - (ctk/get-component-root component))) + (ctk/get-deleted-component-root component))) (defn get-component-shape "Retrieve one shape in the component by id. If with-context? is true, add the @@ -355,7 +355,7 @@ (defn find-remote-shape "Recursively go back by the :shape-ref of the shape until find the correct shape of the original component" - [container libraries shape] + [container libraries shape & {:keys [with-context?] :or {with-context? false}}] (let [top-instance (ctn/get-component-shape (:objects container) shape) component-file (get-in libraries [(:component-file top-instance) :data]) component (ctkl/get-component component-file (:component-id top-instance) true) @@ -375,8 +375,12 @@ (if (nil? remote-shape) nil (if (nil? (:shape-ref remote-shape)) - remote-shape - (find-remote-shape component-container libraries remote-shape))))) + (cond-> remote-shape + (and remote-shape with-context?) + (with-meta {:file {:id (:id file-data) + :data file-data} + :container component-container})) + (find-remote-shape component-container libraries remote-shape :with-context? with-context?))))) (defn direct-copy? "Check if the shape is in a direct copy of the component (i.e. the shape-ref points to shapes inside @@ -901,7 +905,7 @@ (println)) (when (seq (:objects component)) - (let [root (ctk/get-component-root component)] + (let [root (ctk/get-deleted-component-root component)] (dump-shape (:id root) 1 (:objects component) diff --git a/common/test/common_tests/logic/variants_switch_test.cljc b/common/test/common_tests/logic/variants_switch_test.cljc index 62d09bcec4..99a072b540 100644 --- a/common/test/common_tests/logic/variants_switch_test.cljc +++ b/common/test/common_tests/logic/variants_switch_test.cljc @@ -45,7 +45,6 @@ ;; The rect has width 15 after the switch (t/is (= (:width rect02') 15)))) - (t/deftest test-switch-with-override (let [;; ==== Setup file (-> (thf/sample-file :file1) @@ -125,12 +124,10 @@ ;; The rect has width 15 after the switch (t/is (= (:width rect02') 15)))) - (def font-size-path-paragraph [:content :children 0 :children 0 :font-size]) (def font-size-path-0 [:content :children 0 :children 0 :children 0 :font-size]) (def font-size-path-1 [:content :children 0 :children 0 :children 1 :font-size]) - (def text-path-0 [:content :children 0 :children 0 :children 0 :text]) (def text-path-1 [:content :children 0 :children 0 :children 1 :text]) (def text-lines-path [:content :children 0 :children 0 :children]) @@ -188,6 +185,8 @@ ;; The copy clean has no overrides + + copy-clean (ths/get-shape file :copy-clean) copy-clean-t (ths/get-shape file :copy-clean-t) @@ -209,6 +208,8 @@ ;; ==== Action: Switch all the copies + + file' (-> file (tho/swap-component copy-clean :c02 {:new-shape-label :copy-clean-2 :keep-touched? true}) (tho/swap-component copy-font-size :c02 {:new-shape-label :copy-font-size-2 :keep-touched? true}) @@ -234,6 +235,8 @@ ;; Before the switch: ;; * font size 14 ;; * text "hello world" + + (t/is (= (get-in copy-clean-t font-size-path-0) "14")) (t/is (= (get-in copy-clean-t text-path-0) "hello world")) @@ -248,6 +251,8 @@ ;; Before the switch: ;; * font size 25 ;; * text "hello world" + + (t/is (= (get-in copy-font-size-t font-size-path-0) "25")) (t/is (= (get-in copy-font-size-t text-path-0) "hello world")) @@ -306,6 +311,8 @@ ;; The copy clean has no overrides + + copy-clean (ths/get-shape file :copy-clean) copy-clean-t (ths/get-shape file :copy-clean-t) @@ -327,6 +334,8 @@ ;; ==== Action: Switch all the copies + + file' (-> file (tho/swap-component copy-clean :c02 {:new-shape-label :copy-clean-2 :keep-touched? true}) (tho/swap-component copy-font-size :c02 {:new-shape-label :copy-font-size-2 :keep-touched? true}) @@ -352,6 +361,8 @@ ;; Before the switch: ;; * font size 14 ;; * text "hello world" + + (t/is (= (get-in copy-clean-t font-size-path-0) "14")) (t/is (= (get-in copy-clean-t text-path-0) "hello world")) @@ -366,6 +377,8 @@ ;; Before the switch: ;; * font size 25 ;; * text "hello world" + + (t/is (= (get-in copy-font-size-t font-size-path-0) "25")) (t/is (= (get-in copy-font-size-t text-path-0) "hello world")) @@ -401,7 +414,6 @@ (t/is (= (get-in copy-both-t' font-size-path-0) "50")) (t/is (= (get-in copy-both-t' text-path-0) "text overriden")))) - (t/deftest test-switch-with-different-text-text-override (let [;; ==== Setup file (-> (thf/sample-file :file1) @@ -423,6 +435,8 @@ ;; The copy clean has no overrides + + copy-clean (ths/get-shape file :copy-clean) copy-clean-t (ths/get-shape file :copy-clean-t) @@ -444,6 +458,8 @@ ;; ==== Action: Switch all the copies + + file' (-> file (tho/swap-component copy-clean :c02 {:new-shape-label :copy-clean-2 :keep-touched? true}) (tho/swap-component copy-font-size :c02 {:new-shape-label :copy-font-size-2 :keep-touched? true}) @@ -469,6 +485,8 @@ ;; Before the switch: ;; * font size 14 ;; * text "hello world" + + (t/is (= (get-in copy-clean-t font-size-path-0) "14")) (t/is (= (get-in copy-clean-t text-path-0) "hello world")) @@ -483,6 +501,8 @@ ;; Before the switch: ;; * font size 25 ;; * text "hello world" + + (t/is (= (get-in copy-font-size-t font-size-path-0) "25")) (t/is (= (get-in copy-font-size-t text-path-0) "hello world")) @@ -518,7 +538,6 @@ (t/is (= (get-in copy-both-t' font-size-path-0) "25")) (t/is (= (get-in copy-both-t' text-path-0) "bye")))) - (t/deftest test-switch-with-different-text-and-prop-text-override (let [;; ==== Setup file (-> (thf/sample-file :file1) @@ -542,6 +561,8 @@ ;; The copy clean has no overrides + + copy-clean (ths/get-shape file :copy-clean) copy-clean-t (ths/get-shape file :copy-clean-t) @@ -563,6 +584,8 @@ ;; ==== Action: Switch all the copies + + file' (-> file (tho/swap-component copy-clean :c02 {:new-shape-label :copy-clean-2 :keep-touched? true}) (tho/swap-component copy-font-size :c02 {:new-shape-label :copy-font-size-2 :keep-touched? true}) @@ -588,6 +611,8 @@ ;; Before the switch: ;; * font size 14 ;; * text "hello world" + + (t/is (= (get-in copy-clean-t font-size-path-0) "14")) (t/is (= (get-in copy-clean-t text-path-0) "hello world")) @@ -602,6 +627,8 @@ ;; Before the switch: ;; * font size 25 ;; * text "hello world" + + (t/is (= (get-in copy-font-size-t font-size-path-0) "25")) (t/is (= (get-in copy-font-size-t text-path-0) "hello world")) @@ -637,7 +664,6 @@ (t/is (= (get-in copy-both-t' font-size-path-0) "50")) (t/is (= (get-in copy-both-t' text-path-0) "bye")))) - (t/deftest test-switch-with-identical-structure-text-override (let [;; ==== Setup file (-> (thf/sample-file :file1) @@ -657,6 +683,8 @@ ;; Duplicate a text line in copy-structure-clean + + file (change-structure file :copy-structure-clean-t) copy-structure-clean (ths/get-shape file :copy-structure-clean) copy-structure-clean-t (ths/get-shape file :copy-structure-clean-t) @@ -678,6 +706,8 @@ ;; ==== Action: Switch all the copies + + file' (-> file (tho/swap-component copy-structure-clean :c02 {:new-shape-label :copy-structure-clean-2 :keep-touched? true}) (tho/swap-component copy-structure-unif :c02 {:new-shape-label :copy-structure-unif-2 :keep-touched? true}) @@ -763,7 +793,6 @@ (t/is (= (get-in copy-structure-mixed-t' font-size-path-1) "40")) (t/is (= (get-in copy-structure-mixed-t' text-path-1) "new line 2")))) - (t/deftest test-switch-with-different-prop-structure-text-override (let [;; ==== Setup file (-> (thf/sample-file :file1) @@ -784,6 +813,8 @@ ;; Duplicate a text line in copy-structure-clean + + file (change-structure file :copy-structure-clean-t) copy-structure-clean (ths/get-shape file :copy-structure-clean) copy-structure-clean-t (ths/get-shape file :copy-structure-clean-t) @@ -805,6 +836,8 @@ ;; ==== Action: Switch all the copies + + file' (-> file (tho/swap-component copy-structure-clean :c02 {:new-shape-label :copy-structure-clean-2 :keep-touched? true}) (tho/swap-component copy-structure-unif :c02 {:new-shape-label :copy-structure-unif-2 :keep-touched? true}) @@ -906,6 +939,8 @@ ;; Duplicate a text line in copy-structure-clean + + file (change-structure file :copy-structure-clean-t) copy-structure-clean (ths/get-shape file :copy-structure-clean) copy-structure-clean-t (ths/get-shape file :copy-structure-clean-t) @@ -927,6 +962,8 @@ ;; ==== Action: Switch all the copies + + file' (-> file (tho/swap-component copy-structure-clean :c02 {:new-shape-label :copy-structure-clean-2 :keep-touched? true}) (tho/swap-component copy-structure-unif :c02 {:new-shape-label :copy-structure-unif-2 :keep-touched? true}) @@ -971,6 +1008,8 @@ ;; Second line: ;; * font size 25 ;; * text "new line 2" + + (t/is (= (get-in copy-structure-unif-t font-size-path-0) "25")) (t/is (= (get-in copy-structure-unif-t text-path-0) "new line 1")) (t/is (= (get-in copy-structure-unif-t font-size-path-1) "25")) @@ -992,6 +1031,8 @@ ;; Before the switch, second line: ;; * font size 40 ;; * text "new line 2" + + (t/is (= (get-in copy-structure-mixed-t font-size-path-0) "35")) (t/is (= (get-in copy-structure-mixed-t text-path-0) "new line 1")) (t/is (= (get-in copy-structure-mixed-t font-size-path-1) "40")) @@ -1025,6 +1066,8 @@ ;; Duplicate a text line in copy-structure-clean + + file (change-structure file :copy-structure-clean-t) copy-structure-clean (ths/get-shape file :copy-structure-clean) copy-structure-clean-t (ths/get-shape file :copy-structure-clean-t) @@ -1046,6 +1089,8 @@ ;; ==== Action: Switch all the copies + + file' (-> file (tho/swap-component copy-structure-clean :c02 {:new-shape-label :copy-structure-clean-2 :keep-touched? true}) (tho/swap-component copy-structure-unif :c02 {:new-shape-label :copy-structure-unif-2 :keep-touched? true}) @@ -1090,6 +1135,8 @@ ;; Second line: ;; * font size 25 ;; * text "new line 2" + + (t/is (= (get-in copy-structure-unif-t font-size-path-0) "25")) (t/is (= (get-in copy-structure-unif-t text-path-0) "new line 1")) (t/is (= (get-in copy-structure-unif-t font-size-path-1) "25")) @@ -1111,6 +1158,8 @@ ;; Before the switch, second line: ;; * font size 40 ;; * text "new line 2" + + (t/is (= (get-in copy-structure-mixed-t font-size-path-0) "35")) (t/is (= (get-in copy-structure-mixed-t text-path-0) "new line 1")) (t/is (= (get-in copy-structure-mixed-t font-size-path-1) "40")) @@ -1124,7 +1173,6 @@ (t/is (= (get-in copy-structure-mixed-t' text-path-0) "bye")) (t/is (nil? (get-in copy-structure-mixed-t' font-size-path-1))))) - (t/deftest test-switch-variant-for-other-with-same-nested-component (let [;; ==== Setup file (-> (thf/sample-file :file1) @@ -1144,6 +1192,8 @@ ;; On :copy-cp01, change the width of the rect + + changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page)) #{copy-cp01-rect-id} (fn [shape] @@ -1166,8 +1216,6 @@ ;; The width of copy-cp02-rect' is 25 (change is preserved) (t/is (= (:width copy-cp02-rect') 25)))) - - (t/deftest test-switch-variant-that-has-swaped-copy (let [;; ==== Setup file (-> (thf/sample-file :file1) @@ -1193,7 +1241,6 @@ ;; Switch :c01 for :c02 file' (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true}) - copy02' (ths/get-shape file' :copy02) copy-cp02' (ths/get-shape file' :copy-cp02)] (thf/dump-file file') @@ -1207,7 +1254,6 @@ ;;copy-02' had copy-cp02' as child (t/is (= (-> copy02' :shapes first) (:id copy-cp02'))))) - (t/deftest test-switch-variant-that-has-swaped-copy-with-changed-attr (let [;; ==== Setup file (-> (thf/sample-file :file1) @@ -1244,7 +1290,6 @@ ;; Switch :c01 for :c02 file' (tho/swap-component file copy01 :c02 {:new-shape-label :copy02 :keep-touched? true}) - copy02' (ths/get-shape file' :copy02) copy-cp02' (ths/get-shape file' :copy-cp02) copy-cp02-rect' (ths/get-shape-by-id file' (-> copy-cp02' :shapes first))] @@ -1262,3 +1307,58 @@ (t/is (= (-> copy02' :shapes first) (:id copy-cp02'))) ;; The width of copy-cp02-rect' is 25 (change is preserved) (t/is (= (:width copy-cp02-rect') 25)))) + +(t/deftest test-switch-variant-without-touched-but-touched-parent + (let [;; ==== Setup + file (-> (thf/sample-file :file1) + (thv/add-variant-with-child + :v01 :c01 :m01 :c02 :m02 :r01 :r02 + {:child1-params {:width 5} + :child2-params {:width 5}}) + (tho/add-simple-component :external01 :external01-root :external01-child) + + (thc/instantiate-component :c01 + :c01-in-root + :children-labels [:r01-in-c01-in-root] + :parent-label :external01-root)) + + ;; Make a change on r01-in-c01-in-root so it is touched + page (thf/current-page file) + r01-in-c01-in-root (ths/get-shape file :r01-in-c01-in-root) + + changes (cls/generate-update-shapes (pcb/empty-changes nil (:id page)) + #{(:id r01-in-c01-in-root)} + (fn [shape] + (assoc shape :width 25)) + (:objects page) + {}) + + file (thf/apply-changes file changes) + + + ;; Instantiate the component :external01 + + + file (thc/instantiate-component file + :external01 + :external-copy01 + :children-labels [:external-copy01-rect :c01-in-copy]) + page (thf/current-page file) + c01-in-copy (ths/get-shape file :c01-in-copy) + rect01 (get-in page [:objects (-> c01-in-copy :shapes first)]) + + + ;; ==== Action + + + file' (tho/swap-component file c01-in-copy :c02 {:new-shape-label :c02-in-copy :keep-touched? true}) + + page' (thf/current-page file') + c02-in-copy' (ths/get-shape file' :c02-in-copy) + rect02' (get-in page' [:objects (-> c02-in-copy' :shapes first)])] + + (thf/dump-file file :keys [:width :touched]) + ;; The rect had width 25 before the switch + (t/is (= (:width rect01) 25)) + ;; The rect still has width 25 after the switch + (t/is (= (:width rect02') 25)))) diff --git a/frontend/src/app/main/ui/workspace/sidebar/assets/common.cljs b/frontend/src/app/main/ui/workspace/sidebar/assets/common.cljs index 2e91af2fc5..130da0bb82 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/assets/common.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/assets/common.cljs @@ -391,7 +391,7 @@ ;; and the variant-container in which it will be restored still exists (fn [shape] (let [component (find-component shape true) - main (ctk/get-component-root component) + main (ctk/get-deleted-component-root component) objects (dm/get-in libraries [(:component-file shape) :data :pages-index