From 80d688be9375fc51a6b3d732f235f91cc8ca068b Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Thu, 9 Jul 2026 11:37:45 +0200 Subject: [PATCH] :bug: Fix problems with comments clusters (#10543) --- frontend/src/app/main/data/comments.cljs | 20 +- .../src/app/main/data/workspace/comments.cljs | 72 +++- frontend/src/app/main/ui/comments.cljs | 319 +++++++++++------- frontend/src/app/main/ui/comments.scss | 84 ++++- frontend/src/app/main/ui/viewer/comments.cljs | 68 +++- .../main/ui/workspace/viewport/comments.cljs | 43 ++- frontend/test/frontend_tests/runner.cljs | 2 + .../ui/comments_clustering_test.cljs | 144 ++++++++ 8 files changed, 578 insertions(+), 174 deletions(-) create mode 100644 frontend/test/frontend_tests/ui/comments_clustering_test.cljs diff --git a/frontend/src/app/main/data/comments.cljs b/frontend/src/app/main/data/comments.cljs index c0c0dc7409..a69d759272 100644 --- a/frontend/src/app/main/data/comments.cljs +++ b/frontend/src/app/main/data/comments.cljs @@ -503,7 +503,7 @@ (-> state (update :comments-local assoc :open id) (update :comments-local assoc :options nil) - (update :comments-local dissoc :draft))))) + (update :comments-local dissoc :draft :expanded))))) (defn close-thread [] @@ -511,8 +511,26 @@ ptk/UpdateEvent (update [_ state] (-> state + (update :comments-local dissoc :open :draft :options :expanded))))) + +(defn expand-comment-group + "Temporarily mark a proximity cluster of threads as expanded so its bubbles + can be laid out visually without altering their stored positions." + [thread-ids] + (ptk/reify ::expand-comment-group + ptk/UpdateEvent + (update [_ state] + (-> state + (update :comments-local assoc :expanded (set thread-ids)) (update :comments-local dissoc :open :draft :options))))) +(defn collapse-comment-group + [] + (ptk/reify ::collapse-comment-group + ptk/UpdateEvent + (update [_ state] + (update state :comments-local dissoc :expanded)))) + (defn update-filters [{:keys [mode show list] :as params}] (ptk/reify ::update-filters diff --git a/frontend/src/app/main/data/workspace/comments.cljs b/frontend/src/app/main/data/workspace/comments.cljs index 968cc8194e..16251100bf 100644 --- a/frontend/src/app/main/data/workspace/comments.cljs +++ b/frontend/src/app/main/data/workspace/comments.cljs @@ -25,6 +25,7 @@ [app.main.data.workspace.edition :as dwe] [app.main.data.workspace.layout :as dwlo] [app.main.data.workspace.selection :as dws] + [app.main.data.workspace.viewport-wasm :as dwvw] [app.main.data.workspace.zoom :as dwz] [app.main.repo :as rp] [app.main.router :as rt] @@ -79,15 +80,16 @@ (let [local (:comments-local state) comments-mode? (= :comments (get-in state [:workspace-drawing :tool]))] (cond - (:draft local) (rx/of (dcmt/close-thread)) - (:open local) (rx/of (dcmt/close-thread)) + (:draft local) (rx/of (dcmt/close-thread)) + (:open local) (rx/of (dcmt/close-thread)) + (:expanded local) (rx/of (dcmt/collapse-comment-group)) ;; Only clear edition / deselect on interrupt while the comments ;; tool is active. When comments are merely visible during design, ;; `select-shape` emits `:interrupt` and this would otherwise wipe ;; the freshly selected shape, breaking click selection. - comments-mode? (rx/of (dwe/clear-edition-mode) - (dws/deselect-all true)) - :else (rx/empty)))))) + comments-mode? (rx/of (dwe/clear-edition-mode) + (dws/deselect-all true)) + :else (rx/empty)))))) ;; Event responsible of the what should be executed when user clicked ;; on the comments layer. An option can be create a new draft thread, @@ -98,17 +100,28 @@ (ptk/reify ::handle-comment-layer-click ptk/WatchEvent (watch [_ state _] - (if (not= :comments (get-in state [:workspace-drawing :tool])) - (rx/empty) - (let [local (:comments-local state)] - (if (some? (:open local)) - (rx/of (dcmt/close-thread)) - (let [page-id (:current-page-id state) - file-id (:current-file-id state) - params {:position position - :page-id page-id - :file-id file-id}] - (rx/of (dcmt/create-draft params))))))))) + (let [local (:comments-local state) + comments-mode? (= :comments (get-in state [:workspace-drawing :tool]))] + (cond + ;; A click anywhere collapses a temporarily separated cluster, + ;; regardless of the active tool. Opening a thread clears :expanded, + ;; so this never fires while a comment is open. + (some? (:expanded local)) + (rx/of (dcmt/collapse-comment-group)) + + (not comments-mode?) + (rx/empty) + + (some? (:open local)) + (rx/of (dcmt/close-thread)) + + :else + (let [page-id (:current-page-id state) + file-id (:current-file-id state) + params {:position position + :page-id page-id + :file-id file-id}] + (rx/of (dcmt/create-draft params)))))))) (defn center-to-comment-thread [{:keys [position] :as thread}] @@ -127,7 +140,11 @@ nh (- (/ (:height vbox) 2) ph) nx (- (:x position) nw) ny (- (:y position) nh)] - (update local :vbox assoc :x nx :y ny))))))) + (update local :vbox assoc :x nx :y ny))))) + + ptk/EffectEvent + (effect [_ state _] + (dwvw/maybe-sync-workspace-local-viewport! state)))) (defn- set-comment-thread "Stores the comment thread in the workspace state so its bubble re-renders." @@ -305,6 +322,25 @@ distance-overlap 32] (< distance-zoom distance-overlap))) +(defn group-bubbles + "Group bubbles into vectors by proximity: each group holds threads whose + bubbles overlap at the given `zoom`." + [zoom circles] + (letfn [(overlaps-group? [current group] + (some #(overlap-bubbles? zoom current %) group)) + + (find-overlapping-group [groups current] + (some #(when (overlaps-group? current %) %) groups)) + + (add-to-group [groups target current] + (map #(if (= % target) (cons current %) %) groups)) + + (assign [groups current] + (if-let [group (find-overlapping-group groups current)] + (add-to-group groups group current) + (cons [current] groups)))] + (reduce assign [] circles))) + (defn- calculate-zoom-scale-to-ungroup-current-bubble "Calculate the minimum zoom scale needed to keep the current bubble ungrouped from the rest" [zoom thread threads] @@ -373,7 +409,7 @@ (rx/empty)) (->> (rx/of (dwd/select-for-drawing :comments) - (set-zoom-to-separate-grouped-bubbles thread) + ;; Center on the comment (no zoom) and open its thread. (center-to-comment-thread thread) (with-meta (dcmt/open-thread thread) {::ev/origin "workspace"})) (rx/observe-on :async)))))) diff --git a/frontend/src/app/main/ui/comments.cljs b/frontend/src/app/main/ui/comments.cljs index c3bd8044fa..bf49f7acc0 100644 --- a/frontend/src/app/main/ui/comments.cljs +++ b/frontend/src/app/main/ui/comments.cljs @@ -18,8 +18,6 @@ [app.main.data.comments :as dcm] [app.main.data.modal :as modal] [app.main.data.workspace.comments :as dwcm] - [app.main.data.workspace.viewport :as dwv] - [app.main.data.workspace.zoom :as dwz] [app.main.refs :as refs] [app.main.store :as st] [app.main.ui.components.dropdown :refer [dropdown]] @@ -704,15 +702,21 @@ (mf/defc comment-reply-form* {::mf/private true} - [{:keys [on-submit]}] + [{:keys [on-submit on-cancel]}] (let [content (mf/use-state "") disabled? (or (blank-content? @content) (exceeds-length? @content)) + ;; Contexts without a global interrupt handler (e.g. the viewer) pass an + ;; explicit cancel; otherwise fall back to the interrupt cycle. on-cancel (mf/use-fn - #(st/emit! :interrupt)) + (mf/deps on-cancel) + (fn [] + (if (fn? on-cancel) + (on-cancel) + (st/emit! :interrupt)))) on-change (mf/use-fn @@ -799,8 +803,10 @@ (some? position-modifier) (gpt/transform position-modifier)) content (:content draft) - bubble-margin (gpt/point 0 0) + ;; Keep the draft bubble centered on the comment position (matching a + ;; created bubble) while the input box is offset to the side. + bubble-margin (gpt/point 24 24) pos (offset-position position viewport zoom bubble-margin) margin-x (* (:x bubble-margin) (if (= (:h-dir pos) :left) -1 1)) @@ -808,6 +814,9 @@ pos-x (+ (* (:x pos) zoom) margin-x) pos-y (- (* (:y pos) zoom) margin-y) + bubble-x (floor (* (:x position) zoom)) + bubble-y (floor (* (:y position) zoom)) + disabled? (or (blank-content? content) (exceeds-length? content)) @@ -833,39 +842,36 @@ (on-submit draft)))] [:> (mf/provider mentions-context) {:value mentions-s} - [:div {:class (stl/css-case :floating-thread-draft-wrapper true + [:div {:class (stl/css :floating-preview-wrapper :floating-preview-bubble) + :data-testid "floating-thread-bubble" + :style {:top (dm/str bubble-y "px") + :left (dm/str bubble-x "px")} + :on-click dom/stop-propagation} + [:> comment-avatar* {:class (stl/css :avatar-lg) + :image (cfg/resolve-profile-photo-url profile)}]] + + [:div {:class (stl/css-case :floating-thread-draft-inner-wrapper true + :cursor-auto true :left (= (:h-dir pos) :left) :top (= (:v-dir pos) :top)) :style {:top (str pos-y "px") - :left (str pos-x "px")}} - [:div - {:data-testid "floating-thread-bubble" - :style {:top (str pos-y "px") - :left (str pos-x "px")} - :on-click dom/stop-propagation} - [:> comment-avatar* {:class (stl/css :avatar-lg) - :image (cfg/resolve-profile-photo-url profile)}]] - [:div {:class (stl/css :floating-thread-draft-inner-wrapper - :cursor-auto) - :style {:top (str (- pos-y 24) "px") - :left (str (+ pos-x 28) "px")} - - :on-click dom/stop-propagation} - [:div {:class (stl/css :form)} - [:> comment-input* - {:placeholder (tr "labels.write-new-comment") - :value (or content "") - :autofocus true - :on-esc on-esc - :on-change on-change - :on-ctrl-enter on-submit*}] - (when (exceeds-length? content) - [:div {:class (stl/css :error-text)} - (tr "errors.character-limit-exceeded")]) - [:> comment-form-buttons* {:on-submit on-submit* - :on-cancel on-esc - :is-disabled disabled?}]] - [:> mentions-panel*]]]])) + :left (str pos-x "px")} + :on-click dom/stop-propagation} + [:div {:class (stl/css :form)} + [:> comment-input* + {:placeholder (tr "labels.write-new-comment") + :value (or content "") + :autofocus true + :on-esc on-esc + :on-change on-change + :on-ctrl-enter on-submit*}] + (when (exceeds-length? content) + [:div {:class (stl/css :error-text)} + (tr "errors.character-limit-exceeded")]) + [:> comment-form-buttons* {:on-submit on-submit* + :on-cancel on-esc + :is-disabled disabled?}]] + [:> mentions-panel*]]])) (mf/defc comment-floating-thread-header* {::mf/private true} @@ -1057,7 +1063,10 @@ (mf/use-fn (mf/deps thread) (fn [content] - (st/emit! (dcm/add-comment thread content))))] + (st/emit! (dcm/add-comment thread content)))) + + on-cancel + (mf/use-fn #(st/emit! (dcm/close-thread)))] (mf/with-effect [thread-id] (st/emit! (dcm/retrieve-comments thread-id))) @@ -1092,60 +1101,65 @@ [:* {:key (dm/str (:id item))} [:> comment-floating-thread-item* {:comment item}]])] - [:> comment-reply-form* {:on-submit on-submit}] + [:> comment-reply-form* {:on-submit on-submit + :on-cancel (when (= origin :viewer) on-cancel)}] [:> mentions-panel*]])])) -(defn group-bubbles - "Group bubbles in different vectors by proximity" - ([zoom circles] - (group-bubbles zoom circles [] [])) +;; Screen-space gap (px) between concentric rings of an expanded cluster. +(def ^:private expanded-ring-gap 44) - ([zoom circles visited groups] - (if (empty? circles) - groups - (let [current (first circles) - remaining (rest circles) - overlapping-group (some (fn [group] - (when (some (partial dwcm/overlap-bubbles? zoom current) group) group)) - groups)] - (if overlapping-group - (group-bubbles zoom remaining visited (map (fn [group] - (if (= group overlapping-group) - (cons current group) - group)) - groups)) - (group-bubbles zoom remaining visited (cons [current] groups))))))) +;; Number of bubbles that fit in the innermost ring; each further ring +;; grows its capacity proportionally to its circumference. +(def ^:private expanded-ring-base 6) -(defn- inside-vbox? - "Checks if a bubble or a bubble group is inside a viewbox" - [thread-group wl] - (let [vbox (:vbox wl) - positions (mapv :position thread-group) - position (gpt/center-points positions) - pos-x (:x position) - pos-y (:y position) - x1 (:x vbox) - y1 (:y vbox) - x2 (+ x1 (:width vbox)) - y2 (+ y1 (:height vbox))] - (and (> x2 pos-x x1) (> y2 pos-y y1)))) +(defn- expanded-ring-slot + "Return [ring slot capacity] placing the bubble at index `i` (0-based) into + concentric rings, filling the innermost ring first." + [i] + (loop [ring 1 + i i] + (let [capacity (* expanded-ring-base ring)] + (if (< i capacity) + [ring i capacity] + (recur (inc ring) (- i capacity)))))) -(defn- calculate-zoom-scale - "Calculates the zoom level needed to ungroup the largest number of bubbles while - keeping them all visible in the viewbox." - [position zoom threads wl] - (let [num-threads (count threads) - grouped-threads (group-bubbles zoom threads) - num-grouped-threads (count grouped-threads) - zoom-scale-step 1.75 - scaled-zoom (* zoom zoom-scale-step) - zoomed-wl (dwz/impl-update-zoom wl position scaled-zoom) - outside-vbox? (complement inside-vbox?)] - (if (or (= num-threads num-grouped-threads) - (some #(outside-vbox? % zoomed-wl) grouped-threads)) - zoom - (calculate-zoom-scale position scaled-zoom threads zoomed-wl)))) +(defn- expanded-ring-vector + "Pure ring displacement (px) around the cluster center for the bubble at index + `i` (0-based), laid out into concentric rings, innermost first." + [i] + (let [[ring slot capacity] (expanded-ring-slot i) + ;; Start each ring at the top and stagger alternate rings by half a + ;; slot so bubbles don't line up radially. + angle (+ (* (/ slot capacity) 2 mth/PI) + (- (/ mth/PI 2)) + (if (even? ring) (/ mth/PI capacity) 0)) + radius (* ring expanded-ring-gap)] + (gpt/point (* radius (mth/cos angle)) + (* radius (mth/sin angle))))) + +(defn expanded-group-center + "Cluster center point (stored coordinates) shared by all bubbles of a group." + [thread-group] + (gpt/center-points (mapv :position thread-group))) + +(defn expanded-group-offsets + "Return a seq of [thread offset ring] triples laying each thread of + `thread-group` into a centered concentric-ring layout, leaving stored + positions untouched. `offset` is the total screen-space displacement from the + bubble's own position; `ring` is just the displacement from the cluster + center (used to animate the fan-out)." + [zoom thread-group] + (let [threads (sort-by :seqn thread-group) + center (expanded-group-center threads)] + (map-indexed + (fn [i thread] + (let [position (:position thread) + ring (expanded-ring-vector i) + base (gpt/point (* (- (:x center) (:x position)) zoom) + (* (- (:y center) (:y position)) zoom))] + [thread (gpt/add base ring) ring])) + threads))) (mf/defc comment-floating-group* {::mf/wrap [mf/memo]} @@ -1167,16 +1181,15 @@ ;; Click-through while transforming a shape, so it doesn't capture the drag dragging? (some? (mf/deref refs/current-transform)) + thread-ids (mf/with-memo [thread-group] + (into #{} (map :id) thread-group)) + on-click (mf/use-fn - (mf/deps thread-group position zoom) - (fn [] - (let [wl (deref refs/workspace-local) - centered-wl (dwv/calculate-centered-viewbox wl position) - updated-zoom (calculate-zoom-scale position zoom thread-group centered-wl) - scale-zoom (/ updated-zoom zoom)] - (st/emit! (dwv/update-viewport-position-center position) - (dwz/set-zoom position scale-zoom)))))] + (mf/deps thread-ids) + (fn [event] + (dom/stop-propagation event) + (st/emit! (dcm/expand-comment-group thread-ids))))] [:div {:style {:top (dm/str pos-y "px") :left (dm/str pos-x "px") @@ -1189,9 +1202,31 @@ :data-testid (dm/str "floating-thread-bubble-" test-id)} num-threads]])) +(mf/defc comment-floating-ghost* + "Dashed, semi-transparent placeholder shown at the cluster center while its + bubbles are fanned out into the expanded ring." + {::mf/wrap [mf/memo] + ::mf/private true} + [{:keys [thread-group zoom position-modifier]}] + (let [center (expanded-group-center thread-group) + center (cond-> center + (some? position-modifier) + (gpt/transform position-modifier)) + pos-x (floor (* (:x center) zoom)) + pos-y (floor (* (:y center) zoom)) + num-threads (str (count thread-group))] + [:div {:style {:top (dm/str pos-y "px") + :left (dm/str pos-x "px") + :pointer-events "none"} + :class (stl/css :floating-preview-wrapper :floating-preview-bubble :floating-preview-ghost)} + [:> comment-avatar* + {:class (stl/css :avatar-lg) + :variant "read"} + num-threads]])) + (mf/defc comment-floating-bubble* {::mf/wrap [mf/memo]} - [{:keys [thread zoom is-open on-click origin position-modifier]}] + [{:keys [thread zoom is-open on-click origin position-modifier offset ring]}] (let [owner (mf/with-memo [thread] (dcm/get-owner thread)) @@ -1202,6 +1237,10 @@ frame-id (:frame-id thread) + ;; A bubble with `offset` is one of the fanned-out members of an + ;; expanded cluster. + expanded? (some? offset) + ;; Click-through while transforming a shape, so it doesn't capture the drag dragging? (some? (mf/deref refs/current-transform)) @@ -1212,8 +1251,31 @@ :new-position-y nil :new-frame-id frame-id})) - pos-x (floor (* (or (:new-position-x @state) (:x position)) zoom)) - pos-y (floor (* (or (:new-position-y @state) (:y position)) zoom)) + new-x (:new-position-x @state) + new-y (:new-position-y @state) + + ;; While dragging, the new position already accounts for the ring offset; + ;; otherwise an expanded bubble sits at its stored position plus the + ;; screen-space ring offset. + pos-x (if (some? new-x) + (floor (* new-x zoom)) + (+ (floor (* (:x position) zoom)) + (if expanded? (:x offset) 0))) + pos-y (if (some? new-y) + (floor (* new-y zoom)) + (+ (floor (* (:y position) zoom)) + (if expanded? (:y offset) 0))) + + ;; World-space anchor a drag starts from: an expanded bubble is shown at + ;; its ring position, so dragging must begin there, not at its stored + ;; position. + drag-base-x (if expanded? (+ (:x position) (/ (:x offset) zoom)) (:x position)) + drag-base-y (if expanded? (+ (:y position) (/ (:y offset) zoom)) (:y position)) + + ;; CSS custom properties fed to the fan-out animation; nil for regular + ;; (non-expanded) bubbles. + ring-x (when (and expanded? (some? ring)) (dm/str (- (:x ring)) "px")) + ring-y (when (and expanded? (some? ring)) (dm/str (- (:y ring)) "px")) drag? (mf/use-ref nil) was-open? (mf/use-ref nil) @@ -1237,7 +1299,7 @@ on-pointer-up (mf/use-fn - (mf/deps origin thread (select-keys @state [:new-position-x :new-position-y :new-frame-id])) + (mf/deps origin thread expanded? (select-keys @state [:new-position-x :new-position-y :new-frame-id])) (fn [event] (when (not= origin :viewer) (swap! state assoc :is-grabbing false) @@ -1250,13 +1312,16 @@ (some? (:new-position-y @state))) (st/emit! (dwcm/update-comment-thread-position thread [(:new-position-x @state) (:new-position-y @state)])) + ;; Dropping a fanned-out bubble commits its new spot and closes + ;; the temporary cluster expansion. + (when expanded? (st/emit! (dcm/collapse-comment-group))) (swap! state assoc :new-position-x nil :new-position-y nil))))) on-pointer-move (mf/use-fn - (mf/deps origin drag? position zoom) + (mf/deps origin drag? drag-base-x drag-base-y zoom) (fn [event] (when (not= origin :viewer) (mf/set-ref-val! drag? true) @@ -1267,8 +1332,8 @@ delta-x (/ (- (:x current-pt) (:x start-pt)) zoom) delta-y (/ (- (:y current-pt) (:y start-pt)) zoom)] (swap! state assoc - :new-position-x (+ (:x position) delta-x) - :new-position-y (+ (:y position) delta-y))))))) + :new-position-x (+ drag-base-x delta-x) + :new-position-y (+ drag-base-y delta-y))))))) on-pointer-enter (mf/use-fn @@ -1286,7 +1351,7 @@ on-click* (mf/use-fn - (mf/deps origin thread on-click was-open? drag? (select-keys @state [:is-hover])) + (mf/deps origin thread on-click was-open? drag?) (fn [event] (dom/stop-propagation event) (when (or (and (mf/ref-val was-open?) (mf/ref-val drag?)) @@ -1298,34 +1363,42 @@ [:div {:style {:top (dm/str pos-y "px") :left (dm/str pos-x "px") - :pointer-events (when dragging? "none")} - :on-pointer-down on-pointer-down - :on-pointer-up on-pointer-up - :on-pointer-move on-pointer-move - :on-pointer-enter on-pointer-enter - :on-pointer-leave on-pointer-leave - :on-click on-click* + :pointer-events (when dragging? "none") + "--comment-ring-x" ring-x + "--comment-ring-y" ring-y} :class (stl/css-case :floating-preview-wrapper true - :floating-preview-bubble (false? (:is-hover @state)))} + :floating-preview-bubble (false? (:is-hover @state)) + :floating-preview-expanded expanded? + :floating-preview-hovered (:is-hover @state))} - (if (:is-hover @state) - [:div {:class (stl/css-case :floating-thread-wrapper true - :floating-preview-displacement true - :cursor-pointer (false? (:is-grabbing @state)) - :cursor-grabbing (true? (:is-grabbing @state)))} + ;; The avatar circle is the only pointer target: it drives hover, drag and + ;; click, so hovering the preview card below never keeps the tooltip open. + [:div {:on-pointer-down on-pointer-down + :on-pointer-up on-pointer-up + :on-pointer-move on-pointer-move + :on-pointer-enter on-pointer-enter + :on-pointer-leave on-pointer-leave + :on-click on-click* + :class (stl/css-case :floating-preview-avatar true + :cursor-pointer (false? (:is-grabbing @state)) + :cursor-grabbing (true? (:is-grabbing @state)))} + [:> comment-avatar* + {:image (cfg/resolve-profile-photo-url owner) + :class (stl/css :avatar-lg) + :data-testid (dm/str "floating-thread-bubble-" (:seqn thread)) + :variant (cond + (:is-resolved thread) "solved" + (pos? (:count-unread-comments thread)) "unread" + :else "read")}]] + + (when (:is-hover @state) + [:div {:class (stl/css :floating-thread-wrapper + :floating-preview-displacement + :floating-preview-hover-card)} [:div {:class (stl/css :floating-thread-item-wrapper)} [:div {:class (stl/css :floating-thread-item)} [:> comment-info* {:item thread - :profile owner}]]]] - - [:> comment-avatar* - {:image (cfg/resolve-profile-photo-url owner) - :class (stl/css :avatar-lg) - :data-testid (dm/str "floating-thread-bubble-" (:seqn thread)) - :variant (cond - (:is-resolved thread) "solved" - (pos? (:count-unread-comments thread)) "unread" - :else "read")}])])) + :profile owner}]]]])])) (mf/defc comment-sidebar-thread-item* {::mf/private true} diff --git a/frontend/src/app/main/ui/comments.scss b/frontend/src/app/main/ui/comments.scss index 8c99a6c95f..fc47433711 100644 --- a/frontend/src/app/main/ui/comments.scss +++ b/frontend/src/app/main/ui/comments.scss @@ -5,6 +5,7 @@ // Copyright (c) KALEIDOS INC Sucursal en España SL @use "refactor/common-refactor.scss" as deprecated; +@use "ds/_sizes.scss" as *; .cursor-grabbing { cursor: grabbing; @@ -104,6 +105,7 @@ justify-content: center; font-size: deprecated.$fs-12; background-color: var(--color-background-quaternary); + color: var(--color-foreground-quaternary); } .avatar-mask { @@ -167,31 +169,68 @@ z-index: initial; } -.floating-thread-draft-wrapper { - position: absolute; +// Sole pointer target of a floating bubble. +.floating-preview-avatar { display: flex; - flex-direction: row; - column-gap: deprecated.$s-12; + height: $sz-32; + width: $sz-32; +} - --translate-x: 0%; - --translate-y: 0%; +// Expanded-cluster bubble: sits above regular bubbles and fans out on appearance. +.floating-preview-expanded { + z-index: 2; + animation: comment-bubble-fan-out 0.18s ease-out; +} - transform: translate(var(--translate-x), var(--translate-y)); - - &.left { - --translate-x: -100%; - - flex-direction: row-reverse; +@keyframes comment-bubble-fan-out { + from { + translate: var(--comment-ring-x, 0) var(--comment-ring-y, 0); + opacity: 0; } - &.top { - --translate-y: -100%; - - align-items: flex-end; + to { + translate: 0 0; + opacity: 1; } } +// Hovered bubble's preview card floats above every other bubble. +.floating-preview-hovered { + z-index: 10; +} + +// Placeholder shown at the cluster center while its bubbles are fanned out. +.floating-preview-ghost { + z-index: 1; + opacity: 0.6; + pointer-events: none; + animation: comment-ghost-appear 0.18s ease-out; + + .avatar { + border-style: dashed; + background-color: var(--comment-modal-background-color); + } +} + +@keyframes comment-ghost-appear { + from { + opacity: 0; + } + + to { + opacity: 0.6; + } +} + +// Anchored to the wrapper origin and click-through, so hover stays bound to the avatar. +.floating-thread-wrapper.floating-preview-hover-card { + top: 0; + left: 0; + pointer-events: none; +} + .floating-thread-draft-inner-wrapper { + position: absolute; display: flex; flex-direction: column; gap: deprecated.$s-12; @@ -202,6 +241,19 @@ border: deprecated.$s-2 solid var(--modal-border-color); background-color: var(--comment-modal-background-color); max-height: var(--comment-height); + + --translate-x: 0%; + --translate-y: 0%; + + transform: translate(var(--translate-x), var(--translate-y)); + + &.left { + --translate-x: -100%; + } + + &.top { + --translate-y: -100%; + } } .floating-preview-displacement { diff --git a/frontend/src/app/main/ui/viewer/comments.cljs b/frontend/src/app/main/ui/viewer/comments.cljs index 4c18332a5d..e08d50cb5a 100644 --- a/frontend/src/app/main/ui/viewer/comments.cljs +++ b/frontend/src/app/main/ui/viewer/comments.cljs @@ -15,6 +15,7 @@ [app.common.geom.shapes :as gsh] [app.main.data.comments :as dcm] [app.main.data.event :as ev] + [app.main.data.workspace.comments :as dwcm] [app.main.refs :as refs] [app.main.store :as st] [app.main.ui.comments :as cmt] @@ -176,13 +177,22 @@ (-> (dcm/open-thread thread) (with-meta {::ev/origin "viewer"})))))) + expanded (:expanded local) + on-click (mf/use-fn - (mf/deps open-thread-id zoom page-id file-id modifier2) + (mf/deps open-thread-id expanded zoom page-id file-id modifier2) (fn [event] (dom/stop-propagation event) - (if (some? open-thread-id) + (cond + ;; A click anywhere first collapses a temporarily expanded cluster. + (some? expanded) + (st/emit! (dcm/collapse-comment-group)) + + (some? open-thread-id) (st/emit! (dcm/close-thread)) + + :else (let [event (dom/event->native-event event) position (-> (dom/get-offset-position event) (update :x #(/ % zoom)) @@ -204,24 +214,60 @@ (st/emit! (dcm/create-thread-on-viewer params) (dcm/close-thread)))))] + ;; Any zoom change collapses an expanded cluster back, matching the workspace. + (mf/with-effect [zoom] + (st/emit! (dcm/collapse-comment-group))) + [:div {:class (stl/css :comments-section) :on-click on-click} [:div {:class (dm/str cursor " " (stl/css :viewer-comments-container))} [:div {:class (stl/css :threads)} - (for [item threads] - [:> cmt/comment-floating-bubble* - {:thread item - :position-modifier modifier1 - :zoom zoom - :on-click on-bubble-click - :is-open (= (:id item) (:open local)) - :key (:seqn item) - :origin :viewer}]) + (for [thread-group (dwcm/group-bubbles zoom threads)] + (let [group? (> (count thread-group) 1) + thread (first thread-group) + expanded? (and group? + (= expanded (into #{} (map :id) thread-group)))] + (cond + expanded? + [:* {:key (:seqn thread)} + [:> cmt/comment-floating-ghost* + {:thread-group thread-group + :zoom zoom + :position-modifier modifier1}] + (for [[thread offset ring] (cmt/expanded-group-offsets zoom thread-group)] + [:> cmt/comment-floating-bubble* + {:thread thread + :position-modifier modifier1 + :zoom zoom + :offset offset + :ring ring + :on-click on-bubble-click + :is-open false + :origin :viewer + :key (:seqn thread)}])] + + group? + [:> cmt/comment-floating-group* + {:thread-group thread-group + :zoom zoom + :position-modifier modifier1 + :key (:seqn thread)}] + + :else + [:> cmt/comment-floating-bubble* + {:thread thread + :position-modifier modifier1 + :zoom zoom + :on-click on-bubble-click + :is-open (= (:id thread) (:open local)) + :origin :viewer + :key (:seqn thread)}]))) (when-let [thread (get threads-map open-thread-id)] [:> cmt/comment-floating-thread* {:thread thread :position-modifier modifier1 + :origin :viewer :viewport {:offset-x 0 :offset-y 0 :width (:width vsize) :height (:height vsize)} :zoom zoom}]) diff --git a/frontend/src/app/main/ui/workspace/viewport/comments.cljs b/frontend/src/app/main/ui/workspace/viewport/comments.cljs index 8fc476751c..288b537f1b 100644 --- a/frontend/src/app/main/ui/workspace/viewport/comments.cljs +++ b/frontend/src/app/main/ui/workspace/viewport/comments.cljs @@ -28,12 +28,14 @@ (mf/defc comment-floating-bubble-wrapper* {::mf/private true} - [{:keys [thread zoom is-open]}] + [{:keys [thread zoom is-open offset ring]}] (let [position-modifier (use-frame-position-modifier (:frame-id thread))] [:> cmt/comment-floating-bubble* {:thread thread :zoom zoom :position-modifier position-modifier + :offset offset + :ring ring :is-open is-open}])) (mf/defc comment-floating-group-wrapper* @@ -46,6 +48,16 @@ :zoom zoom :position-modifier position-modifier}])) +(mf/defc comment-floating-ghost-wrapper* + {::mf/private true} + [{:keys [thread-group zoom]}] + (let [thread (first thread-group) + position-modifier (use-frame-position-modifier (:frame-id thread))] + [:> cmt/comment-floating-ghost* + {:thread-group thread-group + :zoom zoom + :position-modifier position-modifier}])) + (mf/defc comment-floating-thread-wrapper* {::mf/private true} [{:keys [thread viewport zoom]}] @@ -93,6 +105,10 @@ (st/emit! (dwcm/initialize-comments file-id)) (fn [] (st/emit! ::dwcm/finalize))) + ;; Any viewport change (pan/zoom) collapses an expanded cluster back. + (mf/with-effect [vbox zoom] + (st/emit! (dcm/collapse-comment-group))) + [:div {:class (stl/css :comments-section)} [:div {:id "comments" @@ -102,13 +118,30 @@ [:div {:class (stl/css :threads) :style {:transform (dm/fmt "translate(%px, %px)" pos-x pos-y)}} - (for [thread-group (cmt/group-bubbles zoom threads)] - (let [group? (> (count thread-group) 1) - thread (first thread-group)] - (if group? + (for [thread-group (dwcm/group-bubbles zoom threads)] + (let [group? (> (count thread-group) 1) + thread (first thread-group) + expanded? (and group? + (= (:expanded local) (into #{} (map :id) thread-group)))] + (cond + expanded? + [:* {:key (:seqn thread)} + [:> comment-floating-ghost-wrapper* {:thread-group thread-group + :zoom zoom}] + (for [[thread offset ring] (cmt/expanded-group-offsets zoom thread-group)] + [:> comment-floating-bubble-wrapper* {:thread thread + :zoom zoom + :offset offset + :ring ring + :is-open false + :key (:seqn thread)}])] + + group? [:> comment-floating-group-wrapper* {:thread-group thread-group :zoom zoom :key (:seqn thread)}] + + :else [:> comment-floating-bubble-wrapper* {:thread thread :zoom zoom :is-open (= (:id thread) (:open local)) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 6fc3fdd1cc..a83cba73be 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -52,6 +52,7 @@ [frontend-tests.tokens.style-dictionary-test] [frontend-tests.tokens.token-errors-test] [frontend-tests.tokens.workspace-tokens-remap-test] + [frontend-tests.ui.comments-clustering-test] [frontend-tests.ui.comments-position-modifier-test] [frontend-tests.ui.ds-controls-numeric-input-test] [frontend-tests.ui.measures-menu-props-test] @@ -122,6 +123,7 @@ 'frontend-tests.tokens.style-dictionary-test 'frontend-tests.tokens.token-errors-test 'frontend-tests.tokens.workspace-tokens-remap-test + 'frontend-tests.ui.comments-clustering-test 'frontend-tests.ui.comments-position-modifier-test 'frontend-tests.ui.ds-controls-numeric-input-test 'frontend-tests.ui.measures-menu-props-test diff --git a/frontend/test/frontend_tests/ui/comments_clustering_test.cljs b/frontend/test/frontend_tests/ui/comments_clustering_test.cljs new file mode 100644 index 0000000000..eeaa7d1a97 --- /dev/null +++ b/frontend/test/frontend_tests/ui/comments_clustering_test.cljs @@ -0,0 +1,144 @@ +;; 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 INC Sucursal en España SL + +(ns frontend-tests.ui.comments-clustering-test + (:require + [app.common.geom.point :as gpt] + [app.common.math :as mth] + [app.main.data.comments :as dcm] + [app.main.data.workspace.comments :as dwcm] + [app.main.ui.comments :as cmt] + [cljs.test :as t :include-macros true] + [potok.v2.core :as ptk])) + +(defn- thread + [seqn x y] + {:id seqn :seqn seqn :position (gpt/point x y)}) + +(defn- magnitude + [p] + (gpt/distance p (gpt/point 0 0))) + +(defn- find-group + "Return the group (from a group-bubbles result) that contains the thread with + the given seqn." + [groups seqn] + (some (fn [group] + (when (some #(= (:seqn %) seqn) group) group)) + groups)) + +;; --- overlap-bubbles? ------------------------------------------------------ + +(t/deftest overlap-bubbles-close + (t/testing "bubbles within the overlap distance overlap" + (t/is (true? (dwcm/overlap-bubbles? 1 (thread 1 100 100) (thread 2 110 100)))))) + +(t/deftest overlap-bubbles-far + (t/testing "distant bubbles do not overlap" + (t/is (false? (dwcm/overlap-bubbles? 1 (thread 1 100 100) (thread 2 200 100)))))) + +(t/deftest overlap-bubbles-zoom + (t/testing "zoom scales the screen-space distance, separating close bubbles" + ;; 10px apart in canvas space is 40px on screen at zoom 4, above the 32px + ;; overlap threshold. + (t/is (false? (dwcm/overlap-bubbles? 4 (thread 1 100 100) (thread 2 110 100)))))) + +;; --- group-bubbles --------------------------------------------------------- + +(t/deftest group-bubbles-separate-clusters + (let [threads [(thread 1 100 100) + (thread 2 105 100) + (thread 3 500 500)] + groups (dwcm/group-bubbles 1 threads)] + (t/testing "produces one group per proximity cluster" + (t/is (= 2 (count groups)))) + (t/testing "overlapping bubbles share a group" + (t/is (= 2 (count (find-group groups 1)))) + (t/is (some #(= (:seqn %) 2) (find-group groups 1)))) + (t/testing "a distant bubble stays alone" + (t/is (= 1 (count (find-group groups 3))))))) + +(t/deftest group-bubbles-transitive-chain + ;; A-B and B-C overlap but A-C do not; the shared neighbour B keeps them in a + ;; single group. + (let [threads [(thread 1 100 100) + (thread 2 120 100) + (thread 3 140 100)] + groups (dwcm/group-bubbles 1 threads)] + (t/testing "a chain of overlaps forms a single group" + (t/is (= 1 (count groups))) + (t/is (= 3 (count (first groups))))))) + +;; --- expanded-group-center ------------------------------------------------- + +(t/deftest expanded-group-center-centroid + (t/testing "the cluster center is the centroid of the bubble positions" + (t/is (= (gpt/point 150 150) + (cmt/expanded-group-center [(thread 1 100 100) + (thread 2 200 200)]))))) + +;; --- expanded-group-offsets ------------------------------------------------ + +(t/deftest expanded-group-offsets-single-ring + (let [threads (mapv #(thread % 100 100) (range 6)) + offsets (cmt/expanded-group-offsets 1 threads)] + (t/testing "lays out every bubble of the cluster" + (t/is (= 6 (count offsets)))) + (t/testing "the innermost ring keeps a constant radius" + (t/is (every? #(mth/close? 44 (magnitude (nth % 2))) offsets))) + (t/testing "the first bubble is placed at the top of the ring" + (let [ring (nth (first offsets) 2)] + (t/is (mth/close? 0 (:x ring))) + (t/is (mth/close? -44 (:y ring))))))) + +(t/deftest expanded-group-offsets-concentric-rings + (let [threads (mapv #(thread % 100 100) (range 8)) + rings (mapv #(magnitude (nth % 2)) + (cmt/expanded-group-offsets 1 threads))] + (t/testing "the first six bubbles fill the inner ring" + (t/is (every? #(mth/close? 44 %) (take 6 rings)))) + (t/testing "the overflow spills onto a second, wider ring" + (t/is (every? #(mth/close? 88 %) (drop 6 rings)))))) + +(t/deftest expanded-group-offsets-total-offset + (t/testing "the total offset moves each bubble from its position to the ring" + ;; With all bubbles sharing a position, the base displacement is zero, so the + ;; total offset equals the pure ring vector. + (let [threads (mapv #(thread % 100 100) (range 3))] + (doseq [[_ offset ring] (cmt/expanded-group-offsets 1 threads)] + (t/is (mth/close? (:x offset) (:x ring))) + (t/is (mth/close? (:y offset) (:y ring))))))) + +;; --- cluster state transitions --------------------------------------------- + +(t/deftest expand-comment-group-replaces-open + (let [state {:comments-local {:open 5 :draft {} :options 9}} + result (ptk/update (dcm/expand-comment-group #{1 2 3}) state) + local (:comments-local result)] + (t/testing "expanding a cluster records its member ids" + (t/is (= #{1 2 3} (:expanded local)))) + (t/testing "expanding a cluster clears any open thread or draft" + (t/is (nil? (:open local))) + (t/is (nil? (:draft local)))))) + +(t/deftest collapse-comment-group-preserves-open + (let [state {:comments-local {:expanded #{1 2} :open 5}} + result (ptk/update (dcm/collapse-comment-group) state) + local (:comments-local result)] + (t/testing "collapsing clears the expansion" + (t/is (nil? (:expanded local)))) + (t/testing "collapsing leaves an open thread untouched" + (t/is (= 5 (:open local)))))) + +(t/deftest close-thread-clears-everything + (let [state {:comments-local {:open 5 :draft {} :expanded #{1} :options 9}} + result (ptk/update (dcm/close-thread) state) + local (:comments-local result)] + (t/testing "closing a thread clears open, draft, expanded and options" + (t/is (nil? (:open local))) + (t/is (nil? (:draft local))) + (t/is (nil? (:expanded local))) + (t/is (nil? (:options local))))))