github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-29 05:50:35 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Keep guides and reflows in the undo group of their step

Browse Source 
Two derived commits that left the copies' sync in an undo entry of its own
(Phase 3c).

- Move frame guides in the undo group of their transform.

  A transform of a board sends the move of its guides before the shapes'
  update. The guide move committed every guide of the board, moved or not,
  each in a commit with an undo group of its own, so when the board was a
  main with a guide, its transform and the copies' sync landed in different
  undo entries: one undo reverted only the copies (F6).

  Both transform paths now give the shapes' update an undo group up front
  and pass it to the guide move, which commits only the guides that move,
  all in one commit in that group.

  workspace-frame-guides-undo-test: one undo reverts a rotation of a main
  with a guide, its copy included, and a move of a main together with its
  guide.

- Reflow layouts in the undo group of the step that asked for it.

  A layout reflow runs a moment after the commit that needs it: the
  :layout/update events are buffered for 100 ms and reflowed per page. The
  reflow lost the undo group of the step (F43):
  - the buffer passed no undo group to the reflow;
  - update-shapes asked for a reflow without the group of its commit;
  - the reflow's transform opened an undo transaction, so it made an entry
    of its own instead of stacking on the latest one, and the sync that a
    reflow of a main starts took that entry's fresh group.

  So one undo after a sync that reflows (a flex main's margin edit, a nested
  sync with minimal frames, a swap) reverted only the reflow, or only part
  of the step.

  update-shapes now gives a commit that needs a reflow an undo group up
  front and passes it on; the buffer reflows each page in the group an
  update of that buffer names (those that name none come from the same step,
  like update-layout-child's); the reflow stacks on the latest entry with no
  transaction of its own.

  Cases AJ and AN no longer pin F43: one undo after a nested sync reverts
  the remote change and the float noise of the reflow, and one undo after a
  flex main's margin edit reverts the margin, the sync and the reflow. The
  frontend golden master records that.

AI-assisted-by: claude-opus-5-5
This commit is contained in:
alonso.torres 2026-10-15 15:11:30 +02:00
parent adbddca19a
commit eed3a9ba60
8 changed files with 139 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
common/src/app/common/test_helpers/composable/comp/cases.cljc
View File

@ -625,18 +625,16 @@
"Case AJ (frontend only). A component nested two levels deep, with the
minimal frame size, has a copy; its remote main changes the rect, then one
undo runs. The transform math rewrites the minimal nested frames with float
noise, so the sync of the change reflows them, and the reflow commits land
in undo entries of their own (F43). One undo should revert the change and
its sync; it reverts only the last reflow entry, so the remote rect keeps
the change. How many entries the reflow makes depends on when its 100 ms
buffer flushes, so the case checks the value one undo leaves, never the
undo stack."
noise, so the sync of the change reflows them. The reflow joins the undo
group of the change, so one undo reverts the change, its sync and the
reflow: the remote rect is back to red. How many entries the reflow makes
depends on when its 100 ms buffer flushes, so the case checks the value
one undo leaves, never the undo stack."
[]
(let [m "main"
remote (n/remote-rect-of m)]
{:setup setup/empty-situation
;; the round trip's signature depends on when the reflow buffer flushes
:undo-check {:off "the case undoes its own step (F43)"}
:undo-check {:off "the case undoes its own step"}
:operation (tm/in-sequence
[(n/create-component m red)
(n/make-nested-component m)
@ -645,8 +643,7 @@
(n/change-property remote :fills blue)
(n/undo)])
:asserter (fn [s]
(tm/known-failure "F43" red blue
(fill-color (tm/shape-by-id s (remote s)))))}))
(t/is (= red (fill-color (tm/shape-by-id s (remote s))))))}))
(defn text-override-merges-with-main-text-change
"Case Y. A main text changes its characters or its font size; before that,
@ -1085,17 +1082,6 @@
update-main (n/update-main :copy-root)
follow-up (tm/one-of [main-change reset update-main])]
{:setup setup/flex-component-with-copy
;; the reflow after the sync of the main's margin lands in undo entries
;; of its own, so one undo leaves the margin and the reflow in place
:undo-check {:known-failures [{:bug "F43"
:fails {:undo #{[:object :main-child :layout-item-margin]
[:object :main-child :layout-item-margin-type]
[:object :main-child :x]
[:object :main-child :selrect]
[:object :main-child :points]}
:undo-index #{[:undo-index 2]}}
:op main-change
:runners #{:frontend}}]}
:operation (tm/in-sequence [override follow-up])
:asserter (fn [s]
(let [copy (tm/role-shape s :copy-instance)

33
frontend/src/app/main/data/workspace/guides.cljs
View File

@ -115,16 +115,17 @@
(every? uuid? (:ids args)))
(ptk/reify ::move-frame-guides
ptk/WatchEvent
(watch [_ state _]
(watch [it state _]
(let [ids (:ids args)
object-modifiers (:modifiers args)
object-transforms (:transforms args)
objects (dsh/lookup-page-objects state)
page (dsh/lookup-page state)
objects (:objects page)
is-frame? (fn [id] (= :frame (get-in objects [id :type])))
frame-ids? (into #{} (filter is-frame?) ids)
build-move-event
move-guide
(fn [guide]
(let [frame-id (:frame-id guide)
frame (get objects frame-id)
@ -140,17 +141,25 @@
(gsh/apply-transform transform))
moved (gpt/to-vec (gpt/point (:x frame) (:y frame))
(gpt/point (:x frame') (:y frame')))
(gpt/point (:x frame') (:y frame')))]
(update guide :position + (get moved (:axis guide)))))
guide (update guide :position + (get moved (:axis guide)))]
(update-guides guide)))
;; one commit, in the undo group of the transform, with the
;; guides that move
changes
(->> (:guides page)
(vals)
(filter (comp frame-ids? :frame-id))
(keep (fn [guide]
(let [guide' (move-guide guide)]
(when (not= guide guide') guide'))))
(reduce (fn [changes guide] (pcb/set-guide changes (:id guide) guide))
(-> (pcb/empty-changes it)
(pcb/with-page page)
(pcb/set-undo-group (:undo-group args)))))]
guides (-> state dsh/lookup-page :guides vals)]
(->> guides
(filter (comp frame-ids? :frame-id))
(map build-move-event)
(rx/from))))))
(when (seq (:redo-changes changes))
(rx/of (dwc/commit-changes changes)))))))
(defn update-guide-color
[guide-id color]

17
frontend/src/app/main/data/workspace/modifiers.cljs
View File

@ -687,8 +687,14 @@
ignore-tree
(clp/calculate-ignore-tree objects (keys transforms) transform-shape)
;; the guides move in a commit of their own, in the undo
;; group of the shapes, so the copies' sync joins them both
undo-group
(or (:undo-group params) (uuid/next))
options
(-> params
(assoc :undo-group undo-group)
(assoc :reg-objects? true)
(assoc :ignore-tree ignore-tree)
(assoc :translation? translation?)
@ -728,7 +734,7 @@
(rx/empty))
(rx/of
(clear-local-transform)
(ptk/event ::dwg/move-frame-guides {:ids ids :transforms transforms})
(ptk/event ::dwg/move-frame-guides {:ids ids :transforms transforms :undo-group undo-group})
(ptk/event ::dwcm/move-frame-comment-threads transforms)
(dwsh/update-shapes ids update-shape options))
@ -839,8 +845,14 @@
(clp/calculate-ignore-tree objects (keys object-modifiers)
(clp/modifiers-transform object-modifiers))
;; the guides move in a commit of their own, in the undo group of
;; the shapes, so the copies' sync joins them both
undo-group
(or (:undo-group options) (uuid/next))
options
(-> options
(assoc :undo-group undo-group)
(assoc :reg-objects? true)
(assoc :ignore-tree ignore-tree)
;; Attributes that can change in the transform. This
@ -860,7 +872,8 @@
(cond-> (d/not-empty? pos-data)
(assoc-position-data pos-data shape)))))]
(rx/of (ptk/event ::dwg/move-frame-guides {:ids ids-with-children :modifiers object-modifiers})
(rx/of (ptk/event ::dwg/move-frame-guides {:ids ids-with-children :modifiers object-modifiers
:undo-group undo-group})
(ptk/event ::dwcm/move-frame-comment-threads ids-with-children)
(dwsh/update-shapes ids update-shape options))))))

