🐛 Fix layer orders messed up on move, group, reparent and undo

CHANGES.md

@@ -40,6 +40,7 @@
 - Fix wrong update of text in components [Taiga #4646](https://tree.taiga.io/project/penpot/issue/4646)
 - Fix problem with SVG imports with style [#2605](https://github.com/penpot/penpot/issues/2605)
 - Fix ghost shapes after sync groups in components [Taiga #4649](https://tree.taiga.io/project/penpot/issue/4649)
+- Fix layer orders messed up on move, group, reparent and undo [Github #2672](https://github.com/penpot/penpot/issues/2672)
 
 ## 1.16.2-beta
 

common/src/app/common/pages/changes.cljc

@@ -182,7 +182,7 @@
       (d/update-in-when data [:components component-id :objects] reg-objects))))
 
 (defmethod process-change :mov-objects
-  [data {:keys [parent-id shapes index page-id component-id ignore-touched]}]
+  [data {:keys [parent-id shapes index page-id component-id ignore-touched after-shape]}]
   (letfn [(calculate-invalid-targets [objects shape-id]
             (let [reduce-fn #(into %1 (calculate-invalid-targets objects %2))]
               (->> (get-in objects [shape-id :shapes])
@@ -260,9 +260,12 @@
                 (not= :frame (:type obj))
                 (as-> $$ (reduce (partial assign-frame-id frame-id) $$ (:shapes obj))))))
 
+
+
           (move-objects [objects]
             (let [valid?   (every? (partial is-valid-move? objects) shapes)
                   parent   (get objects parent-id)
+                  index (if (nil? after-shape) index (inc (d/index-of (:shapes parent) after-shape)))
                   frame-id (if (= :frame (:type parent))
                              (:id parent)
                              (:frame-id parent))]

common/src/app/common/pages/changes_builder.cljc

@@ -244,32 +244,26 @@
    (assert-page-id changes)
    (assert-objects changes)
    (let [objects (lookup-objects changes)
-
          set-parent-change
          (cond-> {:type :mov-objects
                   :parent-id parent-id
                   :page-id (::page-id (meta changes))
-                  :shapes (->> shapes (mapv :id))}
+                  :shapes (->> shapes reverse (mapv :id))}
 
            (some? index)
            (assoc :index index))
 
          mk-undo-change
          (fn [change-set shape]
-           (let [idx (or (cph/get-position-on-parent objects (:id shape)) 0)
+           (let [prev-sibling (cph/get-prev-sibling objects (:id shape))]
-                 ;; Different index if the movement was from top to bottom or the other way
-                 ;; Similar that on frontend/src/app/main/ui/workspace/sidebar/layers.cljs
-                 ;; with the 'side' property of the on-drop
-                 idx (if (< index idx)
-                       (inc idx)
-                       idx)]
            (d/preconj
              change-set
              {:type :mov-objects
               :page-id (::page-id (meta changes))
               :parent-id (:parent-id shape)
               :shapes [(:id shape)]
-              :index idx})))]
+              :after-shape prev-sibling
+              :index 0})))] ; index is used in case there is no after-shape (moving bottom shapes)
 
      (-> changes
          (update :redo-changes conj set-parent-change)

common/src/app/common/pages/helpers.cljc

@@ -147,6 +147,16 @@
         prt (get objects pid)]
     (d/index-of (:shapes prt) id)))
 
+(defn get-prev-sibling
+  [objects id]
+  (let [obj (get objects id)
+        pid (:parent-id obj)
+        prt (get objects pid)
+        shapes (:shapes prt)
+        pos (d/index-of shapes id)]
+    (if (= 0 pos) nil (nth shapes (dec pos)))))
+
+
 (defn get-immediate-children
   "Retrieve resolved shape objects that are immediate children
    of the specified shape-id"
@@ -336,6 +346,13 @@
          (map second)
          (into #{}))))
 
+(defn order-by-indexed-shapes
+  [objects ids]
+  (->> (indexed-shapes objects)
+       (sort-by first)
+       (filter (comp (into #{} ids) second))
+       (map second)))
+
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; SHAPES ORGANIZATION (PATH MANAGEMENT)
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

frontend/src/app/main/data/workspace.cljs

@@ -639,14 +639,15 @@
 (defn relocate-shapes-changes [it objects parents parent-id page-id to-index ids
                                groups-to-delete groups-to-unmask shapes-to-detach
                                shapes-to-reroot shapes-to-deroot shapes-to-unconstraint]
-  (let [shapes (map (d/getf objects) ids)]
+  (let [ordered-indexes (cph/order-by-indexed-shapes objects ids)
+        shapes (map (d/getf objects) ordered-indexes)]
 
     (-> (pcb/empty-changes it page-id)
         (pcb/with-objects objects)
 
         ; Move the shapes
         (pcb/change-parent parent-id
-                           (reverse shapes)
+                           shapes
                            to-index)
 
         ; Remove empty groups

frontend/src/app/main/data/workspace/bool.cljs

@@ -20,15 +20,11 @@
    [cuerdas.core :as str]
    [potok.core :as ptk]))
 
-(defn selected-shapes
+(defn selected-shapes-idx
   [state]
   (let [objects (wsh/lookup-page-objects state)]
     (->> (wsh/lookup-selected state)
-         (cph/clean-loops objects)
+         (cph/clean-loops objects))))
-         (map (d/getf objects))
-         (remove cph/frame-shape?)
-         (map #(assoc % ::index (cph/get-position-on-parent objects (:id %))))
-         (sort-by ::index))))
 
 (defn create-bool-data
   [bool-type name shapes objects]
@@ -92,10 +88,15 @@
             base-name (-> bool-type d/name str/capital (str "-1"))
             name (-> (ctt/retrieve-used-names objects)
                      (ctt/generate-unique-name base-name))
-            shapes  (selected-shapes state)]
+            ids  (selected-shapes-idx state)
+            ordered-indexes (cph/order-by-indexed-shapes objects ids)
+            shapes (->> ordered-indexes
+                        (map (d/getf objects))
+                        (remove cph/frame-shape?))]
 
         (when-not (empty? shapes)
-          (let [[boolean-data index] (create-bool-data bool-type name shapes objects)
+          (let [[boolean-data index] (create-bool-data bool-type name (reverse shapes) objects)
+                index (inc index)
                 shape-id (:id boolean-data)
                 changes (-> (pcb/empty-changes it page-id)
                             (pcb/with-objects objects)

frontend/src/app/main/data/workspace/groups.cljs

@@ -22,6 +22,7 @@
 (defn shapes-for-grouping
   [objects selected]
   (->> selected
+       (cph/order-by-indexed-shapes objects)
        (map #(get objects %))
        (map #(assoc % ::index (cph/get-position-on-parent objects (:id %))))
        (sort-by ::index)))
@@ -76,12 +77,13 @@
                         (ctst/generate-unique-name base-name)))
 
         selrect   (gsh/selection-rect shapes)
+        group-idx (inc (::index (last shapes)))
         group     (-> (cts/make-minimal-group frame-id selrect gname)
                       (cts/setup-shape selrect)
                       (assoc :shapes (mapv :id shapes)
                              :parent-id parent-id
                              :frame-id frame-id
-                             :index (::index (first shapes))))
+                             :index group-idx))
 
         ;; Shapes that are in a component, but are not root, must be detached,
         ;; because they will be now children of a non instance group.
@@ -93,8 +95,8 @@
 
         changes   (-> (pcb/empty-changes it page-id)
                       (pcb/with-objects objects)
-                      (pcb/add-object group {:index (::index (first shapes))})
+                      (pcb/add-object group {:index group-idx})
-                      (pcb/change-parent (:id group) shapes)
+                      (pcb/change-parent (:id group) (reverse shapes))
                       (pcb/update-shapes (map :id shapes-to-detach) ctk/detach-shape)
                       (pcb/remove-objects ids-to-delete))]
 
@@ -102,7 +104,9 @@
 
 (defn remove-group-changes
   [it page-id group objects]
-  (let [children  (mapv #(get objects %) (:shapes group))
+  (let [children (->> (:shapes group)
+                      (cph/order-by-indexed-shapes objects)
+                      (mapv #(get objects %)))
         parent-id (cph/get-parent-id objects (:id group))
         parent    (get objects parent-id)
 
@@ -125,8 +129,9 @@
 
 (defn remove-frame-changes
   [it page-id frame objects]
-
+  (let [children (->> (:shapes frame)
-  (let [children      (mapv #(get objects %) (:shapes frame))
+                      (cph/order-by-indexed-shapes objects)
+                      (mapv #(get objects %)))
         parent-id     (cph/get-parent-id objects (:id frame))
         idx-in-parent (cph/get-position-on-parent objects (:id frame))]
 

frontend/src/app/main/data/workspace/shapes.cljs

@@ -118,10 +118,8 @@
       (let [page-id  (:current-page-id state)
             objects (wsh/lookup-page-objects state page-id)
 
-            to-move-shapes
+            ordered-indexes (cph/order-by-indexed-shapes objects shapes)
-            (into []
+            to-move-shapes (map (d/getf objects) ordered-indexes)
-                  (map (d/getf objects))
-                  (reverse (ctst/sort-z-index objects shapes)))
 
             changes
             (when (d/not-empty? to-move-shapes)

frontend/src/app/main/data/workspace/transforms.cljs

@@ -685,6 +685,7 @@
 
             shapes (->> ids (cph/clean-loops objects) (keep lookup))
 
+
             moving-shapes
             (cond->> shapes
               (not layout?)
@@ -694,6 +695,9 @@
               (remove #(and (= (:frame-id %) frame-id)
                             (not= (:parent-id %) frame-id))))
 
+            ordered-indexes (cph/order-by-indexed-shapes objects (map :id moving-shapes))
+            moving-shapes (map (d/getf objects) ordered-indexes)
+
             all-parents
             (reduce (fn [res id]
                       (into res (cph/get-parent-ids objects id)))