🐛 Give unique values to same-name components combined as variants (#12881)

When components whose name matches the variant container are moved into
it, removing the container name leaves an empty value. Combining several
components named the same produced "Value 1" for the first variant and
empty values for the rest, so variants shared the same combination.

Assign the first unused "Value N" to the last property when all computed
values are blank, tracking values given within the same batch.

Combine moves the remaining mains into the container one at a time, in
the combine order, so the generated values follow that order instead of
the layer order.

Closes #12878

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

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

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)

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

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