From dcf18b3aeeae55e932cce09456ca85317109401e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Moya?= Date: Thu, 7 Jul 2022 16:07:34 +0200 Subject: [PATCH] :wrench: Refactor sync-file for performance --- .../src/app/common/pages/changes_builder.cljc | 4 +- common/src/app/common/types/color.cljc | 4 +- common/src/app/common/types/component.cljc | 10 +- common/src/app/common/types/file.cljc | 6 +- common/src/app/common/types/typography.cljc | 4 +- common/test/app/common/types/file_test.cljc | 2 +- .../app/main/data/workspace/libraries.cljs | 136 ++++++++++-------- .../data/workspace/libraries_helpers.cljs | 50 +++++-- .../app/main/ui/workspace/sidebar/assets.cljs | 6 +- frontend/test/app/components_basic_test.cljs | 2 +- frontend/test/app/test_helpers/libraries.cljs | 1 + 11 files changed, 136 insertions(+), 89 deletions(-) diff --git a/common/src/app/common/pages/changes_builder.cljc b/common/src/app/common/pages/changes_builder.cljc index 50891acc1..03b8e1ca4 100644 --- a/common/src/app/common/pages/changes_builder.cljc +++ b/common/src/app/common/pages/changes_builder.cljc @@ -112,7 +112,9 @@ redo-changes (:redo-changes changes) new-changes (if (< index (count redo-changes)) (->> (subvec (:redo-changes changes) index) - (map #(assoc % :page-id uuid/zero))) + (map #(-> % + (assoc :page-id uuid/zero) + (dissoc :component-id)))) []) new-file-data (cp/process-changes file-data new-changes)] (vary-meta changes assoc ::file-data new-file-data diff --git a/common/src/app/common/types/color.cljc b/common/src/app/common/types/color.cljc index ea9ad35d6..5f462796c 100644 --- a/common/src/app/common/types/color.cljc +++ b/common/src/app/common/types/color.cljc @@ -238,9 +238,9 @@ (defn uses-library-color? "Check if the shape uses the given library color." - [shape library-id color] + [shape library-id color-id] (let [all-colors (get-all-colors shape)] - (some #(and (= (:ref-id %) (:id color)) + (some #(and (= (:ref-id %) color-id) (= (:ref-file %) library-id)) all-colors))) diff --git a/common/src/app/common/types/component.cljc b/common/src/app/common/types/component.cljc index 85a15fed3..279b8e247 100644 --- a/common/src/app/common/types/component.cljc +++ b/common/src/app/common/types/component.cljc @@ -7,10 +7,10 @@ (ns app.common.types.component) (defn instance-of? - [shape file-id component] + [shape file-id component-id] (and (some? (:component-id shape)) (some? (:component-file shape)) - (= (:component-id shape) (:id component)) + (= (:component-id shape) component-id) (= (:component-file shape) file-id))) (defn is-main-of? @@ -28,3 +28,9 @@ [component] (get-in component [:objects (:id component)])) +(defn uses-library-components? + "Check if the shape uses any component in the given library." + [shape library-id] + (and (some? (:component-id shape)) + (= (:component-file shape) library-id))) + diff --git a/common/src/app/common/types/file.cljc b/common/src/app/common/types/file.cljc index 43c4a58cb..c6015374a 100644 --- a/common/src/app/common/types/file.cljc +++ b/common/src/app/common/types/file.cljc @@ -123,15 +123,15 @@ (defmethod uses-asset? :component [_ shape library-id component] - (ctk/instance-of? shape library-id component)) + (ctk/instance-of? shape library-id (:id component))) (defmethod uses-asset? :color [_ shape library-id color] - (ctc/uses-library-color? shape library-id color)) + (ctc/uses-library-color? shape library-id (:id color))) (defmethod uses-asset? :typography [_ shape library-id typography] - (cty/uses-library-typography? shape library-id typography)) + (cty/uses-library-typography? shape library-id (:id typography))) (defn find-asset-type-usages "Find all usages of an asset in a file (may be in pages or in the components diff --git a/common/src/app/common/types/typography.cljc b/common/src/app/common/types/typography.cljc index 036b8fe34..0c1a15ef7 100644 --- a/common/src/app/common/types/typography.cljc +++ b/common/src/app/common/types/typography.cljc @@ -49,13 +49,13 @@ (defn uses-library-typography? "Check if the shape uses the given library typography." - [shape library-id typography] + [shape library-id typography-id] (and (= (:type shape) :text) (->> shape :content ;; Check if any node in the content has a reference for the library (txt/node-seq - #(and (= (:typography-ref-id %) (:id typography)) + #(and (= (:typography-ref-id %) typography-id) (= (:typography-ref-file %) library-id)))))) (defn remap-typographies diff --git a/common/test/app/common/types/file_test.cljc b/common/test/app/common/types/file_test.cljc index bb297f8bf..5c5f8527f 100644 --- a/common/test/app/common/types/file_test.cljc +++ b/common/test/app/common/types/file_test.cljc @@ -109,7 +109,7 @@ (t/is (= (count components) 1)) (t/is (= (:name p-group) "Group1")) - (t/is (ctk/instance-of? p-group file-id component1)) + (t/is (ctk/instance-of? p-group file-id (:id component1))) (t/is (not (ctk/is-main-instance? (:id p-group) file-page-id component1))) (t/is (ctk/is-main-of? c-group1 p-group)) diff --git a/frontend/src/app/main/data/workspace/libraries.cljs b/frontend/src/app/main/data/workspace/libraries.cljs index 2217af1dd..6be291020 100644 --- a/frontend/src/app/main/data/workspace/libraries.cljs +++ b/frontend/src/app/main/data/workspace/libraries.cljs @@ -138,7 +138,7 @@ (pcb/update-color color))] (rx/of (dwu/start-undo-transaction) (dch/commit-changes changes) - (sync-file (:current-file-id state) file-id) + (sync-file (:current-file-id state) file-id :colors (:id color)) (dwu/commit-undo-transaction)))) (defn update-color @@ -241,7 +241,7 @@ (pcb/update-typography typography))] (rx/of (dwu/start-undo-transaction) (dch/commit-changes changes) - (sync-file (:current-file-id state) file-id) + (sync-file (:current-file-id state) file-id :typographies (:id typography)) (dwu/commit-undo-transaction)))) (defn update-typography @@ -577,13 +577,15 @@ (ptk/reify ::update-component-sync ptk/WatchEvent (watch [_ state _] - (let [current-file-id (:current-file-id state)] + (let [current-file-id (:current-file-id state) + page (wsh/lookup-page state) + shape (ctn/get-shape page shape-id)] (rx/of (dwu/start-undo-transaction) (update-component shape-id) - (sync-file current-file-id file-id) + (sync-file current-file-id file-id :components (:component-id shape)) (when (not= current-file-id file-id) - (sync-file file-id file-id)) + (sync-file file-id file-id :components (:component-id shape))) (dwu/commit-undo-transaction)))))) (defn update-component-in-bulk @@ -603,62 +605,77 @@ shapes in all pages in the file that use some color, typography or component of the library, and copy the new values to the shapes. Do it also for shapes inside components of the local file library." - [file-id library-id] - (us/assert ::us/uuid file-id) - (us/assert ::us/uuid library-id) - (ptk/reify ::sync-file - ptk/UpdateEvent - (update [_ state] - (if (not= library-id (:current-file-id state)) - (d/assoc-in-when state [:workspace-libraries library-id :synced-at] (dt/now)) - state)) + ([file-id library-id] + (sync-file file-id library-id nil nil)) + ([file-id library-id asset-type asset-id] + (us/assert ::us/uuid file-id) + (us/assert ::us/uuid library-id) + (us/assert (s/nilable #{:colors :components :typographies}) asset-type) + (us/assert (s/nilable ::us/uuid) asset-id) + (ptk/reify ::sync-file + ptk/UpdateEvent + (update [_ state] + (if (not= library-id (:current-file-id state)) + (d/assoc-in-when state [:workspace-libraries library-id :synced-at] (dt/now)) + state)) - ptk/WatchEvent - (watch [it state _] - (when (and (some? file-id) (some? library-id)) ; Prevent race conditions while navigating out of the file - (log/info :msg "SYNC-FILE" - :file (dwlh/pretty-file file-id state) - :library (dwlh/pretty-file library-id state)) - (let [file (wsh/get-file state file-id) + ptk/WatchEvent + (watch [it state _] + (when (and (some? file-id) (some? library-id)) ; Prevent race conditions while navigating out of the file + (log/info :msg "SYNC-FILE" + :file (dwlh/pretty-file file-id state) + :library (dwlh/pretty-file library-id state)) + (let [file (wsh/get-file state file-id) - library-changes (reduce - pcb/concat-changes - (pcb/empty-changes it) - [(dwlh/generate-sync-library it file-id :components library-id state) - (dwlh/generate-sync-library it file-id :colors library-id state) - (dwlh/generate-sync-library it file-id :typographies library-id state)]) - file-changes (reduce - pcb/concat-changes - (pcb/empty-changes it) - [(dwlh/generate-sync-file it file-id :components library-id state) - (dwlh/generate-sync-file it file-id :colors library-id state) - (dwlh/generate-sync-file it file-id :typographies library-id state)]) + sync-components? (or (nil? asset-type) (= asset-type :components)) + sync-colors? (or (nil? asset-type) (= asset-type :colors)) + sync-typographies? (or (nil? asset-type) (= asset-type :typographies)) - changes (pcb/concat-changes library-changes file-changes)] + library-changes (reduce + pcb/concat-changes + (pcb/empty-changes it) + [(when sync-components? + (dwlh/generate-sync-library it file-id :components asset-id library-id state)) + (when sync-colors? + (dwlh/generate-sync-library it file-id :colors asset-id library-id state)) + (when sync-typographies? + (dwlh/generate-sync-library it file-id :typographies asset-id library-id state))]) + file-changes (reduce + pcb/concat-changes + (pcb/empty-changes it) + [(when sync-components? + (dwlh/generate-sync-file it file-id :components asset-id library-id state)) + (when sync-colors? + (dwlh/generate-sync-file it file-id :colors asset-id library-id state)) + (when sync-typographies? + (dwlh/generate-sync-file it file-id :typographies asset-id library-id state))]) - (log/debug :msg "SYNC-FILE finished" :js/rchanges (log-changes - (:redo-changes changes) - file)) - (rx/concat - (rx/of (dm/hide-tag :sync-dialog)) - (when (seq (:redo-changes changes)) - (rx/of (dch/commit-changes (assoc changes ;; TODO a ver qué pasa con esto - :file-id file-id)))) - (when (not= file-id library-id) - ;; When we have just updated the library file, give some time for the - ;; update to finish, before marking this file as synced. - ;; TODO: look for a more precise way of syncing this. - ;; Maybe by using the stream (second argument passed to watch) - ;; to wait for the corresponding changes-committed and then proceed - ;; with the :update-sync mutation. - (rx/concat (rx/timer 3000) - (rp/mutation :update-sync - {:file-id file-id - :library-id library-id}))) - (when (seq (:redo-changes library-changes)) - (rx/of (sync-file-2nd-stage file-id library-id))))))))) + changes (pcb/concat-changes library-changes file-changes)] -(defn sync-file-2nd-stage + (log/debug :msg "SYNC-FILE finished" :js/rchanges (log-changes + (:redo-changes changes) + file)) + (rx/concat + (rx/of (dm/hide-tag :sync-dialog)) + (when (seq (:redo-changes changes)) + (rx/of (dch/commit-changes (assoc changes ;; TODO a ver qué pasa con esto + :file-id file-id)))) + (when (not= file-id library-id) + ;; When we have just updated the library file, give some time for the + ;; update to finish, before marking this file as synced. + ;; TODO: look for a more precise way of syncing this. + ;; Maybe by using the stream (second argument passed to watch) + ;; to wait for the corresponding changes-committed and then proceed + ;; with the :update-sync mutation. + (rx/concat (rx/timer 3000) + (rp/mutation :update-sync + {:file-id file-id + :library-id library-id}))) + (when (and (seq (:redo-changes library-changes)) + sync-components?) + (rx/of (sync-file-2nd-stage file-id library-id)))))))))) + +(defn- sync-file-2nd-stage "If some components have been modified, we need to launch another synchronization to update the instances of the changed components." ;; TODO: this does not work if there are multiple nested components. Only the @@ -667,9 +684,10 @@ ;; recursively. But for this not to cause an infinite loop, we need to ;; implement updated-at at component level, to detect what components have ;; not changed, and then not to apply sync and terminate the loop. - [file-id library-id] + [file-id library-id asset-id] (us/assert ::us/uuid file-id) (us/assert ::us/uuid library-id) + (us/assert (s/nilable ::us/uuid) asset-id) (ptk/reify ::sync-file-2nd-stage ptk/WatchEvent (watch [it state _] @@ -680,8 +698,8 @@ changes (reduce pcb/concat-changes (pcb/empty-changes it) - [(dwlh/generate-sync-file it file-id :components library-id state) - (dwlh/generate-sync-library it file-id :components library-id state)])] + [(dwlh/generate-sync-file it file-id :components asset-id library-id state) + (dwlh/generate-sync-library it file-id :components asset-id library-id state)])] (when (seq (:redo-changes changes)) (log/debug :msg "SYNC-FILE (2nd stage) finished" :js/rchanges (log-changes (:redo-changes changes) diff --git a/frontend/src/app/main/data/workspace/libraries_helpers.cljs b/frontend/src/app/main/data/workspace/libraries_helpers.cljs index 1bb134525..4103e8137 100644 --- a/frontend/src/app/main/data/workspace/libraries_helpers.cljs +++ b/frontend/src/app/main/data/workspace/libraries_helpers.cljs @@ -154,14 +154,19 @@ (defn generate-sync-file "Generate changes to synchronize all shapes in all pages of the given file, - that use assets of the given type in the given library." - [it file-id asset-type library-id state] + that use assets of the given type in the given library. + + If an asset id is given, only shapes linked to this particular asset will + be syncrhonized." + [it file-id asset-type asset-id library-id state] (s/assert #{:colors :components :typographies} asset-type) + (s/assert (s/nilable ::us/uuid) asset-id) (s/assert ::us/uuid file-id) (s/assert ::us/uuid library-id) (log/info :msg "Sync file with library" :asset-type asset-type + :asset-id asset-id :file (pretty-file file-id state) :library (pretty-file library-id state)) @@ -174,6 +179,7 @@ changes (generate-sync-container it asset-type + asset-id library-id state (cph/make-container page :page)))) @@ -182,11 +188,19 @@ (defn generate-sync-library "Generate changes to synchronize all shapes in all components of the local library of the given file, that use assets of the given type in - the given library." - [it file-id asset-type library-id state] + the given library. + + If an asset id is given, only shapes linked to this particular asset will + be syncrhonized." + [it file-id asset-type asset-id library-id state] + (s/assert #{:colors :components :typographies} asset-type) + (s/assert (s/nilable ::us/uuid) asset-id) + (s/assert ::us/uuid file-id) + (s/assert ::us/uuid library-id) (log/info :msg "Sync local components with library" :asset-type asset-type + :asset-id asset-id :file (pretty-file file-id state) :library (pretty-file library-id state)) @@ -199,6 +213,7 @@ changes (generate-sync-container it asset-type + asset-id library-id state (cph/make-container local-component :component)))) @@ -207,14 +222,14 @@ (defn- generate-sync-container "Generate changes to synchronize all shapes in a particular container (a page or a component) that use assets of the given type in the given library." - [it asset-type library-id state container] + [it asset-type asset-id library-id state container] (if (cph/page? container) (log/debug :msg "Sync page in local file" :page-id (:id container)) (log/debug :msg "Sync component in local library" :component-id (:id container))) - (let [linked-shapes (->> (vals (:objects container)) - (filter #(uses-assets? asset-type % library-id (cph/page? container))))] + (let [linked-shapes (->> (vals (:objects container)) + (filter #(uses-assets? asset-type asset-id % library-id (cph/page? container))))] (loop [shapes (seq linked-shapes) changes (-> (pcb/empty-changes it) (pcb/with-container container) @@ -231,21 +246,26 @@ (defmulti uses-assets? "Checks if a shape uses some asset of the given type in the given library." - (fn [asset-type _ _ _] asset-type)) + (fn [asset-type _ _ _ _] asset-type)) (defmethod uses-assets? :components - [_ shape library-id page?] - (and (some? (:component-id shape)) - (= (:component-file shape) library-id) + [_ component-id shape library-id page?] + (and (if (nil? component-id) + (ctk/uses-library-components? shape library-id) + (ctk/instance-of? shape library-id component-id)) (or (:component-root? shape) (not page?)))) ; avoid nested components inside pages (defmethod uses-assets? :colors - [_ shape library-id _] - (ctc/uses-library-colors? shape library-id)) + [_ color-id shape library-id _] + (if (nil? color-id) + (ctc/uses-library-colors? shape library-id) + (ctc/uses-library-color? shape library-id color-id))) (defmethod uses-assets? :typographies - [_ shape library-id _] - (cty/uses-library-typographies? shape library-id)) + [_ typography-id shape library-id _] + (if (nil? typography-id) + (cty/uses-library-typographies? shape library-id) + (cty/uses-library-typography? shape library-id typography-id))) (defmulti generate-sync-shape "Generate changes to synchronize one shape from all assets of the given type diff --git a/frontend/src/app/main/ui/workspace/sidebar/assets.cljs b/frontend/src/app/main/ui/workspace/sidebar/assets.cljs index 8e408c060..fc84f8950 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/assets.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/assets.cljs @@ -568,7 +568,7 @@ (on-assets-delete) (st/emit! (dwu/start-undo-transaction) (dwl/delete-component {:id (:component-id @state)}) - (dwl/sync-file file-id file-id) + (dwl/sync-file file-id file-id :components (:component-id @state)) (dwu/commit-undo-transaction))))) on-rename @@ -1120,7 +1120,7 @@ (on-assets-delete) (st/emit! (dwu/start-undo-transaction) (dwl/delete-color color) - (dwl/sync-file file-id file-id) + (dwl/sync-file file-id file-id :color (:id color)) (dwu/commit-undo-transaction))))) rename-color-clicked @@ -1762,7 +1762,7 @@ (on-assets-delete) (st/emit! (dwu/start-undo-transaction) (dwl/delete-typography (:id @state)) - (dwl/sync-file file-id file-id) + (dwl/sync-file file-id file-id :typographies (:id @state)) (dwu/commit-undo-transaction))))) editing-id (or (:rename-typography local-data) diff --git a/frontend/test/app/components_basic_test.cljs b/frontend/test/app/components_basic_test.cljs index 05474e551..52958aee5 100644 --- a/frontend/test/app/components_basic_test.cljs +++ b/frontend/test/app/components_basic_test.cljs @@ -341,7 +341,7 @@ (ptk/emit! store (dwl/delete-component {:id component-id}) - (dwl/sync-file (:id file) (:id file)) + (dwl/sync-file (:id file) (:id file) :components component-id) :the/end)))) (t/deftest test-instantiate-component diff --git a/frontend/test/app/test_helpers/libraries.cljs b/frontend/test/app/test_helpers/libraries.cljs index 26cc294a6..b2754977b 100644 --- a/frontend/test/app/test_helpers/libraries.cljs +++ b/frontend/test/app/test_helpers/libraries.cljs @@ -5,6 +5,7 @@ [app.common.pages.helpers :as cph] [app.common.types.component :as ctk] [app.common.types.container :as ctn] + [app.main.data.workspace.state-helpers :as wsh] [app.test-helpers.pages :as thp])) ;; ---- Helpers to manage libraries and synchronization