Skip to content

Commit becb59c

Browse files
angusbezzinaclaude
andcommitted
fix(overlay): a click on a pin edits its comment, in both tools (VRT-u209)
A comment written with the FRAME tool could be read and never edited. Pins were made hit-test-inert in frame mode so a press starting on one could still draw a frame (VRT-dp47) — but going inert was only ever meant to stop a pin swallowing a DRAG, and it also removed the only route to the editor. Reported as "I need to click on the comment again to edit the text… right now when I hover on one of the numbers, nothing happens". Diagnosed by measurement rather than by reading: the probe's phase 11c presses a real pin in each tool and printed editingNoteID=nil in frame mode, while point mode opened the editor. That leg had PINNED the empty result as the desired outcome, so the contract was half-written down and the gap looked deliberate. The press is now routed by GEOMETRY at the catcher's release — PinAttentionRule.pressedNote via SelectionGesture — so the pin view stays inert and the pin stays live. TRAVEL, NOT TOOL, IS THE GATE: a click on a pin edits it in either tool, a drag from a pin still draws its frame. One sentence true of both, so the VRT-dp47 report cannot come back; mutation-verified (dropping the gate fails exactly the two drag-from-pin tests) and measured by a new probe leg that drags off a pin and asserts the frame still resolves. The press radius is the pin's OWN (10pt), deliberately not the 20pt attention radius: attending is free and may reach outside the pin, a click is scarce because the same press could have selected the element underneath. Wired to attentionRadius every pin would be a 40pt hole in the canvas that looks 20pt. Highlighting the captured selection while its card is open needed no new plumbing — attendedNoteID is editingNoteID ?? hoveredNoteID, so the open card already wins the tie — but it was only covered for the hover path. It is now asserted end-to-end in the tool it was broken in, and in the probe for both. 230 unit tests, 0 failures; probe 11/11 phases PASS against a live AX tree; iOS module type-checks clean against the iPhoneOS SDK in Swift 6. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent c16ef8b commit becb59c

10 files changed

Lines changed: 443 additions & 41 deletions

File tree

DECISIONS.md

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -263,14 +263,32 @@ window) leaves the catcher's last answer pointing at the pin it saw before.
263263
Observed exactly that way during the visual check — the card read note 1 while the
264264
canvas drew note 3.
265265

266-
**Why pins are inert in frame mode.** `AnnotationPin` is a `Button` mounted above
267-
the catcher, so a press starting on one never reaches the drag gesture — and every
268-
capture plants one exactly where the next frame is most likely to be drawn. That,
269-
plus hover-to-edit dropping a ~284x172 card under the pointer, is the reported "if
270-
I select an element I cannot also use the frame tool". In frame mode the user is
271-
drawing, not editing, so one condition removes both with no gesture negotiation.
272-
Measured both ways in the probe (11c): the same synthesized press opens the editor
273-
in point mode and does nothing in frame mode.
266+
**Why the pin VIEW is inert in frame mode.** `AnnotationPin` is a `Button` mounted
267+
above the catcher, so a press starting on one never reaches the drag gesture — and
268+
every capture plants one exactly where the next frame is most likely to be drawn.
269+
That, plus hover-to-edit dropping a ~284x172 card under the pointer, is the reported
270+
"if I select an element I cannot also use the frame tool". In frame mode the user is
271+
drawing, so one condition removes both with no gesture negotiation.
272+
273+
**Why a pin is still CLICKABLE there (VRT-u209).** Going hit-test-inert was meant to
274+
stop a pin swallowing a DRAG; it also removed the only route to the editor, so a
275+
comment written with the frame tool could be read and never edited — reported as "I
276+
need to click the comment again to edit it… right now when I hover on one of the
277+
numbers, nothing happens", and settled by probe 11c printing `editingNoteID=nil` for
278+
a real press on a real pin in frame mode. The press is now routed by GEOMETRY at the
279+
catcher's release (`PinAttentionRule.pressedNote` via `SelectionGesture`), so the
280+
view stays inert and the pin stays live. **Travel, not tool, is the gate**: a click
281+
on a pin edits it in either tool, a drag from a pin still draws its frame — one
282+
sentence true of both, and the `VRT-dp47` report cannot come back. Probe 11c now
283+
measures all three legs (click in each tool, drag from a pin).
284+
285+
**Why the press radius is the pin's own, not `attentionRadius`.** The two circles are
286+
paid for differently. Attending is free — it draws a picture and takes nothing away —
287+
so it may reach outside the pin (and must, since in point mode the catcher only ever
288+
sees points outside a live button). A click is scarce: the same press could have
289+
selected the element underneath, so its target is exactly the circle the user can
290+
SEE. Wired to `attentionRadius`, every pin would be a 40pt hole in the canvas that
291+
looks 20pt wide.
274292