21
frontend/src/app/main/data/workspace/shape_layout.cljs
View File

@ -61,14 +61,18 @@
update-positions-stream
(if (d/not-empty? ids)
(let [modif-tree (dwm/create-modif-tree ids (ctm/reflow-modifiers))]
;; the reflow stacks on the latest undo entry, which a
;; transaction of its own would not
(if (features/active-feature? state "render-wasm/v1")
(rx/of (dwm/apply-wasm-modifiers modif-tree
:stack-undo? true
:undo-transation? false
:undo-group undo-group
:ignore-touched true))
(rx/of (dwm/apply-modifiers {:page-id page-id
:modifiers modif-tree
:stack-undo? true
:undo-transation? false
:ignore-touched true
:undo-group undo-group}))))
(rx/empty))
@ -112,14 +116,19 @@
(rx/map #(assoc % ::reflow-task (wrf/start! :layout (:ids %))))
(rx/buffer-time 100)
(rx/filter #(d/not-empty? %))
;; The updates of a page reflow together, in the undo group of
;; the step that asked for them: the one an update names, as
;; those with none come from the same step.
(rx/mapcat
(fn [updates]
(->> (group-by :page-id updates)
(map (fn [[page-id updates]]
(let [ids (into #{} (mapcat :ids) updates)]
(update-layout-positions {:page-id page-id
:ids ids
:reflow-tasks (mapv ::reflow-task updates)})))))))
(let [undo-group (some :undo-group updates)]
(->> (group-by :page-id updates)
(map (fn [[page-id updates]]
(let [ids (into #{} (mapcat :ids) updates)]
(update-layout-positions {:page-id page-id
:ids ids
:undo-group undo-group
:reflow-tasks (mapv ::reflow-task updates)}))))))))
(rx/take-until stopper)
;; On workspace teardown clear everything still pending.
(rx/finalize wrf/reset-pending!))))))

