diff --git a/common/src/app/common/test_helpers/composable/comp/cases.cljc b/common/src/app/common/test_helpers/composable/comp/cases.cljc index 9001f4d6b5..8a289afc91 100644 --- a/common/src/app/common/test_helpers/composable/comp/cases.cljc +++ b/common/src/app/common/test_helpers/composable/comp/cases.cljc @@ -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) diff --git a/frontend/src/app/main/data/workspace/guides.cljs b/frontend/src/app/main/data/workspace/guides.cljs index 0fd638411e..57e20dfd8a 100644 --- a/frontend/src/app/main/data/workspace/guides.cljs +++ b/frontend/src/app/main/data/workspace/guides.cljs @@ -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] diff --git a/frontend/src/app/main/data/workspace/modifiers.cljs b/frontend/src/app/main/data/workspace/modifiers.cljs index 8c4aba9eca..a0b9ac564a 100644 --- a/frontend/src/app/main/data/workspace/modifiers.cljs +++ b/frontend/src/app/main/data/workspace/modifiers.cljs @@ -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)))))) diff --git a/frontend/src/app/main/data/workspace/shape_layout.cljs b/frontend/src/app/main/data/workspace/shape_layout.cljs index 82b3886c5a..16d9bc027b 100644 --- a/frontend/src/app/main/data/workspace/shape_layout.cljs +++ b/frontend/src/app/main/data/workspace/shape_layout.cljs @@ -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!)))))) diff --git a/frontend/src/app/main/data/workspace/shapes.cljs b/frontend/src/app/main/data/workspace/shapes.cljs index 347c997f6e..18702666df 100644 --- a/frontend/src/app/main/data/workspace/shapes.cljs +++ b/frontend/src/app/main/data/workspace/shapes.cljs @@ -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 diff --git a/frontend/test/frontend_tests/composable_tests/golden_master.edn b/frontend/test/frontend_tests/composable_tests/golden_master.edn index 7b4feb9103..26259fbe2d 100644 --- a/frontend/test/frontend_tests/composable_tests/golden_master.edn +++ b/frontend/test/frontend_tests/composable_tests/golden_master.edn @@ -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 diff --git a/frontend/test/frontend_tests/data/workspace_frame_guides_undo_test.cljs b/frontend/test/frontend_tests/data/workspace_frame_guides_undo_test.cljs new file mode 100644 index 0000000000..9bf68045ca --- /dev/null +++ b/frontend/test/frontend_tests/data/workspace_frame_guides_undo_test.cljs @@ -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))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 3c358fce29..6fcb10a101 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -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