275293
**The composer covering its own element is ACCEPTED.** A card the user may be
276294
typing into must consume its own clicks; dismissing it or starting the drag from

PARITY.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ row; each asymmetry is closed by code or has a tracked mitigation.
2222
| The first click on the overlay acts | `FirstMouseHostingView` (`acceptsFirstMouse -> true`) on both panels' content views | n/a | ASYMMETRIC BY THE PLATFORM, not by the code: "a mouse-down in a non-key window is discarded unless the view accepts first mouse" is an AppKit window-activation rule with no UIKit equivalent — iOS has no key window a press must first buy, and a touch on the pass-through `UIWindow` is delivered on the first tap. Nothing to mirror; the fix lives entirely in the macOS host |
2323
| Live selection follows the content on scroll | `translateSelection(by:within:)`, driven by the same measurement `KeyablePanel.scrollWheel` already makes | none | SAME asymmetry and the same reason as the row for captured notes below: the correction is affordable on macOS only because the overlay panel intercepts every wheel event and drives the clip itself. The receiving side is shared and unit-tested (`translateSelection` is pure geometry over session state), so an iOS host that observes its own scrollers can adopt it unchanged |
2424
| Recallable selection marks (a captured note's geometry comes back on hover) | shared: `AnnotationNote.anchorRect`/`drawnRect` snapshotted in `AnnotationSession.addNote`, `PinAttentionRule` + `RecalledMark` as pure rules, rendered by the shared `AnnotationMarks` | same | none — every part of it is platform-free. The two rects are WINDOW-LOCAL, which is the one place a platform could have crept in: macOS subtracts the overlay panel's `axOrigin`, iOS passes `.zero` because its view-tree frames are already view-local, and `addNote` takes that origin as a parameter rather than knowing which platform it is on. Both fields are UI-only and out of `CodingKeys`, so the exported record is identical on both |
25-
| Pins inert in frame mode / hover-to-edit gated to point mode | shared `AnnotationPins` (`allowsHitTesting(session.tool == .point)`, and the same condition on the hover that opens the editor) | same | none in the code. ASYMMETRIC VERIFICATION, recorded rather than closed: `AnnotKitOverlayProbe` phase 11c posts a real press at a pin's own coordinates in each mode and is macOS-only, so on iOS the fix is covered only by the unit tests over the session and the rules. Note the hover half is largely moot on touch-only iOS — hover exists there only with a trackpad or pencil |
25+
| Pin VIEW inert in frame mode / hover-to-edit gated to point mode | shared `AnnotationPins` (`allowsHitTesting(session.tool == .point)`, and the same condition on the hover that opens the editor) | same | none in the code. ASYMMETRIC VERIFICATION, recorded rather than closed: `AnnotKitOverlayProbe` phase 11c posts a real press at a pin's own coordinates in each mode and is macOS-only, so on iOS the fix is covered only by the unit tests over the session and the rules. Note the hover half is largely moot on touch-only iOS — hover exists there only with a trackpad or pencil |
26+
| Pin CLICK-TO-EDIT in both tools (VRT-u209) | shared: `SelectionGesture.resolve(…pins:)` consults `PinAttentionRule.pressedNote` on a press that did not travel, and the shared catcher in `OverlayView` opens the editor for `.editNote` | same | none — the whole decision is a pure rule over the notes' stored window-local anchors, so both hosts route a tap identically, and iOS gets it from the one catcher it already shares. The TRAVEL gate is the only platform-sensitive part and it is already parameterized (`SelectionGesture.minimumTravel` is 10 on iOS vs 6 on macOS, because a finger rolls several points on a deliberate tap) — so a touch that would have been a sloppy tap still opens the pin's editor rather than being read as a drag. Verification is the same asymmetry as the row above: probe 11c is macOS-only; iOS relies on the shared unit tests |
2627
| Recall survives a scroll | `KeyablePanel.scrollWheel` measures the translation it applied to the host's `NSScrollView` and feeds `AnnotationSession.translateNotes(by:within:)` | none | ASYMMETRIC BY THE MECHANISM: the correction is affordable on macOS only because the overlay panel already intercepts every wheel event and drives the clip itself, so the exact translation is known at the moment it is applied. iOS does not intercept `UIScrollView`, so nothing there knows a scroll happened; a note's rects stay where they were captured, which is the pre-existing behaviour `AnnotationPins` has always accepted for the pin. The RECEIVING side is shared and unit-tested (`translateNotes` is pure bookkeeping over `pending`), so an iOS host that later observes its own scrollers can adopt it without re-deriving anything |
2728
| Overlay excluded from element lookup | AX window identifier (`AXIntrospection.overlayWindowIdentifier`) filtered out of every `kAXWindows` read | `PassThroughWindow` TYPE identity filtered out of `IOSElementSource.windows()` | ASYMMETRIC BY NECESSITY — the hosts are different window kinds. macOS's overlay is a separate `NSPanel` matched by the identifier the controller stamps on it; iOS's is a `UIWindow` in the HOST's scene sharing its pid, so no pid/scene filter separates it and a type check (internal to the module) cannot drift the way an identifier convention can. Both filter in the shared window lookup, so snapshot / hit-test / region-anchor / marquee agree; leaving it in would let a marquee bind the user's note to AnnotKit's own UI |
2829
| Coordinate space | Cocoa bottom-left to AX top-left flip | UIKit top-left native | iOS needs no flip; shared `ScreenSpace` used only on macOS |

Sources/AnnotKit/Overlay/AnnotationPins.swift

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,20 +19,36 @@ import SwiftUI
1919
/// The whole overlay stays `accessibilityHidden`, so pins never disturb the AX
2020
/// point query's see-through.
2121
///
22-
/// IN FRAME MODE THEY ARE INERT. Sitting above the catcher means a press that
23-
/// starts on a pin never reaches the catcher's drag gesture — and every capture
24-
/// plants one exactly where the user is most likely to draw the next frame, on the
25-
/// thing they just annotated. That is the "I selected an element and now I can't
26-
/// use the frame tool" report. In frame mode the user is drawing, not editing, and
27-
/// the tool is an explicit choice ("the tool you picked is the outcome you get",
28-
/// per ``SelectionGesture``), so one condition removes the collision with no
29-
/// gesture negotiation — the thing `VRT-dp47` learned to stay away from.
22+
/// IN FRAME MODE THE VIEW IS INERT — the view, not the pin. Sitting above the
23+
/// catcher means a press that starts on a pin never reaches the catcher's drag
24+
/// gesture, and every capture plants one exactly where the user is most likely to
25+
/// draw the next frame, on the thing they just annotated. That is the "I selected
26+
/// an element and now I can't use the frame tool" report. One condition removes the
27+
/// collision with no gesture negotiation — the thing `VRT-dp47` learned to stay
28+
/// away from.
3029
///
31-
/// Recall does NOT ride on the pin's own hover, and that is what makes going inert
32-
/// affordable: a view with `allowsHitTesting(false)` receives no hover at all.
33-
/// "Which note's pin is the pointer on?" is answered geometrically by
34-
/// ``PinAttentionRule``, from the catcher's `onContinuousHover`, which covers the
35-
/// whole surface and fires in BOTH modes.
30+
/// WHAT THE PIN STILL DOES THERE is the correction `VRT-u209` made, and it is worth
31+
/// separating from the sentence above because they were conflated for two releases.
32+
/// Going hit-test-inert was only ever meant to stop a pin SWALLOWING A DRAG; it also
33+
/// silently removed the only way to re-open a note, so a comment written with the
34+
/// frame tool could be read and never edited. Measured, not assumed — the probe's
35+
/// phase 11c presses a real pin in both tools and reported `editingNoteID=nil` in
36+
/// frame mode, which is exactly the "I hover the numbers and nothing happens"
37+
/// report. Both behaviours are now had at once, because neither the click nor the
38+
/// recall rides on this view any more:
39+
///
40+
/// * **Click → edit** is resolved GEOMETRICALLY at the catcher's release, by
41+
/// ``PinAttentionRule/pressedNote(atWindowPoint:in:)`` via ``SelectionGesture``,
42+
/// gated on the press not having travelled. So a click on a pin edits it in BOTH
43+
/// tools, and a drag from a pin still draws its frame.
44+
/// * **Recall** ("which note's pin is the pointer on?") is answered the same way
45+
/// from the catcher's `onContinuousHover`, which covers the whole surface and
46+
/// fires in both tools.
47+
///
48+
/// The `Button` below is therefore a point-mode convenience that duplicates a route
49+
/// that already works, not the mechanism — deliberately kept, because it is the one
50+
/// path that answers a press on the pin's exact pixels without consulting geometry,
51+
/// and it costs nothing to leave in agreement with the rule.
3652
struct AnnotationPins: View {
3753
@ObservedObject var session: AnnotationSession
3854

Sources/AnnotKit/Overlay/OverlayView.swift

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -326,8 +326,19 @@ struct OverlayView: View {
326326
tool: session.tool,
327327
from: value.startLocation,
328328
to: value.location,
329-
axOrigin: axOrigin
329+
axOrigin: axOrigin,
330+
// The retained notes, so a click that landed on a pin
331+
// re-opens that note rather than binding a new one.
332+
// Handing the WHOLE set (not a pre-filtered hit) keeps
333+
// the overlap tie-break — last drawn wins — inside the
334+
// rule with the geometry it belongs to.
335+
pins: session.pending
330336
) {
337+
case .editNote(let id):
338+
// The one branch that does NOT touch the app beneath:
339+
// it opens the note's own card, which nils `selected`
340+
// for the usual exactly-one-card-at-a-time reason.
341+
session.beginEditing(id: id)
331342
case .point(let point):
332343
session.select(atAXPoint: point)
333344
case .frame(let rect):

Sources/AnnotKit/Overlay/PinAttentionRule.swift

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,19 +43,53 @@ enum PinAttentionRule {
4343
/// window from empty space has already set the answer to nil on its way out.
4444
static let attentionRadius: CGFloat = pinDiameter
4545

46+
/// How close a CLICK must come to a pin's centre to open that note's editor —
47+
/// the pin's OWN drawn radius, deliberately NOT ``attentionRadius``.
48+
///
49+
/// The two differ because they are paid for differently. Attending a pin is free:
50+
/// it draws a picture and takes nothing away, so a generous annulus outside the
51+
/// pin costs a user nothing when it fires early. A click is scarce — the same
52+
/// press could have selected the element underneath — so its target must be
53+
/// exactly the circle the user can SEE. At ``attentionRadius`` a press 18pt clear
54+
/// of a pin would silently open an editor instead of selecting what was pressed,
55+
/// and the pin would be a 40pt hole in the canvas that looks 20pt wide.
56+
static let pressRadius: CGFloat = pinDiameter / 2
57+
4658
/// The id of the note whose pin the pointer at `point` (WINDOW-LOCAL, the space
4759
/// ``AnnotationNote/anchorRect`` is stored in) is attending, or nil for a point
4860
/// near no pin.
61+
static func attendedNote(atWindowPoint point: CGPoint, in notes: [AnnotationNote]) -> String? {
62+
note(atWindowPoint: point, in: notes, within: attentionRadius)
63+
}
64+
65+
/// The id of the note whose pin a CLICK at `point` (WINDOW-LOCAL) landed on, or
66+
/// nil for a press that hit no pin.
4967
///
68+
/// This is what makes click-to-edit work in BOTH tools. In point mode the pin is
69+
/// a live `Button` and answers its own click; in frame mode it is deliberately
70+
/// `allowsHitTesting(false)` — so a press there reaches the catcher, and geometry
71+
/// is the only thing left that can say "that was pin 3". Deciding it from the
72+
/// stored anchors rather than from a view means the answer does not depend on
73+
/// which tool happens to be active, which is the whole defect this closes: a
74+
/// note captured in frame mode could be read but never re-opened.
75+
///
76+
/// The CALLER owns the click-vs-drag distinction (see ``SelectionGesture``): a
77+
/// press that travelled is a drag, and a drag that began on a pin must still draw
78+
/// its frame.
79+
static func pressedNote(atWindowPoint point: CGPoint, in notes: [AnnotationNote]) -> String? {
80+
note(atWindowPoint: point, in: notes, within: pressRadius)
81+
}
82+
5083
/// LAST NOTE WINS on overlap, because pins are drawn in `pending` order and a
5184
/// later one is therefore painted ON TOP of an earlier one. The user can only
52-
/// mean the pin they can see.
53-
static func attendedNote(atWindowPoint point: CGPoint, in notes: [AnnotationNote]) -> String? {
85+
/// mean the pin they can see — which is why both callers share this walk rather
86+
/// than each writing their own loop and disagreeing about the tie.
87+
private static func note(atWindowPoint point: CGPoint, in notes: [AnnotationNote], within radius: CGFloat) -> String? {
5488
for note in notes.reversed() {
5589
guard let anchor = note.anchorRect?.origin else { continue }
5690
let dx = point.x - anchor.x
5791
let dy = point.y - anchor.y
58-
if dx * dx + dy * dy <= attentionRadius * attentionRadius { return note.id }
92+
if dx * dx + dy * dy <= radius * radius { return note.id }
5993
}
6094
return nil
6195
}

Sources/AnnotKit/Overlay/SelectionGesture.swift

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,11 @@ enum SelectionGesture {
4444
/// — the transform living in one place is why the click path, the frame path
4545
/// and the highlight agree on a secondary display.
4646
enum Outcome: Equatable {
47+
/// The press landed on the numbered pin of an already-captured note: open
48+
/// that note's editor instead of binding anything new. Carries the note id
49+
/// rather than a coordinate because nothing downstream needs the geometry —
50+
/// the card anchors itself to the pin it belongs to.
51+
case editNote(String)
4752
case point(CGPoint)
4853
case frame(CGRect)
4954
case none
@@ -61,12 +66,42 @@ enum SelectionGesture {
6166

6267
/// Route a completed press. The one place the mode semantics live, so they are
6368
/// pinned by tests rather than by a human dragging a mouse.
69+
///
70+
/// `pins` is the retained note set, consulted so a CLICK on an existing pin
71+
/// re-opens that note instead of binding a new one — in BOTH tools. It defaults
72+
/// to empty, which reproduces the original two-branch routing exactly, so a
73+
/// caller with no notes on screen pays nothing and reads the same.
6474
static func resolve(
6575
tool: AnnotationSession.SelectionTool,
6676
from start: CGPoint,
6777
to end: CGPoint,
68-
axOrigin: CGPoint
78+
axOrigin: CGPoint,
79+
pins: [AnnotationNote] = []
6980
) -> Outcome {
81+
// A CLICK on a pin edits that note, whatever the tool. Ordered FIRST because
82+
// it is the most specific reading of the press: the user pressed a control
83+
// that is on screen, drawn, and numbered, and every other branch below is
84+
// about the app UNDERNEATH it.
85+
//
86+
// The travel gate is what keeps this from costing frame mode its main
87+
// gesture. A press that travelled is a DRAG, and a drag that began on a pin
88+
// must still draw its frame — pins sit exactly where the user just annotated,
89+
// so they are precisely where the next frame gets drawn from (the `VRT-dp47`
90+
// report, and the reason the pins are hit-test-inert in frame mode at all).
91+
// Gating on travel rather than on tool keeps one sentence true of both: a
92+
// click on a pin edits it, a drag from a pin draws.
93+
//
94+
// COORDINATES: `start` is WINDOW-LOCAL here, which is the space
95+
// ``AnnotationNote/anchorRect`` is stored in — so it is handed to the rule
96+
// UNSHIFTED. The `axOrigin` additions below are what turn the same point into
97+
// the AX screen space the element queries use; adding it here too would look
98+
// symmetrical and test every pin against a point off by the window's screen
99+
// origin (invisible on the primary display, wrong on every other one).
100+
if !travelledFarEnough(from: start, to: end),
101+
let id = PinAttentionRule.pressedNote(atWindowPoint: start, in: pins) {
102+
return .editNote(id)
103+
}
104+
70105
switch tool {
71106
case .point:
72107
// `startLocation`, not `location`: it is where the user AIMED, and it is

0 commit comments

Comments
 (0)