diff --git a/common/src/app/common/logic/variant_properties.cljc b/common/src/app/common/logic/variant_properties.cljc index 3e950a81b8..02490a3c81 100644 --- a/common/src/app/common/logic/variant_properties.cljc +++ b/common/src/app/common/logic/variant_properties.cljc @@ -196,6 +196,19 @@ shape-name (ctv/remove-prefix (:name shape) container-name)] (ctv/path-to-properties shape-name base-properties min-props))) +(defn- fill-blank-properties + "When every value in `props` is blank, set the last one to the first + `Value N` not contained in `used-values`, so the variant does not end up + with an empty combination." + [props used-values] + (if (and (seq props) (every? (comp str/blank? :value) props)) + (let [value (->> (iterate inc 1) + (map #(str ctv/value-prefix %)) + (remove used-values) + first)] + (assoc-in (vec props) [(dec (count props)) :value] value)) + props)) + (defn generate-make-shapes-variant [changes shapes variant-container] @@ -244,24 +257,32 @@ changes (pcb/update-shapes changes (map :id shapes) #(assoc % :variant-id variant-id - :name (:name variant-container)))] - (reduce - (fn [changes shape] - (let [component (ctcl/get-component data (:component-id shape) true)] - (if (or (zero? num-shapes) ;; do nothing if there are no shapes - (and (= variant-id (:variant-id shape)) ;; or we are only moving the shape inside its parent (it is - (not (:deleted component)))) ;; the same parent and the component isn't deleted) - changes - (let [props (create-new-properties shape total-props) - variant-name (ctv/properties-to-name props)] - (-> (pcb/update-component changes - (:component-id shape) - #(assoc % :variant-id variant-id - :variant-properties props - :name cname - :path cpath) - {:apply-changes-local-library? true}) - (pcb/update-shapes [(:id shape)] - #(assoc % :variant-name variant-name))))))) - changes - shapes))) + :name (:name variant-container))) + + ;; Property values already present in the container's variants + used-values (->> (cfv/extract-properties-values data objects variant-id) + (mapcat :value) + set)] + (first + (reduce + (fn [[changes used-values] shape] + (let [component (ctcl/get-component data (:component-id shape) true)] + (if (or (zero? num-shapes) ;; do nothing if there are no shapes + (and (= variant-id (:variant-id shape)) ;; or we are only moving the shape inside its parent (it is + (not (:deleted component)))) ;; the same parent and the component isn't deleted) + [changes used-values] + (let [props (-> (create-new-properties shape total-props) + (fill-blank-properties used-values)) + variant-name (ctv/properties-to-name props)] + [(-> (pcb/update-component changes + (:component-id shape) + #(assoc % :variant-id variant-id + :variant-properties props + :name cname + :path cpath) + {:apply-changes-local-library? true}) + (pcb/update-shapes [(:id shape)] + #(assoc % :variant-name variant-name))) + (into used-values (map :value) props)])))) + [changes used-values] + shapes)))) diff --git a/common/test/common_tests/logic/variants_test.cljc b/common/test/common_tests/logic/variants_test.cljc index 0323df3afb..8162cf5598 100644 --- a/common/test/common_tests/logic/variants_test.cljc +++ b/common/test/common_tests/logic/variants_test.cljc @@ -412,6 +412,42 @@ (t/is (contains? values "Bigred")))) +(t/deftest test-make-shapes-variant-same-name-components-get-unique-values + ;; Mirrors combine-as-variants with components that share the container + ;; name: the first main defines the container, the rest are moved into it. + (let [file (-> (thf/sample-file :file1) + (tho/add-frame :v01 {:name "Tag"}) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:parent-label :v01 :name "Tag"}) + (tho/add-simple-component :c02 :m02 :r02 + :root-params {:name "Tag"}) + (tho/add-simple-component :c03 :m03 :r03 + :root-params {:name "Tag"})) + page (thf/current-page file) + define (-> (pcb/empty-changes nil (:id page)) + (pcb/with-objects (:objects page)) + (pcb/with-library-data (:data file)) + (clv/generate-define-variant-container + (thi/id :v01) (thi/id :m01) (thi/id :c01) "Tag")) + file (thf/apply-changes file define) + + page (thf/current-page file) + container (ths/get-shape file :v01) + shapes [(ths/get-shape file :m02) (ths/get-shape file :m03)] + changes (-> (pcb/empty-changes nil) + (pcb/with-page-id (:id page)) + (pcb/with-library-data (:data file)) + (pcb/with-objects (:objects page)) + (clvp/generate-make-shapes-variant shapes container) + (pcb/change-parent (:id container) shapes 0)) + file' (thf/apply-changes file changes) + values (mapv #(-> (thc/get-component file' %) :variant-properties first :value) + [:c01 :c02 :c03])] + + (t/is (= ["Value 1" "Value 2" "Value 3"] values)) + (t/is (= "Value 2" (:variant-name (ths/get-shape file' :m02)))) + (t/is (= "Value 3" (:variant-name (ths/get-shape file' :m03)))))) + (t/deftest test-duplicate-variant-container (let [;; ==== Setup diff --git a/frontend/src/app/main/data/workspace/variants.cljs b/frontend/src/app/main/data/workspace/variants.cljs index 46730ee112..49eb435f34 100644 --- a/frontend/src/app/main/data/workspace/variants.cljs +++ b/frontend/src/app/main/data/workspace/variants.cljs @@ -697,10 +697,13 @@ (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) + ;; One relocation per main, in `ids` order, so the + ;; generated property values follow the combine order + (rx/from (map #(dwsh/relocate-shapes #{%} variant-id 0) (rest ids))) + + ;; Each relocation inserts at index 0, so restore the + ;; intended order afterwards + (rx/of (order-variant-children variant-id ids) (dwsh/update-shapes ids #(-> % (assoc :constraints-h :left) (assoc :constraints-v :top) diff --git a/frontend/test/frontend_tests/data/workspace_variants_test.cljs b/frontend/test/frontend_tests/data/workspace_variants_test.cljs index 20c2fa7acf..d596add6c2 100644 --- a/frontend/test/frontend_tests/data/workspace_variants_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_variants_test.cljs @@ -190,6 +190,16 @@ (tho/add-simple-component :c02 :m02 :r02 :root-params {:name "UI / Beta" :x 150 :y 0 :width 100 :height 80}))) +(defn- three-same-name-mains-file + [] + (-> (cthf/sample-file :file1) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:name "Tag" :x 0 :y 0 :width 100 :height 80}) + (tho/add-simple-component :c02 :m02 :r02 + :root-params {:name "Tag" :x 150 :y 0 :width 100 :height 80}) + (tho/add-simple-component :c03 :m03 :r03 + :root-params {:name "Tag" :x 300 :y 0 :width 100 :height 80}))) + (defn- path-named-main-file [] (-> (cthf/sample-file :file1) @@ -524,6 +534,49 @@ (t/is (= variant-id (:parent-id m01') (:parent-id m02'))))) after-commits-idle)))) +(t/deftest combine-as-variants-same-name-gives-unique-values + (t/async + done + (let [file (three-same-name-mains-file) + ids [(cthi/id :m01) (cthi/id :m02) (cthi/id :m03)] + variant-id (uuid/next) + store (ths/setup-store file)] + (ths/run-store + store + done + [(dwv/combine-as-variants ids {:variant-id variant-id :trigger "test"})] + (fn [state] + (let [file' (ths/get-file-from-state state) + values (mapv #(-> (cthc/get-component file' %) :variant-properties first :value) + [:c01 :c02 :c03])] + (cthf/validate-file! file') + (t/is (= ["Value 1" "Value 2" "Value 3"] values)))) + after-commits-idle)))) + +(t/deftest combine-as-variants-same-name-values-follow-combine-order + ;; The combine order differs from the layer order of the mains + (t/async + done + (let [file (three-same-name-mains-file) + ids [(cthi/id :m03) (cthi/id :m01) (cthi/id :m02)] + variant-id (uuid/next) + store (ths/setup-store file)] + (ths/run-store + store + done + [(dwv/combine-as-variants ids {:variant-id variant-id :trigger "test"})] + (fn [state] + (let [file' (ths/get-file-from-state state) + objects (page-objects file') + values (mapv #(-> (cthc/get-component file' %) :variant-properties first :value) + [:c03 :c01 :c02]) + ordered (->> (cfv/find-variant-components (:data file') objects variant-id) + (mapv #(-> % :variant-properties first :value)))] + (cthf/validate-file! file') + (t/is (= ["Value 1" "Value 2" "Value 3"] values)) + (t/is (= ["Value 1" "Value 2" "Value 3"] ordered)))) + after-commits-idle)))) + (t/deftest combine-as-variants-from-another-page-validates-after-every-commit (t/async done diff --git a/plugins/apps/plugin-api-test-suite/src/tests/variants.test.ts b/plugins/apps/plugin-api-test-suite/src/tests/variants.test.ts index 451e75f91b..ace7b44d32 100644 --- a/plugins/apps/plugin-api-test-suite/src/tests/variants.test.ts +++ b/plugins/apps/plugin-api-test-suite/src/tests/variants.test.ts @@ -41,6 +41,27 @@ function sameIds(a: string[], b: string[]): boolean { return a.length === b.length && a.every((id, i) => id === b[i]); } +// Value of the first property for each variant component, in variant order. +function firstPropertyValues(container: VariantContainer): string[] { + const variants = container.variants; + if (!variants) return []; + const property = variants.properties[0]; + return variants + .variantComponents() + .map((c) => (c as LibraryVariantComponent).variantProps[property]); +} + +function variantValueOf( + container: VariantContainer, + componentId: string, +): string | undefined { + const variants = container.variants; + const component = variants + ?.variantComponents() + .find((c) => c.id === componentId) as LibraryVariantComponent | undefined; + return component?.variantProps[variants!.properties[0]]; +} + // Polls until the condition holds. On timeout it returns normally so the // assertion that follows reports the actual mismatch. async function waitFor( @@ -134,6 +155,72 @@ describe('Variants', () => { expect(variantComponentIds(container)).toEqual(expected); }); + // The components built here all share the name "Rectangle". Combining + // components with the same name must give every variant a unique, + // non-empty value, numbered in the combine order (#12878). + test('createVariantFromComponents gives same-name components unique values in input order', async (ctx) => { + const a = componentWithMain(ctx); + const b = componentWithMain(ctx); + const c = componentWithMain(ctx); + const container = ctx.penpot.createVariantFromComponents([ + c.main, + a.main, + b.main, + ]); + const expected = ['Value 1', 'Value 2', 'Value 3']; + await waitFor(() => sameIds(firstPropertyValues(container), expected)); + expect(firstPropertyValues(container)).toEqual(expected); + expect(variantComponentIds(container)).toEqual([ + c.comp.id, + a.comp.id, + b.comp.id, + ]); + }); + + test('combineAsVariants gives same-name components unique values in combine order', async (ctx) => { + const a = componentWithMain(ctx); + const b = componentWithMain(ctx); + const c = componentWithMain(ctx); + const container = a.main.combineAsVariants([b.main.id, c.main.id]); + const expected = ['Value 1', 'Value 2', 'Value 3']; + await waitFor(() => sameIds(firstPropertyValues(container), expected)); + expect(firstPropertyValues(container)).toEqual(expected); + expect(variantComponentIds(container)).toEqual([ + a.comp.id, + b.comp.id, + c.comp.id, + ]); + }); + + test('a same-name main moved into a variant container takes the first free value', async (ctx) => { + const a = componentWithMain(ctx); + const b = componentWithMain(ctx); + const c = componentWithMain(ctx); + const container = ctx.penpot.createVariantFromComponents([ + a.main, + b.main, + c.main, + ]); + await waitFor(() => firstPropertyValues(container).includes('Value 2')); + + // Free "Value 2" so the moved main has a gap to fill. + const withValue2 = container + .variants!.variantComponents() + .find( + (comp) => variantValueOf(container, comp.id) === 'Value 2', + ) as LibraryVariantComponent; + withValue2.setVariantProperty(0, 'Renamed'); + await waitFor(() => !firstPropertyValues(container).includes('Value 2')); + + const d = componentWithMain(ctx); + container.appendChild(d.main); + await waitFor(() => variantValueOf(container, d.comp.id) !== undefined); + expect(variantValueOf(container, d.comp.id)).toBe('Value 2'); + + const values = firstPropertyValues(container); + expect(new Set(values).size).toBe(values.length); + }); + test('variant component exposes variant props and Variants', (ctx) => { const rect = ctx.penpot.createRectangle(); ctx.board.appendChild(rect);