From 9ae262b0ff9e11ad9c033ae29b158f8c991f6ebb Mon Sep 17 00:00:00 2001 From: elhombretecla Date: Wed, 28 Oct 2026 08:45:58 +0100 Subject: [PATCH] :sparkles: Show added and deleted layers in the board that holds them The compare dialog drew nothing for a deleted layer when the branch was open, and nothing for an added one when main was: the diff named the layer by id and label only, and the open file lacks it. The detail pane was left with the layer's name and nothing else. Added and deleted shape entries now carry the whole shape, as the document that holds it has it, under `:shape`. The key names no side, so it reads the same in both directions. The previews use it. A layer added or deleted inside a board is shown in that board on both sides, outlined in green or red on the side that holds it, so the change reads as what the board gains or loses. The facts of the detail pane (where it was, type, size) read it as well. A board that moved or resized now draws at its own size on the side the open file lacks: the diff leaves out `:selrect` and `:points`, so they are rebuilt from position and size instead of kept from the open side. --- common/src/app/common/files/branch_merge.cljc | 15 +++- .../common_tests/files_branch_merge_test.cljc | 22 +++++ .../sidebar/branch_review/compare.cljs | 41 ++++++--- .../sidebar/branch_review/facts.cljs | 15 +++- .../sidebar/branch_review/previews.cljs | 85 ++++++++++++++----- .../sidebar/branch_review/previews.scss | 15 ++++ frontend/test/frontend_tests/runner.cljs | 2 + .../ui/branch_review_previews_test.cljs | 59 +++++++++++++ 8 files changed, 218 insertions(+), 36 deletions(-) create mode 100644 frontend/test/frontend_tests/ui/branch_review_previews_test.cljs diff --git a/common/src/app/common/files/branch_merge.cljc b/common/src/app/common/files/branch_merge.cljc index 262d3785aa..fc0d34ffe1 100644 --- a/common/src/app/common/files/branch_merge.cljc +++ b/common/src/app/common/files/branch_merge.cljc @@ -839,6 +839,19 @@ :base (get bo (:id e)) :main (get to (:id e)) :branch (get oo (:id e)))) + ;; an added or deleted shape is missing from + ;; one of the two documents, and the compare + ;; panel may have that one open: the entry + ;; carries the FULL shape as the side that + ;; holds it has it, so the panel can draw it + ;; and say what it is. `:shape` names no side, + ;; so `doc-sides` leaves it alone + with-shape (fn [e] + (case (:status e) + :added (assoc e :shape (get oo (:id e))) + :deleted (assoc e :shape (or (get bo (:id e)) + (get to (:id e)))) + e)) passed (tree-policy-pass res bo to oo sbo sto reorders pid modes)] {:changes (mapv (fn [e] ;; a `:refuse` entry names the @@ -847,7 +860,7 @@ ;; display (if (= :unsupported (:status e)) e - (enrich e))) + (-> e enrich with-shape))) (:changes passed)) :conflicts (mapv (comp rehydrate enrich) (:conflicts passed))})) common)] diff --git a/common/test/common_tests/files_branch_merge_test.cljc b/common/test/common_tests/files_branch_merge_test.cljc index 633daaf2a5..cbb430f593 100644 --- a/common/test/common_tests/files_branch_merge_test.cljc +++ b/common/test/common_tests/files_branch_merge_test.cljc @@ -154,6 +154,28 @@ (t/is (= :deleted (:status c))) (t/is (= :s2 (:id c))))) +(t/deftest added-and-deleted-carry-the-shape + ;; the open file lacks the shape on one side, so the entry carries it + ;; whole, as the document that holds it has it, in both directions + (let [base (mkdata {:s1 {:id :s1 :name "A" :selrect {:x 1}} + :s2 {:id :s2 :name "B" :selrect {:x 2}}}) + branch (mkdata {:s1 {:id :s1 :name "A" :selrect {:x 1}} + :s3 {:id :s3 :name "C" :selrect {:x 3}}}) + merge (into {} (map (juxt :id identity)) + (:changes (bm/compute-merge base base branch :branch->main))) + update (into {} (map (juxt :id identity)) + (:changes (bm/compute-merge base branch base :main->branch)))] + (t/is (= {:id :s2 :name "B" :selrect {:x 2}} (get-in merge [:s2 :shape]))) + (t/is (= {:id :s3 :name "C" :selrect {:x 3}} (get-in merge [:s3 :shape]))) + (t/is (= {:id :s2 :name "B" :selrect {:x 2}} (get-in update [:s2 :shape]))) + (t/is (= {:id :s3 :name "C" :selrect {:x 3}} (get-in update [:s3 :shape]))))) + +(t/deftest modified-carries-no-shape + (let [base (mkdata {:s1 {:id :s1 :name "A" :fill "red"}}) + branch (mkdata {:s1 {:id :s1 :name "A" :fill "blue"}}) + r (bm/compute-merge base base branch :branch->main)] + (t/is (not (contains? (first (:changes r)) :shape))))) + (t/deftest fast-forward-main-untouched ;; main == base; branch changed -> all branch changes apply cleanly (let [base (mkdata {:s1 {:id :s1 :name "A" :fill "red"}}) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs index 6228fc7d23..3e6aadf874 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/compare.cljs @@ -226,7 +226,7 @@ "MAIN and BRANCH, side by side, each with what it holds for the change." {::mf/private true} [{:keys [kind main-value branch-value main-objects branch-objects - main-detail branch-detail main-missing branch-missing large]}] + main-detail branch-detail main-missing branch-missing large highlight]}] [:div {:class (stl/css :cards)} [:> preview-card* {:label (tr "workspace.branches.review.side.main") :side "main" @@ -235,7 +235,8 @@ [:> brp/entity-preview* {:kind kind :value main-value :objects main-objects - :missing main-missing}]] + :missing main-missing + :highlight highlight}]] [:> preview-card* {:label (tr "workspace.branches.review.side.branch") :side "branch" :detail branch-detail @@ -243,7 +244,8 @@ [:> brp/entity-preview* {:kind kind :value branch-value :objects branch-objects - :missing branch-missing}]]]) + :missing branch-missing + :highlight highlight}]]]) (mf/defc view-modes* "Side by side or overlay for the previews of a change, as icon toggles, @@ -282,12 +284,23 @@ (when objects (brp/side-objects objects page-items :main open-side))) branch-objs (mf/with-memo [objects page-items open-side] (when objects (brp/side-objects objects page-items :branch open-side))) - main-v (if (= :shape kind) - (or (get main-objs (:id item)) (get item :main)) - (brp/side-entity item :main open-side (library-entity data item))) - branch-v (if (= :shape kind) - (or (get branch-objs (:id item)) (get item :branch)) - (brp/side-entity item :branch open-side (library-entity data item))) + ;; an added or deleted layer inside a board is shown in it: the + ;; board on both sides, the layer outlined on the side that holds + ;; it, so the change reads as what the board gains or loses + stage (when (and (= :shape kind) (contains? #{:added :deleted} (:status item))) + (let [holder (if (contains? main-objs (:id item)) main-objs branch-objs) + fid (brp/context-frame-id holder (:id item))] + (when (and (some? fid) (contains? main-objs fid) (contains? branch-objs fid)) + fid))) + highlight (when stage {:id (:id item) :status (:status item)}) + main-v (cond + stage (get main-objs stage) + (= :shape kind) (or (get main-objs (:id item)) (get item :main)) + :else (brp/side-entity item :main open-side (library-entity data item))) + branch-v (cond + stage (get branch-objs stage) + (= :shape kind) (or (get branch-objs (:id item)) (get item :branch)) + :else (brp/side-entity item :branch open-side (library-entity data item))) main-detail (some-> (:main-at diff-meta) ct/inst ct/timeago) branch-detail (some-> (:branch-at diff-meta) ct/inst ct/timeago)] (when (or main-v branch-v) @@ -300,12 +313,15 @@ [:div {:class (stl/css :overlay-stage)} (when main-v [:div {:class (stl/css :overlay-layer)} - [:> brp/entity-preview* {:kind kind :value main-v :objects main-objs}]]) + [:> brp/entity-preview* {:kind kind :value main-v :objects main-objs + :highlight highlight}]]) (when branch-v [:div {:class (stl/css :overlay-layer :is-top)} - [:> brp/entity-preview* {:kind kind :value branch-v :objects branch-objs}]])]] + [:> brp/entity-preview* {:kind kind :value branch-v :objects branch-objs + :highlight highlight}]])]] [:> side-cards* {:kind kind :large large + :highlight highlight :main-value main-v :branch-value branch-v :main-objects main-objs @@ -402,7 +418,8 @@ :shape (when (contains? #{:added :deleted} (:status item)) - [:> brf/shape-facts* {:item item :data data :incoming incoming}]) + [:> brf/shape-facts* {:item item :items items :data data + :open-side open-side :incoming incoming}]) (:component :color :typography :token) [:> brf/usage-facts* {:item item :data data}] diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs index d2f17acb21..571afd1f50 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/facts.cljs @@ -8,7 +8,7 @@ "What the detail pane says about an entry that has no before and after: a new or deleted page or layer, and the library entries other layers depend on. It answers what the entry is, where it lands and what it - affects, from the open file." + affects, from the open file and, for a layer it lacks, from the diff." (:require-macros [app.main.style :as stl]) (:require [app.common.data.macros :as dm] @@ -137,8 +137,17 @@ (mf/defc shape-facts* "A new or deleted layer: where it lands (or was), what it is and, for a deleted main component, the copies it leaves behind." - [{:keys [item data incoming]}] - (let [objects (get-in data [:pages-index (:page-id item) :objects]) + [{:keys [item items data open-side incoming]}] + (let [open-objs (get-in data [:pages-index (:page-id item) :objects]) + ;; a layer the open file lacks is read on the other side, where + ;; the changes of `items` put it and its deleted parents back + objects (mf/with-memo [open-objs items item open-side] + (if (or (contains? open-objs (:id item)) (nil? open-objs)) + open-objs + (brp/side-objects open-objs + (filterv #(= (:page-id item) (:page-id %)) items) + (if (= open-side :main) :branch :main) + open-side))) shape (get objects (:id item)) page (get-in data [:pages-index (:page-id item) :name]) deleted? (= :deleted (:status item)) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.cljs index 7b6eb7e431..9edbc319a1 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.cljs @@ -6,12 +6,13 @@ (ns app.main.ui.workspace.sidebar.branch-review.previews "Visual previews of the two sides of a change. The diff carries whole - entities only for conflicts; for a clean change the open file holds one - side and the changed attrs give the other, which is enough to draw both - the layer and the board it sits in." + entities for conflicts and for added or deleted shapes; for a modified + entity the open file holds one side and the changed attrs give the + other, which is enough to draw both the layer and the board it sits in." (:require-macros [app.main.style :as stl]) (:require [app.common.data.macros :as dm] + [app.common.types.shape :as cts] [app.common.uuid :as uuid] [app.main.render :as render] [app.main.ui.ds.foundations.assets.icon :as i] @@ -28,6 +29,11 @@ icon instead." #{:rect :circle :frame :path :bool :text :group}) +(def ^:private box-shape-types + "Shape types whose geometry is their position and size; a path or a + boolean takes it from its content, a group from its children." + #{:rect :circle :frame :text :image :svg-raw}) + (defn open-side "Which document the open file is for `branch`: `:branch` when it is the branch file, `:main` otherwise." @@ -41,7 +47,8 @@ both; a clean change carries the attrs that differ, so the side the open file does not hold is the open entity with those attrs swapped in. An entry that does not exist on a side (added on the other one, or - deleted) has no entity there." + deleted) has no entity there. An added or deleted shape the open file + lacks comes whole in the entry's `:shape`." [item side open-side open-entity] (cond (= :conflict (:status item)) @@ -51,7 +58,7 @@ open-entity (nil? open-entity) - nil + (:shape item) (and (= :added (:status item)) (= side :main)) nil @@ -60,18 +67,27 @@ nil :else - (reduce-kv (fn [entity attr values] - (let [v (get values side)] - (if (nil? v) - (dissoc entity attr) - (assoc entity attr v)))) - open-entity - (:changed-attrs item)))) + (let [attrs (:changed-attrs item) + entity (reduce-kv (fn [entity attr values] + (let [v (get values side)] + (if (nil? v) + (dissoc entity attr) + (assoc entity attr v)))) + open-entity + attrs)] + ;; the diff leaves out `:selrect` and `:points` (they follow from + ;; position and size), so a box that moved or resized gets them + ;; again from its own, or it would draw with the open side's + (if (and (contains? box-shape-types (:type entity)) + (some #(contains? attrs %) [:x :y :width :height])) + (cts/setup-rect (dissoc entity :selrect :points)) + entity)))) (defn side-objects "The page `objects` as they stand on `side`, with every shape change of `items` that lives on this page put on that side. A shape missing on - that side is taken out of its parent, so boards draw without it." + that side is taken out of its parent, so boards draw without it, and a + shape only that side holds is put into its parent, so boards draw it." [objects items side open-side] (reduce (fn [objects item] @@ -80,7 +96,12 @@ (let [id (:id item) entity (side-entity item side open-side (get objects id))] (if (some? entity) - (assoc objects id entity) + (let [parent-id (:parent-id entity)] + (cond-> (assoc objects id entity) + (and (contains? objects parent-id) + (not (contains? objects id)) + (not (some #(= id %) (get-in objects [parent-id :shapes])))) + (update-in [parent-id :shapes] (fnil conj []) id))) (let [parent-id (:parent-id (get objects id))] (cond-> (dissoc objects id) (contains? objects parent-id) @@ -100,23 +121,47 @@ (let [sr (:selrect shape)] {:x (:x sr 0) :y (:y sr 0) :width (:width sr 1) :height (:height sr 1)})))) +(defn context-frame-id + "The nearest board that holds shape `id` in `objects`, the page root + left out: the stage an added or deleted layer is shown on." + [objects id] + (loop [parent-id (:parent-id (get objects id)) + depth 0] + (let [parent (get objects parent-id)] + (cond + (or (nil? parent) (= parent-id uuid/zero) (>= depth 64)) nil + (= :frame (:type parent)) parent-id + :else (recur (:parent-id parent) (inc depth)))))) + (mf/defc shape-preview* "Renders `shape` to a fitted SVG with the real shape renderers. `objects` - supplies its children (a board or group draws them)." + supplies its children (a board or group draws them). `highlight` + (`{:id :status}`) outlines the layer an added or deleted change is + about, when `objects` holds it." {::mf/private true} - [{:keys [shape objects]}] + [{:keys [shape objects highlight]}] (let [{:keys [x y width height]} (shape-bounds shape) width (max 1 width) height (max 1 height) objects (mf/with-memo [shape objects] (assoc (or objects {}) (:id shape) shape)) + marked (some->> (:id highlight) (get objects) shape-bounds) wrapper (mf/with-memo [objects] (render/shape-wrapper-factory objects))] [:svg {:class (stl/css :preview-svg) :viewBox (dm/str x " " y " " width " " height) :preserveAspectRatio "xMidYMid meet" :xmlns "http://www.w3.org/2000/svg" :fill "none"} - [:> wrapper {:shape shape}]])) + [:> wrapper {:shape shape}] + (when marked + [:rect {:class (stl/css-case :preview-highlight true + :is-added (= :added (:status highlight)) + :is-deleted (= :deleted (:status highlight))) + :x (:x marked) + :y (:y marked) + :width (max 1 (:width marked)) + :height (max 1 (:height marked)) + :vector-effect "non-scaling-stroke"}])])) (mf/defc page-preview* "The whole page `objects` drawn to fit: every top-level layer, framed @@ -192,8 +237,8 @@ shapes render to SVG, library colors to a swatch, typographies to a text sample; everything else shows a type icon. A side with nothing to draw says why (`missing`: `{:reason :side :open-side}`, see - `missing-reason`)." - [{:keys [kind value objects missing]}] + `missing-reason`). A shape `highlight` is passed on to `shape-preview*`." + [{:keys [kind value objects missing highlight]}] (cond (nil? value) [:> missing-preview* {:reason (:reason missing :unavailable) @@ -202,7 +247,7 @@ (= kind :shape) (if (contains? renderable-shape-types (:type value)) - [:> shape-preview* {:shape value :objects objects}] + [:> shape-preview* {:shape value :objects objects :highlight highlight}] [:div {:class (stl/css :preview-empty)} [:> i/icon* {:icon-id (brc/shape-icon-id {:shape-type (:type value)})}]]) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.scss b/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.scss index a90fd6b733..7130249a5b 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.scss +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/previews.scss @@ -13,6 +13,21 @@ block-size: 100%; } +// the outline of the layer an added or deleted change is about, drawn +// over the board that holds it; the stroke keeps its width at any zoom +.preview-highlight { + --preview-highlight-color: var(--color-accent-success); + + fill: color-mix(in srgb, var(--preview-highlight-color) 12%, transparent); + stroke: var(--preview-highlight-color); + stroke-width: 2; + stroke-dasharray: 4 3; + + &.is-deleted { + --preview-highlight-color: var(--color-accent-error); + } +} + .preview-swatch { inline-size: 100%; block-size: 100%; diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 77678bb47c..f7d6ad59cb 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -111,6 +111,7 @@ [frontend-tests.tokens.token-errors-test] [frontend-tests.tokens.workspace-tokens-remap-test] [frontend-tests.ui.branch-review-common-test] + [frontend-tests.ui.branch-review-previews-test] [frontend-tests.ui.branches-compare-labels-test] [frontend-tests.ui.check-updates-test] [frontend-tests.ui.color-token-position-test] @@ -270,6 +271,7 @@ 'frontend-tests.tokens.logic.tokens-status-test 'frontend-tests.tokens.workspace-tokens-remap-test 'frontend-tests.ui.branch-review-common-test + 'frontend-tests.ui.branch-review-previews-test 'frontend-tests.ui.branches-compare-labels-test 'frontend-tests.ui.check-updates-test 'frontend-tests.ui.color-token-position-test diff --git a/frontend/test/frontend_tests/ui/branch_review_previews_test.cljs b/frontend/test/frontend_tests/ui/branch_review_previews_test.cljs new file mode 100644 index 0000000000..0f99c7f85e --- /dev/null +++ b/frontend/test/frontend_tests/ui/branch_review_previews_test.cljs @@ -0,0 +1,59 @@ +;; 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.ui.branch-review-previews-test + "How the compare previews rebuild the side the open file lacks: a + deleted layer comes back from the diff, inside the board it was in, + and a resized board draws at its own size." + (:require + [app.common.types.shape :as cts] + [app.common.uuid :as uuid] + [app.main.ui.workspace.sidebar.branch-review.previews :as brp] + [cljs.test :as t :include-macros true])) + +(def ^:private board-id (uuid/next)) +(def ^:private icon-id (uuid/next)) +(def ^:private path-id (uuid/next)) + +(def ^:private branch-objects + "The branch, open: the button lost its icon and shrank." + {uuid/zero {:id uuid/zero :type :frame :shapes [board-id]} + board-id (cts/setup-rect {:id board-id :type :frame :name "Button" + :parent-id uuid/zero :x 0 :y 0 :width 131 :height 48 + :shapes []})}) + +(def ^:private icon + {:id icon-id :type :group :name "icon" :parent-id board-id + :x 119 :y 12 :width 24 :height 24 :shapes [path-id]}) + +(def ^:private items + [{:kind :shape :status :deleted :id path-id + :shape {:id path-id :type :path :name "path" :parent-id icon-id}} + {:kind :shape :status :deleted :id icon-id :shape icon} + {:kind :shape :status :modified :id board-id + :changed-attrs {:width {:main 163 :branch 131}}}]) + +(t/deftest a-deleted-layer-comes-back-on-the-side-that-has-it + (let [main (brp/side-objects branch-objects items :main :branch)] + (t/is (= icon (get main icon-id))) + (t/is (contains? main path-id)) + (t/is (= [icon-id] (get-in main [board-id :shapes]))))) + +(t/deftest the-side-that-deleted-it-stays-without-it + (let [branch (brp/side-objects branch-objects items :branch :branch)] + (t/is (not (contains? branch icon-id))) + (t/is (= [] (get-in branch [board-id :shapes]))))) + +(t/deftest a-resized-board-draws-at-its-own-size + (let [main (brp/side-objects branch-objects items :main :branch)] + (t/is (= 163 (get-in main [board-id :selrect :width]))) + (t/is (= 131 (get-in branch-objects [board-id :selrect :width]))))) + +(t/deftest a-layer-is-staged-in-the-nearest-board + (let [main (brp/side-objects branch-objects items :main :branch)] + (t/is (= board-id (brp/context-frame-id main path-id))) + (t/is (= board-id (brp/context-frame-id main icon-id))) + (t/is (nil? (brp/context-frame-id main board-id)))))