diff --git a/common/src/app/common/files/changes_builder.cljc b/common/src/app/common/files/changes_builder.cljc index 6aa6ad9311..a00e5bd800 100644 --- a/common/src/app/common/files/changes_builder.cljc +++ b/common/src/app/common/files/changes_builder.cljc @@ -134,10 +134,13 @@ ;; redo-changes is a vector and :undo-changes is a list (defn concat-changes + "`changes1` followed by `changes2`: the redo changes of the second after + those of the first, its undo changes before. The undo list is built at + once, so joining many changes nests no lazy sequences." [changes1 changes2] (-> changes1 (update :redo-changes d/concat-vec (:redo-changes changes2)) - (update :undo-changes #(concat (:undo-changes changes2) %)))) + (update :undo-changes #(reduce conj (or % '()) (reverse (:undo-changes changes2)))))) ; TODO: remove this when not needed (defn- assert-page-id! diff --git a/common/src/app/common/logic/libraries.cljc b/common/src/app/common/logic/libraries.cljc index d1eb582a3b..d4ca830f5b 100644 --- a/common/src/app/common/logic/libraries.cljc +++ b/common/src/app/common/logic/libraries.cljc @@ -97,6 +97,7 @@ (declare sync-shape-recursive) (declare direct-sync) (declare inverse-sync) +(declare mark-local-changes) (declare compare-children) (declare add-shape-to-instance) @@ -611,18 +612,21 @@ (generate-rename-component (:component-id shape-inst) (:name shape-inst) library))] (if component - (sync-shape-recursive changes - inverse-sync - {:container container - :component component - :library library - :file file - :libraries libraries - :root-inst shape-inst - :root-main (ctf/get-component-root library component) - :initial-root? (:component-root shape-inst)} - shape-inst - (ctf/find-remote-shape container libraries shape-inst)) + (let [synced-ids (volatile! #{})] + (-> (sync-shape-recursive changes + inverse-sync + {:container container + :component component + :library library + :file file + :libraries libraries + :root-inst shape-inst + :root-main (ctf/get-component-root library component) + :initial-root? (:component-root shape-inst) + :synced-ids synced-ids} + shape-inst + (ctf/find-remote-shape container libraries shape-inst)) + (mark-local-changes @synced-ids))) changes))) (def ^:private inverse-sync @@ -718,17 +722,22 @@ false)) :after-children - (fn [changes {:keys [shape-inst]}] - ;; The inverse sync may be made on a component that is inside a - ;; remote library. We need to separate changes that are from - ;; local and remote files. - (let [check-local (fn [change] - (cond-> change - (= (:id change) (:id shape-inst)) - (assoc :local-change? true)))] - (-> changes - (update :redo-changes (partial mapv check-local)) - (update :undo-changes (partial map check-local)))))}) + (fn [changes {:keys [shape-inst synced-ids]}] + (vswap! synced-ids conj (:id shape-inst)) + changes)}) + +(defn- mark-local-changes + "Mark as `:local-change?` the changes of the copy shapes `ids`. The inverse + sync may update a component of a remote library, so the frontend commits + the copy's changes and the main's apart." + [changes ids] + (let [check-local (fn [change] + (cond-> change + (contains? ids (:id change)) + (assoc :local-change? true)))] + (-> changes + (update :redo-changes #(mapv check-local %)) + (update :undo-changes #(apply list (map check-local %)))))) ;; ---- Operation generation helpers ---- @@ -1284,15 +1293,16 @@ container {:type :reg-objects :shapes all-parents})) + ;; undone as the shape first, then its parents + (update :undo-changes conj (make-change + container + {:type :reg-objects + :shapes all-parents})) (update :undo-changes conj (make-change container {:type :mod-obj :id (:id dest-shape) :operations (vec uoperations)})) - (update :undo-changes concat [(make-change - container - {:type :reg-objects - :shapes all-parents})]) (pcb/apply-changes-local)))) diff --git a/common/test/common_tests/logic/large_sync_undo_test.cljc b/common/test/common_tests/logic/large_sync_undo_test.cljc new file mode 100644 index 0000000000..4dc6ad9226 --- /dev/null +++ b/common/test/common_tests/logic/large_sync_undo_test.cljc @@ -0,0 +1,70 @@ +;; 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 common-tests.logic.large-sync-undo-test + (:require + [app.common.files.changes-builder :as pcb] + [app.common.geom.point :as gpt] + [app.common.logic.libraries :as cll] + [app.common.logic.shapes :as cls] + [app.common.test-helpers.components :as thc] + [app.common.test-helpers.compositions :as tho] + [app.common.test-helpers.files :as thf] + [app.common.test-helpers.ids-map :as thi] + [app.common.test-helpers.shapes :as ths] + [app.common.types.container :as ctn] + [app.common.types.file :as ctf] + [app.common.types.pages-list :as ctpl] + [app.common.uuid :as uuid] + [clojure.test :as t])) + +(t/use-fixtures :each thi/test-fixture) + +(defn- add-copies + "`file` with `n` copies of the component `component-label` at the root of + its page, added in one step (the copies of a large design system)." + [file component-label n] + (let [page (thf/current-page file) + component (thc/get-component file component-label) + copies (map (fn [_] + (second (ctn/make-component-instance page component (:data file) + (gpt/point 100 100) {}))) + (range n))] + (ctf/update-file-data + file + (fn [data] + (ctpl/update-page data (:id page) + (fn [page] + (-> page + (update :objects into (comp cat (map (juxt :id identity))) copies) + (update-in [:objects uuid/zero :shapes] into (map (comp :id first) copies))))))))) + +(defn- read-count + "How many changes `changes` holds, or `:stack-overflow` when reading them + overflows the stack." + [changes] + (try + (count changes) + (catch #?(:clj StackOverflowError :cljs js/RangeError) _ + :stack-overflow))) + +(t/deftest undo-of-a-sync-into-many-copies-can-be-read + ;; A large sync builds a long undo list; reading it must not recurse once + ;; per synced shape (F84). + (let [file (-> (thf/sample-file :file1) + (tho/add-simple-component :component1 :main-root :main-child) + (add-copies :component1 5000)) + page (thf/current-page file) + edit (cls/generate-update-shapes (pcb/empty-changes nil (:id page)) + #{(thi/id :main-child)} + #(assoc % :fills (ths/sample-fills-color :fill-color "#ff0000")) + (:objects page) + {}) + file (thf/apply-changes file edit :validate? false) + sync (cll/generate-sync-file-changes (pcb/empty-changes) nil :components (:id file) + (thi/id :component1) (:id file) {(:id file) file} (:id file))] + (t/is (= 10000 (count (:redo-changes sync)))) + (t/is (= 10000 (read-count (:undo-changes sync)))))) diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 23ebca27cf..9aa58a15d7 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -70,6 +70,7 @@ [common-tests.logic.composable-sync-test] [common-tests.logic.copying-and-duplicating-test] [common-tests.logic.duplicated-pages-test] + [common-tests.logic.large-sync-undo-test] [common-tests.logic.library-asset-sync-test] [common-tests.logic.move-shapes-test] [common-tests.logic.multiple-nesting-levels-test] @@ -190,6 +191,7 @@ 'common-tests.logic.composable-sync-test 'common-tests.logic.copying-and-duplicating-test 'common-tests.logic.duplicated-pages-test + 'common-tests.logic.large-sync-undo-test 'common-tests.logic.library-asset-sync-test 'common-tests.logic.move-shapes-test 'common-tests.logic.multiple-nesting-levels-test