mirror of
https://github.com/penpot/penpot.git
synced 2026-05-30 04:08:08 +00:00
🐛 Fix update library dialog when a component position changes
Do not show the library sync popup when the only differences are global x/y changes on library components. We now generate the actual sync changes and only notify if there are real redo-changes to apply. Run cll/generate-sync-file-changes for candidate libraries and filter out those with empty :redo-changes. The expensive check is deferred via rx/timer 0 so it runs asynchronously and does not block the UI. Why: Position-only changes are normalized during sync (via reposition-shape) and never propagate to copies; showing the popup in that case was a false positive. Performance: The check is deferred to the next tick to avoid UI stutter on large files with many libraries.
This commit is contained in:
Pablo Alba
Pablo Alba
parent
56d8dc678c
commit
3cecc29276
@ -486,36 +486,41 @@
|
||||
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 synchronized."
|
||||
[changes file-id asset-type asset-id library-id libraries current-file-id]
|
||||
(assert (contains? #{:colors :components :typographies} asset-type))
|
||||
(assert (or (nil? asset-id) (uuid? asset-id)))
|
||||
(assert (uuid? file-id))
|
||||
(assert (uuid? library-id))
|
||||
be synchronized.
|
||||
|
||||
(container-log :info asset-id
|
||||
:msg "Sync file with library"
|
||||
:asset-type asset-type
|
||||
:asset-id asset-id
|
||||
:file (pretty-file file-id libraries current-file-id)
|
||||
:library (pretty-file library-id libraries current-file-id))
|
||||
If early-return? is true, stops as soon as the first change is generated."
|
||||
([changes file-id asset-type asset-id library-id libraries current-file-id]
|
||||
(generate-sync-file changes file-id asset-type asset-id library-id libraries current-file-id false))
|
||||
([changes file-id asset-type asset-id library-id libraries current-file-id early-return?]
|
||||
(assert (contains? #{:colors :components :typographies} asset-type))
|
||||
(assert (or (nil? asset-id) (uuid? asset-id)))
|
||||
(assert (uuid? file-id))
|
||||
(assert (uuid? library-id))
|
||||
|
||||
(let [file (get-in libraries [file-id :data])]
|
||||
(loop [containers (ctf/object-containers-seq file)
|
||||
changes changes]
|
||||
(if-let [container (first containers)]
|
||||
(do
|
||||
(recur (next containers)
|
||||
(pcb/concat-changes ;;TODO Remove concat changes
|
||||
changes
|
||||
(generate-sync-container (pcb/empty-changes nil)
|
||||
asset-type
|
||||
asset-id
|
||||
library-id
|
||||
container
|
||||
libraries
|
||||
current-file-id))))
|
||||
changes))))
|
||||
(container-log :info asset-id
|
||||
:msg "Sync file with library"
|
||||
:asset-type asset-type
|
||||
:asset-id asset-id
|
||||
:file (pretty-file file-id libraries current-file-id)
|
||||
:library (pretty-file library-id libraries current-file-id))
|
||||
|
||||
(let [file (get-in libraries [file-id :data])]
|
||||
(loop [containers (ctf/object-containers-seq file)
|
||||
changes changes]
|
||||
(let [container (first containers)]
|
||||
(if (or (nil? container)
|
||||
(and early-return? (seq (:redo-changes changes))))
|
||||
changes
|
||||
(recur (next containers)
|
||||
(pcb/concat-changes ;;TODO Remove concat changes
|
||||
changes
|
||||
(generate-sync-container (pcb/empty-changes nil)
|
||||
asset-type
|
||||
asset-id
|
||||
library-id
|
||||
container
|
||||
libraries
|
||||
current-file-id)))))))))
|
||||
|
||||
(defn generate-sync-library
|
||||
"Generate changes to synchronize all shapes in all components of the
|
||||
@ -523,35 +528,41 @@
|
||||
the given library.
|
||||
|
||||
If an asset id is given, only shapes linked to this particular asset will
|
||||
be synchronized."
|
||||
[changes file-id asset-type asset-id library-id libraries current-file-id]
|
||||
(assert (contains? #{:colors :components :typographies} asset-type))
|
||||
(assert (or (nil? asset-id) (uuid? asset-id)))
|
||||
(assert (uuid? file-id))
|
||||
(assert (uuid? library-id))
|
||||
be synchronized.
|
||||
|
||||
(container-log :info asset-id
|
||||
:msg "Sync local components with library"
|
||||
:asset-type asset-type
|
||||
:asset-id asset-id
|
||||
:file (pretty-file file-id libraries current-file-id)
|
||||
:library (pretty-file library-id libraries current-file-id))
|
||||
If early-return? is true, stops as soon as the first change is generated."
|
||||
([changes file-id asset-type asset-id library-id libraries current-file-id]
|
||||
(generate-sync-library changes file-id asset-type asset-id library-id libraries current-file-id false))
|
||||
([changes file-id asset-type asset-id library-id libraries current-file-id early-return?]
|
||||
(assert (contains? #{:colors :components :typographies} asset-type))
|
||||
(assert (or (nil? asset-id) (uuid? asset-id)))
|
||||
(assert (uuid? file-id))
|
||||
(assert (uuid? library-id))
|
||||
|
||||
(let [file (get-in libraries [file-id :data])]
|
||||
(loop [local-components (ctkl/components-seq file)
|
||||
changes changes]
|
||||
(if-let [local-component (first local-components)]
|
||||
(recur (next local-components)
|
||||
(pcb/concat-changes ;;TODO Remove concat changes
|
||||
changes
|
||||
(generate-sync-container (pcb/empty-changes nil)
|
||||
asset-type
|
||||
asset-id
|
||||
library-id
|
||||
(cfh/make-container local-component :component)
|
||||
libraries
|
||||
current-file-id)))
|
||||
changes))))
|
||||
(container-log :info asset-id
|
||||
:msg "Sync local components with library"
|
||||
:asset-type asset-type
|
||||
:asset-id asset-id
|
||||
:file (pretty-file file-id libraries current-file-id)
|
||||
:library (pretty-file library-id libraries current-file-id))
|
||||
|
||||
(let [file (get-in libraries [file-id :data])]
|
||||
(loop [local-components (ctkl/components-seq file)
|
||||
changes changes]
|
||||
(let [local-component (first local-components)]
|
||||
(if (or (nil? local-component)
|
||||
(and early-return? (seq (:redo-changes changes))))
|
||||
changes
|
||||
(recur (next local-components)
|
||||
(pcb/concat-changes ;;TODO Remove concat changes
|
||||
changes
|
||||
(generate-sync-container (pcb/empty-changes nil)
|
||||
asset-type
|
||||
asset-id
|
||||
library-id
|
||||
(cfh/make-container local-component :component)
|
||||
libraries
|
||||
current-file-id)))))))))
|
||||
|
||||
(defn- generate-sync-container
|
||||
"Generate changes to synchronize all shapes in a particular container (a page
|
||||
@ -2645,29 +2656,30 @@
|
||||
(generate-new-shape-for-swap shape file page libraries id-new-component index target-cell keep-props-values))]
|
||||
[new-shape all-parents changes]))
|
||||
|
||||
(defn generate-sync-file-changes
|
||||
[changes undo-group asset-type file-id asset-id library-id libraries current-file-id]
|
||||
(let [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))]
|
||||
(cond-> changes
|
||||
:always
|
||||
(pcb/set-undo-group undo-group)
|
||||
;; library-changes
|
||||
sync-components?
|
||||
(generate-sync-library file-id :components asset-id library-id libraries current-file-id)
|
||||
sync-colors?
|
||||
(generate-sync-library file-id :colors asset-id library-id libraries current-file-id)
|
||||
sync-typographies?
|
||||
(generate-sync-library file-id :typographies asset-id library-id libraries current-file-id)
|
||||
(defn- maybe-sync
|
||||
[c enabled? done? f]
|
||||
(if (and enabled? (not (done? c)))
|
||||
(f c)
|
||||
c))
|
||||
|
||||
(defn generate-sync-file-changes
|
||||
([changes undo-group asset-type file-id asset-id library-id libraries current-file-id]
|
||||
(generate-sync-file-changes changes undo-group asset-type file-id asset-id library-id libraries current-file-id false))
|
||||
([changes undo-group asset-type file-id asset-id library-id libraries current-file-id early-return?]
|
||||
(let [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))
|
||||
done? (fn [c] (and early-return? (seq (:redo-changes c))))]
|
||||
(-> (pcb/set-undo-group changes undo-group)
|
||||
;; library-changes
|
||||
(maybe-sync sync-components? done? #(generate-sync-library % file-id :components asset-id library-id libraries current-file-id early-return?))
|
||||
(maybe-sync sync-colors? done? #(generate-sync-library % file-id :colors asset-id library-id libraries current-file-id early-return?))
|
||||
(maybe-sync sync-typographies? done? #(generate-sync-library % file-id :typographies asset-id library-id libraries current-file-id early-return?))
|
||||
;; file-changes
|
||||
(maybe-sync sync-components? done? #(generate-sync-file % file-id :components asset-id library-id libraries current-file-id early-return?))
|
||||
(maybe-sync sync-colors? done? #(generate-sync-file % file-id :colors asset-id library-id libraries current-file-id early-return?))
|
||||
(maybe-sync sync-typographies? done? #(generate-sync-file % file-id :typographies asset-id library-id libraries current-file-id early-return?))))))
|
||||
|
||||
;; file-changes
|
||||
sync-components?
|
||||
(generate-sync-file file-id :components asset-id library-id libraries current-file-id)
|
||||
sync-colors?
|
||||
(generate-sync-file file-id :colors asset-id library-id libraries current-file-id)
|
||||
sync-typographies?
|
||||
(generate-sync-file file-id :typographies asset-id library-id libraries current-file-id))))
|
||||
|
||||
(defn generate-sync-head
|
||||
[changes file-full libraries container id reset?]
|
||||
|
||||
@ -8,6 +8,8 @@
|
||||
(:require
|
||||
[app.common.data :as d]
|
||||
[app.common.files.changes-builder :as pcb]
|
||||
[app.common.geom.point :as gpt]
|
||||
[app.common.geom.shapes :as gsh]
|
||||
[app.common.logic.libraries :as cll]
|
||||
[app.common.logic.shapes :as cls]
|
||||
[app.common.test-helpers.components :as thc]
|
||||
@ -488,3 +490,52 @@
|
||||
(t/is (= (:fill-opacity fill') 1))
|
||||
(t/is (= (:touched copy2-root') nil))
|
||||
(t/is (= (:touched copy2-child') nil))))
|
||||
|
||||
(t/deftest test-no-sync-changes-when-only-position-changes
|
||||
;; Regression: the library sync dialog was shown even when a library component
|
||||
;; was only moved (x/y changed). Position changes are normalised by
|
||||
;; reposition-shape during sync and never propagate to copies, so
|
||||
;; generate-sync-file-changes must return empty :redo-changes in this case.
|
||||
(let [;; ==== Setup
|
||||
;; Use integer width/height so that floating-point arithmetic of
|
||||
;; move(+delta) followed by reposition(-delta) cancels out exactly.
|
||||
file (-> (thf/sample-file :file1)
|
||||
(tho/add-simple-component-with-copy :component1
|
||||
:main-root
|
||||
:main-child
|
||||
:copy-root
|
||||
:main-root-params {:width 100 :height 100}
|
||||
:main-child-params {:width 50 :height 50}))
|
||||
page (thf/current-page file)
|
||||
main-root (ths/get-shape file :main-root)
|
||||
main-child (ths/get-shape file :main-child)
|
||||
|
||||
;; ==== Action
|
||||
;; Move the entire main component (root + child) by a non-zero integer delta.
|
||||
;; This is a position-only change: no fills, strokes or other
|
||||
;; attributes are modified.
|
||||
delta (gpt/point 100 150)
|
||||
changes1 (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
|
||||
#{(:id main-root) (:id main-child)}
|
||||
(fn [shape] (gsh/move shape delta))
|
||||
(:objects page)
|
||||
{})
|
||||
|
||||
updated-file (thf/apply-changes file changes1)
|
||||
|
||||
;; Run the full sync to check whether any real redo-changes are produced.
|
||||
;; The fixed frontend code filters out libraries whose sync produces no
|
||||
;; :redo-changes before showing the "library updated" notification.
|
||||
sync-changes (cll/generate-sync-file-changes (pcb/empty-changes)
|
||||
nil
|
||||
:components
|
||||
(:id updated-file)
|
||||
(thi/id :component1)
|
||||
(:id updated-file)
|
||||
{(:id updated-file) updated-file}
|
||||
(:id updated-file))]
|
||||
|
||||
;; ==== Check
|
||||
;; A position-only change in the main component must not propagate to copies
|
||||
;; and therefore must produce no redo-changes.
|
||||
(t/is (empty? (:redo-changes sync-changes)))))
|
||||
|
||||
@ -1281,38 +1281,62 @@
|
||||
file (dsh/lookup-file state file-id)
|
||||
file-data (get file :data)
|
||||
ignore-until (get file :ignore-sync-until)
|
||||
permissions (:permissions state)
|
||||
permissions (:permissions state)
|
||||
|
||||
libraries-need-sync
|
||||
(->> (vals (get state :files))
|
||||
(filter #(= (:library-of %) file-id))
|
||||
(filter #(seq (assets-need-sync % file-data ignore-until))))
|
||||
(filter #(seq (assets-need-sync % file-data ignore-until))))]
|
||||
|
||||
do-more-info
|
||||
#(modal/show! :libraries-dialog {:starting-tab "updates" :file-id file-id})
|
||||
(if-not (and (:can-edit permissions)
|
||||
(seq libraries-need-sync))
|
||||
;; Fast path: no libraries need sync based on timestamps
|
||||
(rx/empty)
|
||||
|
||||
do-update
|
||||
#(do (apply st/emit! (map (fn [library]
|
||||
(sync-file (:current-file-id state)
|
||||
(:id library)))
|
||||
libraries-need-sync))
|
||||
(st/emit! (ntf/hide)))
|
||||
;; Defer the expensive change generation check to avoid blocking the UI.
|
||||
;; For files with many libraries, this prevents stuttering/freezing.
|
||||
(->> (rx/timer 0)
|
||||
(rx/map (fn [_]
|
||||
;; This runs asynchronously on the next tick.
|
||||
;; Filter libraries to only those that would produce actual sync changes.
|
||||
(let [libraries (dsh/lookup-libraries state)]
|
||||
(filter (fn [library]
|
||||
(seq (:redo-changes
|
||||
(cll/generate-sync-file-changes
|
||||
(pcb/empty-changes)
|
||||
nil
|
||||
nil
|
||||
file-id
|
||||
nil
|
||||
(:id library)
|
||||
libraries
|
||||
file-id
|
||||
true))))
|
||||
libraries-need-sync))))
|
||||
(rx/filter seq)
|
||||
(rx/map (fn [libraries-with-changes]
|
||||
(let [do-more-info
|
||||
#(modal/show! :libraries-dialog {:starting-tab "updates" :file-id file-id})
|
||||
|
||||
do-dismiss
|
||||
#(st/emit! ignore-sync (ntf/hide))]
|
||||
do-update
|
||||
#(do (apply st/emit! (map (fn [library]
|
||||
(sync-file file-id (:id library)))
|
||||
libraries-with-changes))
|
||||
(st/emit! (ntf/hide)))
|
||||
|
||||
(when (and (:can-edit permissions)
|
||||
(seq libraries-need-sync))
|
||||
(rx/of (ntf/dialog
|
||||
:content (tr "workspace.updates.there-are-updates")
|
||||
:controls :inline-actions
|
||||
:links [{:label (tr "workspace.updates.more-info")
|
||||
:callback do-more-info}]
|
||||
:cancel {:label (tr "workspace.updates.dismiss")
|
||||
:callback do-dismiss}
|
||||
:accept {:label (tr "workspace.updates.update")
|
||||
:callback do-update}
|
||||
:tag :sync-dialog)))))))
|
||||
do-dismiss
|
||||
#(st/emit! ignore-sync (ntf/hide))]
|
||||
|
||||
(ntf/dialog
|
||||
:content (tr "workspace.updates.there-are-updates")
|
||||
:controls :inline-actions
|
||||
:links [{:label (tr "workspace.updates.more-info")
|
||||
:callback do-more-info}]
|
||||
:cancel {:label (tr "workspace.updates.dismiss")
|
||||
:callback do-dismiss}
|
||||
:accept {:label (tr "workspace.updates.update")
|
||||
:callback do-update}
|
||||
:tag :sync-dialog))))))))))
|
||||
|
||||
(defn touch-component
|
||||
"Update the modified-at attribute of the component to now"
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user