mirror of
https://github.com/penpot/penpot.git
synced 2026-08-23 13:18:36 +00:00
🐛 Fix path editor code review findings
Fix issues identified during code review of path editor enhancements: - Fix unused binding lint warning in distribute-content that blocked CI - Fix collision-step floor comparison to use round instead of floor, correctly detecting collisions when coordinates drift slightly below integer boundaries - Fix resolve-edit-fills to recurse through empty parent groups when searching for inherited fills in nested group hierarchies - Fix expand-coincident-node-indices to use fuzzy comparison (gpt/close?) instead of exact equality, handling floating-point divergence after transforms or rotations - Remove unreachable dead code in path-point* on-pointer-down handler - Add tests for collision-step boundary cases, nested group fill inheritance, and coincident node alignment/flipping AI-assisted-by: mimo-v2.5-pro
This commit is contained in:
parent
7770aa807b
commit
9cc83c5c2b
@ -40,7 +40,7 @@
|
||||
(comp
|
||||
(filter (fn [[_ segment]]
|
||||
(and (not= :close-path (:command segment))
|
||||
(contains? points (helpers/segment->point segment)))))
|
||||
(some #(gpt/close? % (helpers/segment->point segment)) points))))
|
||||
(map first))
|
||||
(d/enumerate content))))
|
||||
|
||||
@ -198,7 +198,7 @@
|
||||
deltas (into {}
|
||||
(comp
|
||||
(map-indexed
|
||||
(fn [k {:keys [point indices]}]
|
||||
(fn [k {:keys [indices]}]
|
||||
(let [target (+ lo (* k step))]
|
||||
(map (fn [i]
|
||||
(let [node-point (get index->point i)
|
||||
|
||||
@ -1514,6 +1514,35 @@
|
||||
(t/is (= (gpt/point 10.0 0.0)
|
||||
(path.helpers/segment->point (nth result 1))))))
|
||||
|
||||
(t/deftest segment-align-content-coincident-nodes
|
||||
(t/testing "align-content groups coincident nodes with sub-epsilon coordinate differences"
|
||||
(let [content (path/content
|
||||
[{:command :move-to :params {:x 0.0 :y 0.0}}
|
||||
{:command :line-to :params {:x 10.0001 :y 2.0}}
|
||||
{:command :line-to :params {:x 10.0 :y 20.0}}])
|
||||
pts (fn [c] (mapv (comp (juxt :x :y) :params) (vec c)))]
|
||||
;; Nodes at ~10.0 should be grouped together for alignment
|
||||
(t/is (= [[0.0 0.0] [0.0 2.0] [0.0 20.0]]
|
||||
(pts (path/align-content content #{0 1 2} :hleft)))))))
|
||||
|
||||
(t/deftest segment-flip-content-coincident-nodes
|
||||
(t/testing "flip-content handles coincident nodes with sub-epsilon coordinate differences"
|
||||
(let [content (path/content
|
||||
[{:command :move-to :params {:x 0.0 :y 0.0}}
|
||||
{:command :line-to :params {:x 10.0001 :y 0.0}}
|
||||
{:command :line-to :params {:x 10.0 :y 10.0}}])
|
||||
pts (fn [c] (mapv (comp (juxt :x :y) :params) (vec c)))
|
||||
result (path/flip-content content #{0 1 2} :horizontal)]
|
||||
;; All nodes should flip across the horizontal center.
|
||||
;; Floating-point imprecision from the 10.0001 input is expected.
|
||||
(let [[[x0 y0] [x1 y1] [x2 y2]] (pts result)]
|
||||
(t/is (mth/close? 10.0 x0 0.001))
|
||||
(t/is (mth/close? 0.0 y0 0.001))
|
||||
(t/is (mth/close? 0.0 x1 0.001))
|
||||
(t/is (mth/close? 0.0 y1 0.001))
|
||||
(t/is (mth/close? 0.0 x2 0.001))
|
||||
(t/is (mth/close? 10.0 y2 0.001))))))
|
||||
|
||||
(t/deftest segment-set-handler-points
|
||||
(let [content (path/content
|
||||
[{:command :move-to :params {:x 0.0 :y 0.0}}
|
||||
|
||||
@ -68,7 +68,7 @@
|
||||
y-step (/ (:y delta) (:y paste-offset))]
|
||||
(when (and (not (neg? x-step))
|
||||
(mth/close? x-step y-step)
|
||||
(mth/close? x-step (mth/floor x-step)))
|
||||
(mth/close? x-step (mth/round x-step)))
|
||||
(long (mth/round x-step)))))
|
||||
|
||||
(defn available-offset-step
|
||||
|
||||
@ -883,7 +883,11 @@
|
||||
(let [parent (get objects parent-id)]
|
||||
(cond
|
||||
(nil? parent) []
|
||||
(cfh/group-shape? parent) (svg-fills/resolve-shape-fills parent)
|
||||
(cfh/group-shape? parent) (let [fills (svg-fills/resolve-shape-fills parent)]
|
||||
(if (seq fills)
|
||||
fills
|
||||
(recur (:parent-id parent)
|
||||
(conj visited parent-id))))
|
||||
(cfh/frame-shape? parent) []
|
||||
:else (recur (:parent-id parent)
|
||||
(conj visited parent-id)))))))))
|
||||
|
||||
@ -119,19 +119,15 @@
|
||||
(uwvv/capture-pointer event)
|
||||
(dom/stop-propagation event)
|
||||
(dom/prevent-default event)
|
||||
;; Preview nodes store their split params as metadata.
|
||||
;; FIXME: revisit this, using meta here breaks equality checks
|
||||
(if (and is-new (some? (meta position)))
|
||||
(st/emit! (drp/create-node-at-position (meta position)))
|
||||
(let [is-shift (kbd/shift? event)
|
||||
is-alt (kbd/alt? event)
|
||||
is-mod (kbd/mod? event)]
|
||||
(cond
|
||||
is-move
|
||||
(st/emit! (drp/start-move-path-point index is-shift is-alt is-mod))
|
||||
(let [is-shift (kbd/shift? event)
|
||||
is-alt (kbd/alt? event)
|
||||
is-mod (kbd/mod? event)]
|
||||
(cond
|
||||
is-move
|
||||
(st/emit! (drp/start-move-path-point index is-shift is-alt is-mod))
|
||||
|
||||
is-draw
|
||||
(st/emit! (drp/on-draw-node-pointer-down index position is-alt is-mod)))))))]
|
||||
is-draw
|
||||
(st/emit! (drp/on-draw-node-pointer-down index position is-alt is-mod))))))]
|
||||
|
||||
[:g.path-point
|
||||
[:circle.path-point
|
||||
|
||||
@ -95,3 +95,13 @@
|
||||
result (path.edition/resolve-edit-fills shape objects)]
|
||||
;; Should return empty fills instead of infinite loop
|
||||
(t/is (= [] result)))))
|
||||
|
||||
(t/deftest resolve-edit-fills-with-empty-intermediate-group
|
||||
(t/testing "resolve-edit-fills traverses past empty parent groups to find fills from ancestor"
|
||||
(let [objects {1 {:type :group :parent-id 2 :fills []}
|
||||
2 {:type :group :parent-id 3 :fills [{:fill-color "#00ff00"}]}
|
||||
3 {:type :frame :parent-id nil :fills []}}
|
||||
shape {:type :path :parent-id 1 :fills []}
|
||||
result (path.edition/resolve-edit-fills shape objects)]
|
||||
;; Should inherit fills from OuterGroup through empty InnerGroup
|
||||
(t/is (= [{:fill-color "#00ff00"}] result)))))
|
||||
|
||||
@ -237,3 +237,21 @@
|
||||
step (path.clipboard/available-offset-step existing pasted)]
|
||||
;; Should detect collision at step 1 and return step 0 as available
|
||||
(t/is (= 0 step)))))
|
||||
|
||||
(t/deftest collision-step-with-coordinates-slightly-below-integer
|
||||
(t/testing "collision-step detects collision when coordinates drift slightly below integer boundary"
|
||||
(let [pasted (gpt/point 10.0 10.0)
|
||||
existing (gpt/point 19.999 19.999)
|
||||
step (path.clipboard/collision-step pasted existing)]
|
||||
;; x-step = 0.9999, y-step = 0.9999
|
||||
;; With round-based check, these should be detected as collision at step 1
|
||||
(t/is (some? step))
|
||||
(t/is (= 1 step)))))
|
||||
|
||||
(t/deftest available-offset-step-with-coordinates-slightly-below-integer
|
||||
(t/testing "available-offset-step detects collision with sub-integer coordinate drift"
|
||||
(let [existing #{(gpt/point 19.999 19.999)}
|
||||
pasted #{(gpt/point 10.0 10.0)}
|
||||
step (path.clipboard/available-offset-step existing pasted)]
|
||||
;; Should detect collision at step 1 and return step 0 as available
|
||||
(t/is (= 0 step)))))
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user