10
frontend/src/app/main/data/workspace/shapes.cljs
View File

@ -17,6 +17,7 @@
[app.common.types.container :as ctn]
[app.common.types.shape :as cts]
[app.common.types.shape-tree :as ctst]
[app.common.uuid :as uuid]
[app.main.data.changes :as dch]
[app.main.data.comments :as dc]
[app.main.data.event :as ev]
@ -263,8 +264,12 @@
{:keys [layout-ids text-ids]}
(wrfs/reflow-ids state page-id objects changed-objects ids props)
;; the reflow joins the undo group of this commit
changes
(add-undo-group changes state)]
(let [changes (add-undo-group changes state)]
(cond-> changes
(and layout-ids (nil? (:undo-group changes)))
(pcb/set-undo-group (uuid/next))))]
(rx/concat
;; Announces the texts still to be re-measured, so a reflow wait
@ -281,7 +286,8 @@
;; Update layouts for properties marked
(if layout-ids
(rx/of (ptk/data-event :layout/update {:ids layout-ids}))
(rx/of (ptk/data-event :layout/update {:ids layout-ids
:undo-group (:undo-group changes)}))
(rx/empty)))))))))
(defn add-shape

4
frontend/test/frontend_tests/composable_tests/golden_master.edn
View File

@ -30,7 +30,7 @@
{:transcript "one-of -> \n {:target :copy-root, :value 500}", :states ["df9a66ef7c8ee469" "c4f0c92f92575cda"]}
{:transcript "one-of -> \n one-of -> ", :states ["94318a628db305b6"]}]
:case-aj-reflow-after-nested-sync-is-its-own-undo-step
[{:transcript " {:component \"main\", :id sync-main-component}\n {:component \"main\", :level 0, :outer sync-main-outercomp-0}\n {:component \"main\", :level 1, :outer sync-main-outercomp-1}\n {:component \"main\", :copy sync-main-copyhead-0}\n {:target #fn, :property :fills, :value \"#0000ff\"}\n {:times 1}", :states ["43ca8b2de80a51fc" "43ca8b2de80a51fc" "9c3022331b3fe9ae"]}]
[{:transcript " {:component \"main\", :id sync-main-component}\n {:component \"main\", :level 0, :outer sync-main-outercomp-0}\n {:component \"main\", :level 1, :outer sync-main-outercomp-1}\n {:component \"main\", :copy sync-main-copyhead-0}\n {:target #fn, :property :fills, :value \"#0000ff\"}\n {:times 1}", :states ["43ca8b2de80a51fc" "32b45a5ac7536b08" "7104bfaccd42cd3b"]}]
:case-ak-update-main-pushes-grid-cells
[{:transcript " {:target :copy-child, :to :other, :keep-touched? false}\n {:board :copy-root, :child :copy-child, :row 1, :column 3}\n {:target :copy-root}", :states ["5a6af6c0b8f86ac3" "278e4ff6bf0df746" "322e03c88f4a9dce" "432cfef7e9b05164"]}
{:transcript "one-of -> \n {:board :copy-root, :child :copy-child, :row 1, :column 3}\n {:target :copy-root}", :states ["9900a7f71dc0cb5f" "de66ab1c3d4cc157" "6b5ff9fd86e2aa87"]}]
@ -40,7 +40,7 @@
:case-am-update-main-pushes-a-swap-inside-a-group
[{:transcript " {:target :copy-child, :to :other, :keep-touched? false}\n {:target :copy-root}", :states ["b4f808a12f723283" "9b695fc212f26b53" "32b94deb961ac763"]}]
:case-an-flex-child-override-follows-sync-reset-and-update-main
[{:transcript " {:target :copy-instance, :property :margin, :value 8}\n {:target :main-instance, :property :margin, :value 20}", :states ["234a21e54bf21bd5" "03664b1e6eb74856" "9c3b12abbc6089ce"]}
[{:transcript " {:target :copy-instance, :property :margin, :value 8}\n {:target :main-instance, :property :margin, :value 20}", :states ["234a21e54bf21bd5" "03664b1e6eb74856" "820db97e4e4b43dc"]}
{:transcript " {:target :copy-instance, :property :margin, :value 8}\n {:target :copy-root}", :states ["234a21e54bf21bd5" "1f92f9b4d2e50598" "a09954797e57930c"]}
{:transcript " {:target :copy-instance, :property :margin, :value 8}\n {:target :copy-root}", :states ["234a21e54bf21bd5" "ace1e76c1da2c443" "8f3452e82544f1ad"]}]
:case-ao-copy-dropped-into-a-grid-cell-reaches-copies

69
frontend/test/frontend_tests/data/workspace_frame_guides_undo_test.cljs Normal file
View File

@ -0,0 +1,69 @@
;; This Source Code Form is subject to the terms of the Mozilla Public
;; License, v. 2.0. If a copy of the MPL was not distributed with this
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
;;
;; Copyright (c) KALEIDOS SUBSIDIARY SL
(ns frontend-tests.data.workspace-frame-guides-undo-test
"Undo of a transform of a main board that holds a guide: the transform
moves the guide in a commit of its own before the shapes, and the copies'
sync must still be undone with the transform."
(:require
[app.common.files.changes-builder :as pcb]
[app.common.test-helpers.composable.comp.setups :as setup]
[app.common.test-helpers.files :as thf]
[app.common.test-helpers.ids-map :as thi]
[app.common.test-helpers.shapes :as ths]
[app.common.uuid :as uuid]
[app.main.data.changes :as dch]
[app.main.data.workspace.transforms :as dwt]
[app.main.data.workspace.undo :as dwu]
[cljs.test :as t :include-macros true]
[frontend-tests.composable-tests.interpreter :as ftm]))
(t/use-fixtures :each ftm/fixture)
(defn- add-guide
"The commit, out of the undo stack, of a guide on the board `label`."
[label]
(let [page (thf/current-page (ftm/current-file))
id (uuid/next)]
(dch/commit-changes
(-> (pcb/empty-changes nil (:id page))
(pcb/with-page page)
(pcb/set-guide id {:id id
:axis :x
:position 10
:frame-id (thi/id label)})
(pcb/set-save-undo? false)))))
(defn- rotation-of
[label]
(:rotation (ths/get-shape (ftm/current-file) label)))
(defn- guide-positions
[]
(mapv :position (vals (:guides (thf/current-page (ftm/current-file))))))
(t/deftest ^:async one-undo-reverts-a-rotation-of-a-main-with-a-guide
;; the guide does not move, and adds no commit of its own
(ftm/install! (setup/simple-component-with-labeled-copy))
(await (ftm/await-step [(add-guide :main-root)]))
(await (ftm/await-step [(dwt/increase-rotation [(thi/id :main-root)] 45)]))
(t/is (= 45 (rotation-of :main-root)))
(t/is (= 45 (rotation-of :copy-root)))
(t/is (= :settled (await (ftm/await-step [dwu/undo]))))
(t/is (= 0 (rotation-of :main-root)))
(t/is (= 0 (rotation-of :copy-root))))
(t/deftest ^:async one-undo-reverts-a-move-of-a-main-with-its-guide
(ftm/install! (setup/simple-component-with-labeled-copy))
(await (ftm/await-step [(add-guide :main-root)]))
(let [x (:x (ths/get-shape (ftm/current-file) :main-root))]
(await (ftm/await-step [(dwt/update-position (thi/id :main-root) {:x (+ x 50)})]))
(t/is (= [60] (guide-positions)))
(t/is (= :settled (await (ftm/await-step [dwu/undo]))))
(t/is (= x (:x (ths/get-shape (ftm/current-file) :main-root))))
(t/is (= [10] (guide-positions)))))

2
frontend/test/frontend_tests/runner.cljs
View File

@ -34,6 +34,7 @@
[frontend-tests.data.workspace-context-menu-test]
[frontend-tests.data.workspace-edit-watchers-test]
[frontend-tests.data.workspace-font-variants-test]
[frontend-tests.data.workspace-frame-guides-undo-test]
[frontend-tests.data.workspace-interactions-test]
[frontend-tests.data.workspace-libraries-test]
[frontend-tests.data.workspace-library-links-test]
@ -214,6 +215,7 @@
'frontend-tests.data.workspace-edit-watchers-test
'frontend-tests.data.workspace-interactions-test
'frontend-tests.data.workspace-font-variants-test
'frontend-tests.data.workspace-frame-guides-undo-test
'frontend-tests.data.workspace-libraries-test
'frontend-tests.data.workspace-library-links-test
'frontend-tests.data.workspace-library-updates-test