From d4952ae9e97601b19e3b2635f02cd3b5b9af72de Mon Sep 17 00:00:00 2001 From: Daneel Date: Thu, 5 Mar 2026 02:08:55 +0100 Subject: [PATCH] patches: pre-upstream review fixes (all blockers + nits) - 0000: fix double blank line before #ifdef, remove extra block scope braces - 0001: spell out 'AT' -> 'assistive technology'; fix stub comment - 0002: revert unrelated em-dash->triple-dash in windowWillResize strings - 0003-0005: renumber from [PATCH N/9] to [PATCH N/8] - 0006: remove 'Block-style cursors' from Known Limitations; fix @xref note - 0007: remove spurious ns_ax_face_is_selected ChangeLog entry - 0008: refactor goto skip_overlay_scan to nested if; remove spurious blank line - All: renumber series (0000=[PATCH 1/1], 0001-0008=[PATCH 1/8]-[PATCH 8/8]) --- FIXES-APPLIED.md | 211 ++++++++++ REVIEW-FINAL.md | 395 ++++++++++++++++++ ...-with-macOS-Zoom-for-cursor-tracking.patch | 17 +- ...cessibility-base-classes-and-helpers.patch | 6 +- ...plement-buffer-accessibility-element.patch | 8 +- ...-notifications-and-mode-line-element.patch | 2 +- ...dd-interactive-span-elements-for-Tab.patch | 2 +- ...ibility-into-EmacsView-and-redisplay.patch | 2 +- ...-VoiceOver-section-to-macOS-appendix.patch | 11 +- ...nce-overlay-completions-to-VoiceOver.patch | 4 +- ...child-frame-completions-to-VoiceOver.patch | 91 ++-- 11 files changed, 657 insertions(+), 92 deletions(-) create mode 100644 FIXES-APPLIED.md create mode 100644 REVIEW-FINAL.md diff --git a/FIXES-APPLIED.md b/FIXES-APPLIED.md new file mode 100644 index 0000000..84f3195 --- /dev/null +++ b/FIXES-APPLIED.md @@ -0,0 +1,211 @@ +# Fixes Applied to Patch Series + +All fixes applied to `/tmp/emacs-doom-review/patches/` on 2026-03-04. + +--- + +## FIX 1 — 0002: Revert em-dash → triple-dash in windowWillResize strings + +**File:** `0002-ns-implement-buffer-accessibility-element.patch` + +The `windowWillResize:` hunk incorrectly changed the window-resize title separator from +em-dash (U+2014, ` — `) to triple-dash (` --- `) in: +- `strstr (t, " — ")` — used to strip the size suffix when resizing begins +- `esprintf (size_title, "%s — (%d × %d)", …)` — used to format the title + +Both `-` lines were converted to context (` ` prefix) and the corresponding `+` lines +were removed. The hunk header counts remained unchanged (13,13) since the net effect +on old/new line counts is zero. + +--- + +## FIX 2 — 0007: Remove spurious ns_ax_face_is_selected from commit message + +**File:** `0007-ns-announce-overlay-completions-to-VoiceOver.patch` + +The ChangeLog entry for `ns_ax_face_is_selected` was removed from the commit message: +``` +* src/nsterm.m (ns_ax_face_is_selected): New static function; matches +'current', 'selected', 'selection' in face symbol names. +``` +This function does not exist in the diff — the actual function is +`ns_face_name_matches_selected_p` (introduced in patch 0000/0001). + +--- + +## FIX 3 — All patches: Renumber series for proper separation + +The patches were renumbered to reflect two independent series: + +| File | Before | After | Series | +|--------|---------------|---------------|-------------------| +| 0000 | [PATCH 1/9] | [PATCH 1/1] | Zoom (standalone) | +| 0001 | [PATCH 2/9] | [PATCH 1/8] | VoiceOver | +| 0002 | [PATCH 3/9] | [PATCH 2/8] | VoiceOver | +| 0003 | [PATCH 4/9] | [PATCH 3/8] | VoiceOver | +| 0004 | [PATCH 5/9] | [PATCH 4/8] | VoiceOver | +| 0005 | [PATCH 6/9] | [PATCH 5/8] | VoiceOver | +| 0006 | [PATCH 7/9] | [PATCH 6/8] | VoiceOver | +| 0007 | [PATCH 8/9] | [PATCH 7/8] | VoiceOver | +| 0008 | [PATCH 9/9] | [PATCH 8/8] | VoiceOver | + +--- + +## FIX 4 — 0008: Refactor goto to nested if + +**File:** `0008-ns-announce-child-frame-completions-to-VoiceOver.patch` + +**Note:** The task description incorrectly named file 0007; the goto is in 0008. + +In `postAccessibilityNotificationsForFrame:`, the overlay completion scan used a +`goto skip_overlay_scan;` pattern. Refactored to a standard nested `if` block: + +Before (in diff): +```c +if (!MINI_WINDOW_P (w) || didTextChange) + goto skip_overlay_scan; + +int selected_line = -1; +NSString *candidate = ns_ax_selected_overlay_text (…); +if (candidate) + { … } + +skip_overlay_scan: +/* --- Cursor moved … */ +``` + +After (in diff): +```c +if (MINI_WINDOW_P (w) && !didTextChange) + { + int selected_line = -1; + NSString *candidate = ns_ax_selected_overlay_text (…); + if (candidate) + { … } + } +/* --- Cursor moved … */ +``` + +The extended comment explaining the `didTextChange` guard was preserved. The inner +body code is restored to its original indentation level. Hunk header updated from +`+9609,49` to `+9609,48` (one fewer net added line). + +--- + +## FIX 5 — 0006: Remove "Block-style cursors" from Known Limitations + +**File:** `0006-doc-add-VoiceOver-section-to-macOS-appendix.patch` + +Removed the `@item` bullet from the Known Limitations `@itemize`: +```texinfo +@item +Block-style cursors are handled correctly: character navigation +announces the character at the cursor position, not the character +before it. +``` +This is a feature, not a limitation. The hunk header was updated from +`+273,82` to `+273,78` (4 fewer added lines). + +--- + +## FIX 6 — 0001: Spell out "AT" in commit message + +**File:** `0001-ns-add-accessibility-base-classes-and-helpers.patch` + +In the ChangeLog entry for `syms_of_nsterm`: + +Before: `set non-nil automatically when an AT is detected at startup.` +After: `set non-nil automatically when an assistive technology (AT) is detected at startup.` + +--- + +## FIX 7 — 0005: Spell out "AT" in commit message + +**File:** `0005-ns-wire-accessibility-into-EmacsView-and-redisplay.patch` + +**Result: No-op.** The commit message of patch 0005 (lines before the `---` separator) +contains no "AT" abbreviation. The "AT" occurrences found by grep are in code comments +within the diff body (not the commit message proper). No changes were made. + +--- + +## FIX 8 — 0006: Remove spurious @xref note from commit message + +**File:** `0006-doc-add-VoiceOver-section-to-macOS-appendix.patch` + +Removed the sentence "Use @xref for cross-reference at sentence start." from the +ChangeLog entry. This referenced an `@xref` that does not appear in the patch. + +Before: +``` +enabled, and known limitations. Use @xref for cross-reference at +sentence start. Correct description of ns-accessibility-enabled +``` +After: +``` +enabled, and known limitations. Correct description of ns-accessibility-enabled +``` + +--- + +## FIX 9 — 0001: Update stub comment reference + +**File:** `0001-ns-add-accessibility-base-classes-and-helpers.patch` + +In the stub `@implementation EmacsAccessibilityBuffer (InteractiveSpans)`: + +Before: `/* Stub: full implementation added in patch 0004. */` +After: `/* Stub: full implementation in the following patch. */` + +This avoids a hard-coded patch number that may shift if the series is reordered. + +--- + +## FIX 10 — 0000: Style nits + +**File:** `0000-ns-integrate-with-macOS-Zoom-for-cursor-tracking.patch` + +### a) Double blank line before `#ifdef NS_IMPL_COCOA` + +In the large inserted block starting at `-1081,6 +1086,N`, an extra blank `+` line +preceded `+#ifdef NS_IMPL_COCOA`, creating a triple blank gap. The extra `+\n` line +was removed. Hunk header updated from `+1086,293` to `+1086,292`. + +### b) Unnecessary block scope in `ns_draw_window_cursor` + +The Zoom integration block used a gratuitous `{ … }` scope wrapper: +```c +{ + EmacsView *view = FRAME_NS_VIEW (f); + if (view && on_p && active_p) + { … } +} +``` +Removed the outer braces and dedented the contents by one level: +```c +EmacsView *view = FRAME_NS_VIEW (f); +if (view && on_p && active_p) + { … } +``` +Hunk header updated from `+3559,45` to `+3559,43` (2 fewer added lines). + +--- + +## FIX 11 — 0008: Remove spurious blank line in announceChildFrameCompletion + +**File:** `0008-ns-announce-child-frame-completions-to-VoiceOver.patch` + +Removed the blank line between the opening `{` and the first comment in the +`announceChildFrameCompletion` method body. Hunk header updated from +`+12991,156` to `+12991,155`. + +--- + +## Verification + +All 9 patch files pass post-fix verification: +- All Subject: lines present with correct [PATCH N/M] numbering +- All @@ hunk headers syntactically valid +- No `goto skip_overlay_scan` or `skip_overlay_scan:` label remaining +- No triple-dash in functional window-resize strings +- No `ns_ax_face_is_selected` in 0007 commit message diff --git a/REVIEW-FINAL.md b/REVIEW-FINAL.md new file mode 100644 index 0000000..c4c3f7b --- /dev/null +++ b/REVIEW-FINAL.md @@ -0,0 +1,395 @@ +The review is written to `/tmp/emacs-doom-review/REVIEW-FINAL.md` (418 lines). Here's the summary of findings: + +--- + +## Review Summary + +**Overall verdict: READY WITH MINOR FIXES** + +The series has matured well through six rounds. Architecture is sound, threading model is correct, performance claims are supportable. Two items block upstream submission: + +### Must fix + +**Patch 0002** — Contains an unrelated functional change: the window-resize title string changes `strstr(t, " — ")` and `esprintf(..., "%s — ...")` from Unicode em-dash to triple-dash `---`. This is a behavioral change to non-accessibility code embedded in an accessibility patch. A maintainer will catch it immediately. + +**Patch 0007** — The commit message claims to introduce `ns_ax_face_is_selected` as a new function, but no such function appears in the diff. The actual code calls `ns_face_name_matches_selected_p` from patch 0000. The ChangeLog entry must be corrected. + +### Structural (must fix before submission) + +**[PATCH 1/9] numbering** — All nine patches are numbered as one series, but the README says 0000 is independent of 0001–0008. For emacs-devel submission, either add a `[PATCH 0/9]` cover letter explaining the split, or submit 0000 separately as `[PATCH 1/1]`. + +### Notable nits (fix recommended) + +- `goto skip_overlay_scan` in patch 0007 — unusual in Emacs C; Stefan Monnier will likely ask for a nested `if` instead +- "Known Limitations" fifth bullet in patch 0006 ("Block-style cursors are handled correctly...") is a feature, not a limitation +- "AT" abbreviation in commit messages — spell out "assistive technology" +- Patch 0006 commit message references an `@xref` that isn't there + +### What's clean (no action needed) + +Patches 0003, 0004, 0005 are LGTM. The threading model (AX thread → dispatch_sync to main), the `@synchronized` cache guards, `block_input`/`unblock_input` specpdl protection, GNUstep exclusion, and performance claims all check out. +p` TTL caching with `CFAbsoluteTimeGetCurrent()` is + well motivated; the 1-second TTL justification is documented clearly. +- `ns_face_name_matches_selected_p` using `strstr` on face symbol names + is an accepted heuristic; the comment acknowledges it. +- specpdl unwind protection is correct throughout. +- `ns_zoom_find_child_frame_candidate`: inside the FOR_EACH_FRAME body, + `block_input()` is called before `SPECPDL_INDEX()`. In practice this + is safe (SPECPDL_INDEX cannot fail), but the canonical Emacs ordering + is SPECPDL_INDEX -> record_unwind -> block_input. Patch 0008 later + reverses this order in `ns_ax_buffer_text` with an explanatory comment; + 0000 is left inconsistent. Nit only. +- Double blank line before `#ifdef NS_IMPL_COCOA` at the top of the new + block (~line 93 of the diff). GNU Emacs style uses a single blank + line between top-level definitions. +- Block scope `{ EmacsView *view = ...; }` in `ns_draw_window_cursor` is + unnecessary (no outer `view` in scope at that point); the extra braces + can be removed. + +**Verdict: LGTM with nits** -- fix numbering before submission; the +code is otherwise clean. + +--- + +### 0001: Add accessibility base classes and helpers [PATCH 2/9] + +**Subject / commit message** +Correct format. The commit message abbreviates "AT" for "assistive +technology" -- spell it out for an emacs-devel audience. + +**Code quality** + +- GC safety analysis on `lispWindow` is thorough and correct. +- `@synchronized (self)` guards on visibleRuns/lineStartOffsets are + correct; they protect against concurrent reads from the AX server + thread while the main thread rebuilds the cache. +- `ns_ax_buffer_text` correctly uses `Fbuffer_substring_no_properties` + instead of raw `BUF_BYTE_ADDRESS` to handle the buffer gap. ✓ +- `Fget_char_property (pos, Qinvisible, Qnil)` with Qnil meaning the + current buffer is correct after `set_buffer_internal_1(b)`. ✓ +- `TEXT_PROP_MEANS_INVISIBLE` correctly mirrors xdisp.c logic. ✓ +- The stub `@implementation EmacsAccessibilityBuffer (InteractiveSpans)` + has a comment reading "full implementation added in patch 0004". Once + the series numbering is resolved this should say "in the following + patch" or match the final [PATCH N/M] numbers. + +**Verdict: LGTM with nits.** + +--- + +### 0002: Implement buffer accessibility element [PATCH 3/9] + +**SERIOUS ISSUE -- unrelated functional change** + +Deep in the diff, in the context of `windowWillResize:toSize:`, the patch +changes: + +```diff +- char *pos = strstr (t, " \xe2\x80\x94 "); ++ char *pos = strstr (t, " --- "); + ... +- esprintf (size_title, "%s \xe2\x80\x94 (%d x %d)", old_title, cols, rows); ++ esprintf (size_title, "%s --- (%d x %d)", old_title, cols, rows); +``` + +This replaces a Unicode em-dash (U+2014) with three ASCII hyphens in the +*functional* window-resize title string. This: + +- Is unrelated to accessibility. +- Changes the visual appearance of the resize title bar. +- Must be in its own commit (with its own ChangeLog entry and + justification) or reverted. A maintainer will reject the patch if + they spot this. + +All other content in this patch is correct: + +- `@synchronized (self)` in `invalidateTextCache` correctly releases + Objective-C objects under the lock; `invalidateInteractiveSpans` is + called outside the lock to avoid mutex inversion. ✓ +- Binary search in `accessibilityIndexForCharpos:` and + `charposForAccessibilityIndex:` is correct; the O(1) ASCII fast-path + is well-motivated and the comment explains why it matters. ✓ +- `setAccessibilitySelectedTextRange:` correctly deactivates the mark + (with explanatory comment about VoiceOver word-boundary hints). ✓ +- `dispatch_async` for setters (no return value needed) vs. + `dispatch_sync` for getters (return value required) is the correct + pattern throughout. ✓ +- `accessibilityStyleRangeForIndex:` is the only getter without its own + main-thread check; it relies on two sub-calls that each dispatch + individually. Correct but two round-trips instead of one. Minor nit. + +**Verdict: Needs work** -- remove the window title em-dash change. + +--- + +### 0003: Add AX notifications and mode-line element [PATCH 4/9] + +**Subject / commit message** +Correct. ChangeLog entries are detailed and accurate. + +**Code quality** + +- `postTextChangedNotification:` single-character fast path (grab the + inserted char for `AXTextChangeValue`) is a useful optimization. +- `postFocusedCursorNotification:direction:granularity:markActive: + oldMarkActive:` granularity reasoning -- omit for char moves to prevent + block-cursor double-speech -- matches documented WebKit behaviour. ✓ +- `postCompletionAnnouncementForBuffer:point:` four-level fallback chain + (completion--string -> mouse-face -> completions-highlight overlay -> + line text) is comprehensive. +- `[ws mutableCopy]` / `[trims release]` in the word-announcement path + is correct MRC. ✓ +- `EmacsAccessibilityModeLine` returns `NSAccessibilityStaticTextRole`; + `accessibilityValue` delegates to `ns_ax_mode_line_text`. Correct. + +**Verdict: LGTM.** + +--- + +### 0004: Add interactive span elements for Tab [PATCH 5/9] + +**Subject / commit message** +Correct. + +**Code quality** + +- `ns_ax_scan_interactive_spans` priority ordering (widget > button > + follow-link > org-link > completion > keymap-overlay) is a reasonable + policy; documented in the function comment. +- Skip-to-next-change logic uses the minimum of per-property change + points: O(properties * log(buffer)) rather than O(chars). ✓ +- `EmacsAccessibilityInteractiveSpan.isAccessibilityFocused` reads + `pb.cachedPoint` (a plain `ptrdiff_t`) without `@synchronized`. This + is safe because `cachedPoint` is only written on the main thread and + the ivar is machine-word aligned (naturally atomic on ARM64/x86-64). + Acceptable; a brief comment noting this would be welcome. +- `setAccessibilityFocused:` correctly uses `dispatch_async` with the + Lisp_Object-captured-by-value comment. ✓ +- Removes the stub `@implementation` from patch 0001 cleanly. ✓ + +**Verdict: LGTM.** + +--- + +### 0005: Wire accessibility into EmacsView and redisplay [PATCH 6/9] + +**Subject / commit message** +Correct. + +**Code quality** + +- Re-entrance guard (`accessibilityUpdating`) is placed correctly at the + start of `postAccessibilityUpdates`. ✓ +- `ns_update_accessibility_state` uses `AXIsProcessTrustedWithOptions` + with `kAXTrustedCheckOptionPrompt: @NO` -- does not prompt the user. ✓ +- `(__bridge id)` and `(__bridge CFDictionaryRef)` casts in MRC context + are correct for toll-free bridging (no ownership transfer). ✓ +- `com.apple.accessibility.api` distributed notification is the standard + (if undocumented) mechanism used by WebKit et al. ✓ +- Window-switch detection via `lastSelectedWindow` comparison is clean. +- `accessibilityBoundsForRange:` / `accessibilityFrameForRange:` / the + legacy `accessibilityAttributeValue:forParameter:` delegation chain is + complete and handles both modern and pre-10.10 AX API callers. ✓ +- `postAccessibilityUpdates` calls `rebuildAccessibilityTree` before the + per-element notification loop when the tree is stale, with a clear + comment explaining why (cannot diff state after fresh elements are + created). ✓ + +**Verdict: LGTM.** + +--- + +### 0006: doc: Add VoiceOver section to macOS appendix [PATCH 7/9] + +**Texinfo structure** + +- `@node VoiceOver Accessibility` inserted between `Mac / GNUstep Events` + and `GNUstep Support`. Menu entry correctly added. ✓ +- `@cindex`, `@vindex`, `@itemize @bullet`, `@subheading` all used + correctly. ✓ +- `@section` title "VoiceOver Accessibility (macOS)" is appropriate for + the macOS appendix. + +**Content issue** + +In the "Known Limitations" `@itemize`, the fifth bullet reads: + + Block-style cursors are handled correctly: character navigation + announces the character at the cursor position, not the character + before it. + +This is not a limitation -- it is a correctness feature. It does not +belong in a "Known Limitations" section. Remove this bullet or move it +to the main feature description above. + +**Commit message note** + +The commit message says "Use @xref for cross-reference at sentence start" +but no `@xref` appears in the added Texinfo. The note is stale from a +previous revision; remove it or add the intended @xref. + +**Verdict: LGTM with nits** -- fix the Known Limitations bullet; clean up +the @xref note in the commit message. + +--- + +### 0007: Announce overlay completions to VoiceOver [PATCH 8/9] + +**SERIOUS ISSUE -- commit message describes non-existent function** + +The commit message states: + + * src/nsterm.m (ns_ax_face_is_selected): New static function; matches + 'current', 'selected', 'selection' in face symbol names. + +No function named `ns_ax_face_is_selected` appears anywhere in this +patch. The actual code calls `ns_face_name_matches_selected_p`, which +was introduced in patch 0000. The commit message must be corrected to +remove this spurious ChangeLog entry. A maintainer applying the patch +would immediately notice the claimed function does not appear in the diff. + +**Code quality** + +- `ns_ax_selected_overlay_text` uses `SDATA` for the newline scan and + correctly notes the data pointer is used only before Lisp calls that + could trigger GC. ✓ +- Stack-allocated `line_starts[512]` / `line_ends[512]` is safe given + the vertico-count upper bound and the 512-line cap. ✓ +- `BUF_CHARS_MODIFF` (not `BUF_MODIFF`) now gates `ValueChanged`; the + reason (text-property-only changes from font-lock / Vertico prompt + bump BUF_MODIFF but not BUF_CHARS_MODIFF) is well-explained. ✓ +- Separate `if` for `BUF_OVERLAY_MODIFF` (not `else if`) correctly + handles frameworks that bump both counters in one command cycle. ✓ +- Removing `ensureTextCache` from the cursor-moved branch prevents the + O(visible-buffer-text) rebuild cost on every keystroke. ✓ +- `goto skip_overlay_scan` is unusual in Emacs C code. GNU Emacs does + use goto for error exits, but using it to skip over a block is more + readable as a nested `if (!MINI_WINDOW_P (w) && !didTextChange)`. + Stefan Monnier is likely to request a refactor here. +- `block_input` / `record_unwind_protect_void` ordering in + `ns_ax_buffer_text` is corrected and documented. ✓ +- Em-dash to `---` conversions in *comments* are cosmetic cleanup; + acceptable (unlike the functional string changes in patch 0002). + +**Verdict: Needs work** -- fix the commit message; consider refactoring +the goto. + +--- + +### 0008: Announce child frame completions to VoiceOver [PATCH 9/9] + +**Subject / commit message** +Correct and detailed. ChangeLog entries accurately describe all changes. + +**Code quality** + +- `voiceoverSetPoint` flag design (set in `setAccessibilitySelectedText + Range:`, consumed and reset in the next notification cycle) is correct + and clean. ✓ +- `singleLineMove` adjacency detection (NSRange boundary comparison) is + package-agnostic; no evil/doom/spacemacs-specific code. ✓ +- `childFrameLastBuffer` stored as `BVAR(b, name)` (an interned symbol, + always reachable from obarray) rather than a raw buffer pointer is the + correct GC-safe approach. The comment explaining this is clear. ✓ +- `childFrameLastCandidate` as a `char *` with `xstrdup`/`xfree` is + correct C memory management; freed in `dealloc`. ✓ +- `postEchoAreaAnnouncementIfNeeded` reads `echo_area_buffer[0]` directly + with a comment explaining why `with_echo_area_buffer` is not used. + The `minibuf_level == 0` guard is correct. ✓ +- Re-entrance guard comment correctly notes `announceChildFrameCompletion` + makes Lisp calls that can trigger redisplay; the guard is placed before + the call. ✓ +- `childFrameCompletionActive` + child-frame-still-visible check for + restoring VoiceOver focus after popup close is clean. ✓ +- `postFocusedCursorNotification:` updated to omit direction/granularity + for discontiguous jumps (let VoiceOver determine what to read) matches + the new reasoning about org-agenda blank-line gaps. ✓ +- NEWS entry upgraded from `---` to `+++` now that documentation exists + (added in patch 0006). ✓ +- Minor: `announceChildFrameCompletion` has a spurious blank line between + the opening `{` and the first comment. Cosmetic only. + +**Verdict: LGTM with nits.** + +--- + +## Cross-Cutting Issues + +### 1. Patch numbering inconsistency (structural -- must fix) + +All nine patch files carry [PATCH 1/9]-[PATCH 9/9], implying a single +series. The README documents 0000 and 0001-0008 as *independent*. For +upstream submission via git-send-email, either: + +- Treat all nine as one series with a [PATCH 0/9] cover letter; or +- Separate 0000 into its own submission ([PATCH 1/1]) and renumber the + VoiceOver series [PATCH 1/8]-[PATCH 8/8]. + +### 2. Em-dash conversion in functional code (serious -- must fix in 0002) + +Em-dash to triple-dash conversions in *comments* (patches 0007, 0008) +are acceptable style cleanup. The same conversion in *functional string +literals* in patch 0002 (`strstr` / `esprintf` window title code) is a +behavioral change that does not belong in an accessibility patch. + +### 3. block_input ordering inconsistency (nit) + +Patches 0001-0006 use `SPECPDL_INDEX -> record_unwind -> block_input`. +Patch 0008 (and a targeted fix in 0007) adopt `block_input -> record_unwind` +and document why. Both orderings are safe in practice, but the series +leaves them inconsistent across the added functions. + +### 4. "AT" abbreviation in commit messages (nit) + +Patches 0001 and 0005 use "AT" for "assistive technology" in commit +messages without expanding it. Spell it out on first use. + +### 5. Stub comment reference to "patch 0004" (nit) + +The stub `@implementation EmacsAccessibilityBuffer (InteractiveSpans)` in +patch 0001 says "full implementation added in patch 0004". Once the +series numbering is finalised, update this to match the actual [PATCH N] +number, or change it to "the following patch." + +### 6. Threading model assessment + +The dispatch_sync-to-main pattern for all AX getter methods is correct +and matches WebKit's approach. The dispatch_async-for-setters pattern +is correct (no return value needed). The @synchronized blocks on the +shared cache are correctly scoped. The re-entrance guard in +`postAccessibilityUpdates` is correctly placed. No threading issues +found. + +### 7. GNUstep exclusion + +All new code is within `#ifdef NS_IMPL_COCOA` guards. GNUstep is +correctly excluded. ✓ + +### 8. Performance claims + +- O(log n) index lookups via binary search on visible runs: correct. ✓ +- O(log L) line queries via precomputed lineStartOffsets: correct. ✓ +- BUF_MODIFF gating (not BUF_CHARS_MODIFF) for cache validity: correct + and thoroughly justified. ✓ +- Zero overhead when `ns_accessibility_enabled` is nil: verified by + TESTING.txt item 14. ✓ + +--- + +## Verdict + +| Patch | Files | Status | +|-------|-------|--------| +| 0000 | nsterm.h, nsterm.m, NEWS | **LGTM with nits** -- fix numbering; minor style nits | +| 0001 | nsterm.h, nsterm.m | **LGTM with nits** -- spell out "AT"; update stub comment | +| 0002 | nsterm.m | **Needs work** -- remove em-dash/triple-dash change from window-resize string | +| 0003 | nsterm.m | **LGTM** | +| 0004 | nsterm.m | **LGTM** | +| 0005 | nsterm.m, NEWS | **LGTM** | +| 0006 | macos.texi, nsterm.m | **LGTM with nits** -- fix Known Limitations bullet; clarify @xref note | +| 0007 | nsterm.h, nsterm.m | **Needs work** -- fix commit message (remove spurious ns_ax_face_is_selected entry); consider refactoring goto | +| 0008 | nsterm.h, nsterm.m, macos.texi, NEWS | **LGTM with nits** -- remove spurious blank line in method body | + +**Bottom line**: Two patches need targeted fixes before submission (0002: +remove unrelated string change; 0007: correct commit message). After +those fixes, the series is ready for emacs-devel. diff --git a/patches/0000-ns-integrate-with-macOS-Zoom-for-cursor-tracking.patch b/patches/0000-ns-integrate-with-macOS-Zoom-for-cursor-tracking.patch index ebb6884..cdd9849 100644 --- a/patches/0000-ns-integrate-with-macOS-Zoom-for-cursor-tracking.patch +++ b/patches/0000-ns-integrate-with-macOS-Zoom-for-cursor-tracking.patch @@ -1,7 +1,7 @@ From 2bce9ba4ad500eabad619e684ba319b58f9b1fca Mon Sep 17 00:00:00 2001 From: Martin Sukany Date: Wed, 4 Mar 2026 15:23:53 +0100 -Subject: [PATCH 1/9] ns: integrate with macOS Zoom for cursor tracking +Subject: [PATCH 1/1] ns: integrate with macOS Zoom for cursor tracking Inform macOS Zoom of the text cursor position so the zoomed viewport follows keyboard focus in Emacs. Also track completion candidates so @@ -86,11 +86,10 @@ index 932d209f56..6333a7253a 100644 #endif static EmacsMenu *dockMenu; -@@ -1081,6 +1086,293 @@ static NSRect constrain_frame_rect(NSRect frameRect, bool isFullscreen) +@@ -1081,6 +1086,292 @@ static NSRect constrain_frame_rect(NSRect frameRect, bool isFullscreen) } -+ +#ifdef NS_IMPL_COCOA +#if defined (MAC_OS_X_VERSION_MIN_REQUIRED) \ + && MAC_OS_X_VERSION_MIN_REQUIRED >= 101000 @@ -422,7 +421,7 @@ index 932d209f56..6333a7253a 100644 } static void -@@ -3232,6 +3559,45 @@ Note that CURSOR_WIDTH is meaningful only for (h)bar cursors. +@@ -3232,6 +3559,43 @@ Note that CURSOR_WIDTH is meaningful only for (h)bar cursors. /* Prevent the cursor from being drawn outside the text area. */ r = NSIntersectionRect (r, ns_row_rect (w, glyph_row, TEXT_AREA)); @@ -436,10 +435,9 @@ index 932d209f56..6333a7253a 100644 + -> NSWindow (convertRect:toView:nil) + -> NSScreen (convertRectToScreen:) + -> CGRect with y-flip for CoreGraphics top-left origin. */ -+ { -+ EmacsView *view = FRAME_NS_VIEW (f); -+ if (view && on_p && active_p) -+ { ++ EmacsView *view = FRAME_NS_VIEW (f); ++ if (view && on_p && active_p) ++ { + view->lastCursorRect = r; + view->zoomCursorUpdated = YES; + @@ -461,8 +459,7 @@ index 932d209f56..6333a7253a 100644 + kUAZoomFocusTypeInsertionPoint); + } +#endif -+ } -+ } ++ } +#endif /* NS_IMPL_COCOA */ + ns_focus (f, NULL, 0); diff --git a/patches/0001-ns-add-accessibility-base-classes-and-helpers.patch b/patches/0001-ns-add-accessibility-base-classes-and-helpers.patch index a684a4d..9f47fce 100644 --- a/patches/0001-ns-add-accessibility-base-classes-and-helpers.patch +++ b/patches/0001-ns-add-accessibility-base-classes-and-helpers.patch @@ -1,7 +1,7 @@ From 573beced02b3f9b70ba82694d8e4790cfeee9563 Mon Sep 17 00:00:00 2001 From: Martin Sukany Date: Wed, 4 Mar 2026 15:23:53 +0100 -Subject: [PATCH 2/9] ns: add accessibility base classes and helpers +Subject: [PATCH 1/8] ns: add accessibility base classes and helpers Add the foundation for macOS VoiceOver accessibility in the NS (Cocoa) port. No existing code paths are modified. @@ -26,7 +26,7 @@ rect via glyph matrix. (EmacsAccessibilityElement): Implement base class. (syms_of_nsterm): Register accessibility DEFSYMs. Add DEFVAR_BOOL ns-accessibility-enabled with corrected doc: initial value is nil, -set non-nil automatically when an AT is detected at startup. +set non-nil automatically when an assistive technology (AT) is detected at startup. --- src/nsterm.h | 131 +++++++++++++++ src/nsterm.m | 466 +++++++++++++++++++++++++++++++++++++++++++++++++++ @@ -633,7 +633,7 @@ index 6333a7253a..9c53001e37 100644 + +- (void)invalidateInteractiveSpans +{ -+ /* Stub: full implementation added in patch 0004. */ ++ /* Stub: full implementation in the following patch. */ +} + +@end diff --git a/patches/0002-ns-implement-buffer-accessibility-element.patch b/patches/0002-ns-implement-buffer-accessibility-element.patch index ee1e772..752f046 100644 --- a/patches/0002-ns-implement-buffer-accessibility-element.patch +++ b/patches/0002-ns-implement-buffer-accessibility-element.patch @@ -1,7 +1,7 @@ From 64859d37421bdaabe2ec416285b6f1847da0737c Mon Sep 17 00:00:00 2001 From: Martin Sukany Date: Wed, 4 Mar 2026 15:23:54 +0100 -Subject: [PATCH 3/9] ns: implement buffer accessibility element +Subject: [PATCH 2/8] ns: implement buffer accessibility element Implement the NSAccessibility text protocol for Emacs buffer windows. @@ -1176,15 +1176,13 @@ index 9c53001e37..e4b3fb17a0 100644 if (old_title == 0) { char *t = strdup ([[[self window] title] UTF8String]); -- char *pos = strstr (t, " — "); -+ char *pos = strstr (t, " --- "); + char *pos = strstr (t, " — "); if (pos) *pos = '\0'; old_title = t; } size_title = xmalloc (strlen (old_title) + 40); -- esprintf (size_title, "%s — (%d × %d)", old_title, cols, rows); -+ esprintf (size_title, "%s --- (%d × %d)", old_title, cols, rows); + esprintf (size_title, "%s — (%d × %d)", old_title, cols, rows); [window setTitle: [NSString stringWithUTF8String: size_title]]; [window display]; xfree (size_title); diff --git a/patches/0003-ns-add-AX-notifications-and-mode-line-element.patch b/patches/0003-ns-add-AX-notifications-and-mode-line-element.patch index 2379a20..64be064 100644 --- a/patches/0003-ns-add-AX-notifications-and-mode-line-element.patch +++ b/patches/0003-ns-add-AX-notifications-and-mode-line-element.patch @@ -1,7 +1,7 @@ From 11aa323081fd9117381413b6d8e477659d6afc29 Mon Sep 17 00:00:00 2001 From: Martin Sukany Date: Wed, 4 Mar 2026 15:23:55 +0100 -Subject: [PATCH 4/9] ns: add AX notifications and mode-line element +Subject: [PATCH 3/8] ns: add AX notifications and mode-line element Add VoiceOver notification dispatch and mode-line readout. diff --git a/patches/0004-ns-add-interactive-span-elements-for-Tab.patch b/patches/0004-ns-add-interactive-span-elements-for-Tab.patch index d04022b..78513e5 100644 --- a/patches/0004-ns-add-interactive-span-elements-for-Tab.patch +++ b/patches/0004-ns-add-interactive-span-elements-for-Tab.patch @@ -1,7 +1,7 @@ From 9d99df6f95ac2011a1a9f3de448f2f0ec4c27145 Mon Sep 17 00:00:00 2001 From: Martin Sukany Date: Wed, 4 Mar 2026 15:23:55 +0100 -Subject: [PATCH 5/9] ns: add interactive span elements for Tab +Subject: [PATCH 4/8] ns: add interactive span elements for Tab * src/nsterm.m (ns_ax_scan_interactive_spans): New function; scans the visible portion of a buffer for interactive text properties diff --git a/patches/0005-ns-wire-accessibility-into-EmacsView-and-redisplay.patch b/patches/0005-ns-wire-accessibility-into-EmacsView-and-redisplay.patch index 6d0ed49..42953f1 100644 --- a/patches/0005-ns-wire-accessibility-into-EmacsView-and-redisplay.patch +++ b/patches/0005-ns-wire-accessibility-into-EmacsView-and-redisplay.patch @@ -1,7 +1,7 @@ From cba3eda4d3c50dcd77c73353c6a8d2713bfcb8ae Mon Sep 17 00:00:00 2001 From: Martin Sukany Date: Wed, 4 Mar 2026 15:23:55 +0100 -Subject: [PATCH 6/9] ns: wire accessibility into EmacsView and redisplay +Subject: [PATCH 5/8] ns: wire accessibility into EmacsView and redisplay Wire the accessibility element tree into EmacsView and hook it into the redisplay cycle. diff --git a/patches/0006-doc-add-VoiceOver-section-to-macOS-appendix.patch b/patches/0006-doc-add-VoiceOver-section-to-macOS-appendix.patch index 3a9cf24..886e261 100644 --- a/patches/0006-doc-add-VoiceOver-section-to-macOS-appendix.patch +++ b/patches/0006-doc-add-VoiceOver-section-to-macOS-appendix.patch @@ -1,13 +1,12 @@ From a1ea69133e5a5c04076901dce47cd54683c4ca2e Mon Sep 17 00:00:00 2001 From: Martin Sukany Date: Wed, 4 Mar 2026 15:23:55 +0100 -Subject: [PATCH 7/9] doc: add VoiceOver section to macOS appendix +Subject: [PATCH 6/8] doc: add VoiceOver section to macOS appendix * doc/emacs/macos.texi (VoiceOver Accessibility): New node between 'Mac / GNUstep Events' and 'GNUstep Support'. Document screen reader usage, keyboard navigation, completion announcements, ns-accessibility- -enabled, and known limitations. Use @xref for cross-reference at -sentence start. Correct description of ns-accessibility-enabled +enabled, and known limitations. Correct description of ns-accessibility-enabled default: initial value is nil, set automatically at startup. --- doc/emacs/macos.texi | 77 ++++++++++++++++++++++++++++++++++++++++++++ @@ -26,7 +25,7 @@ index 6bd334f48e..72ac3a9aa9 100644 * GNUstep Support:: Details on status of GNUstep support. @end menu -@@ -272,6 +273,82 @@ and return the result as a string. You can also use the Lisp function +@@ -272,6 +273,78 @@ and return the result as a string. You can also use the Lisp function services and receive the results back. Note that you may need to restart Emacs to access newly-available services. @@ -96,10 +95,6 @@ index 6bd334f48e..72ac3a9aa9 100644 +Right-to-left (bidi) text is exposed correctly as buffer content, +but @code{accessibilityRangeForPosition} hit-testing assumes +left-to-right glyph layout. -+@item -+Block-style cursors are handled correctly: character navigation -+announces the character at the cursor position, not the character -+before it. +@end itemize + + This support is available only on the Cocoa build. GNUstep has a diff --git a/patches/0007-ns-announce-overlay-completions-to-VoiceOver.patch b/patches/0007-ns-announce-overlay-completions-to-VoiceOver.patch index 093040a..afaa561 100644 --- a/patches/0007-ns-announce-overlay-completions-to-VoiceOver.patch +++ b/patches/0007-ns-announce-overlay-completions-to-VoiceOver.patch @@ -1,14 +1,12 @@ From b6bc1d102334e32dbc3e284d9e65b0f304c3e694 Mon Sep 17 00:00:00 2001 From: Martin Sukany Date: Wed, 4 Mar 2026 15:23:56 +0100 -Subject: [PATCH 8/9] ns: announce overlay completions to VoiceOver +Subject: [PATCH 7/8] ns: announce overlay completions to VoiceOver Completion frameworks such as Vertico, Ivy, and Icomplete render candidates via overlay before-string/after-string properties. Without this change VoiceOver cannot read overlay-based completion UIs. -* src/nsterm.m (ns_ax_face_is_selected): New static function; matches -'current', 'selected', 'selection' in face symbol names. (ns_ax_selected_overlay_text): New function; scan overlay strings in the window for a line with a selected face; return its text. (ensureTextCache): Switch cache-validity counter from BUF_CHARS_MODIFF diff --git a/patches/0008-ns-announce-child-frame-completions-to-VoiceOver.patch b/patches/0008-ns-announce-child-frame-completions-to-VoiceOver.patch index ab4f005..601d5ba 100644 --- a/patches/0008-ns-announce-child-frame-completions-to-VoiceOver.patch +++ b/patches/0008-ns-announce-child-frame-completions-to-VoiceOver.patch @@ -1,7 +1,7 @@ From a6b1def9a83e23cc0e3325d795ac22b46eb16a5d Mon Sep 17 00:00:00 2001 From: Martin Sukany Date: Wed, 4 Mar 2026 15:23:56 +0100 -Subject: [PATCH 9/9] ns: announce child frame completions to VoiceOver +Subject: [PATCH 8/8] ns: announce child frame completions to VoiceOver Child frame popups (Corfu, Company-mode child frames) render completion candidates in a separate frame whose buffer is not accessible via the @@ -438,7 +438,7 @@ index 209b8a0a1d..13696786ab 100644 } } -@@ -9437,41 +9609,49 @@ frameworks like Vertico bump BOTH BUF_MODIFF (via text property +@@ -9437,41 +9609,48 @@ frameworks like Vertico bump BOTH BUF_MODIFF (via text property displayed in the minibuffer. In normal editing buffers, font-lock and other modes change BUF_OVERLAY_MODIFF on every redisplay, triggering O(overlays) work per keystroke. @@ -452,68 +452,40 @@ index 209b8a0a1d..13696786ab 100644 + echo produced by postTextChangedNotification, making typed + characters inaudible. VoiceOver should read the overlay + candidate only when the user navigates (C-n/C-p), not types. */ -+ if (!MINI_WINDOW_P (w) || didTextChange) -+ goto skip_overlay_scan; -+ -+ int selected_line = -1; -+ NSString *candidate -+ = ns_ax_selected_overlay_text (b, BUF_BEGV (b), BUF_ZV (b), -+ &selected_line); -+ if (candidate) ++ if (MINI_WINDOW_P (w) && !didTextChange) { -- int selected_line = -1; -- NSString *candidate -- = ns_ax_selected_overlay_text (b, BUF_BEGV (b), BUF_ZV (b), -- &selected_line); -- if (candidate) -+ /* Deduplicate: only announce when the candidate changed. */ -+ if (![candidate isEqualToString: -+ self.cachedCompletionAnnouncement]) + int selected_line = -1; + NSString *candidate + = ns_ax_selected_overlay_text (b, BUF_BEGV (b), BUF_ZV (b), + &selected_line); + if (candidate) { -- /* Deduplicate: only announce when the candidate changed. */ -- if (![candidate isEqualToString: -- self.cachedCompletionAnnouncement]) -- { -- self.cachedCompletionAnnouncement = candidate; -- -- /* Announce the candidate text directly via NSApp. -- Do NOT post SelectedTextChanged --- that would cause -- VoiceOver to read the AX text at the cursor position -- (the minibuffer input line), not the overlay candidate. -- AnnouncementRequested with High priority interrupts -- any current speech and announces our text. */ -- NSDictionary *annInfo = @{ -- NSAccessibilityAnnouncementKey: candidate, -- NSAccessibilityPriorityKey: -- @(NSAccessibilityPriorityHigh) -- }; -- ns_ax_post_notification_with_info ( -- NSApp, -- NSAccessibilityAnnouncementRequestedNotification, -- annInfo); -- } -+ self.cachedCompletionAnnouncement = candidate; -+ -+ /* Announce the candidate text directly via NSApp. -+ Do NOT post SelectedTextChanged --- that would cause -+ VoiceOver to read the AX text at the cursor position -+ (the minibuffer input line), not the overlay candidate. -+ AnnouncementRequested with High priority interrupts -+ any current speech and announces our text. */ -+ NSDictionary *annInfo = @{ -+ NSAccessibilityAnnouncementKey: candidate, -+ NSAccessibilityPriorityKey: -+ @(NSAccessibilityPriorityHigh) -+ }; -+ ns_ax_post_notification_with_info ( -+ NSApp, -+ NSAccessibilityAnnouncementRequestedNotification, -+ annInfo); + /* Deduplicate: only announce when the candidate changed. */ + if (![candidate isEqualToString: + self.cachedCompletionAnnouncement]) + { + self.cachedCompletionAnnouncement = candidate; + + /* Announce the candidate text directly via NSApp. + Do NOT post SelectedTextChanged --- that would cause + VoiceOver to read the AX text at the cursor position + (the minibuffer input line), not the overlay candidate. + AnnouncementRequested with High priority interrupts + any current speech and announces our text. */ + NSDictionary *annInfo = @{ + NSAccessibilityAnnouncementKey: candidate, + NSAccessibilityPriorityKey: + @(NSAccessibilityPriorityHigh) + }; + ns_ax_post_notification_with_info ( + NSApp, + NSAccessibilityAnnouncementRequestedNotification, + annInfo); + } } } } -+ skip_overlay_scan: /* --- Cursor moved or selection changed --- Independent check from the overlay branch above. */ if (point != self.cachedPoint || markActive != self.cachedMarkActive) @@ -652,7 +624,7 @@ index 209b8a0a1d..13696786ab 100644 scrollbarsNeedingUpdate = 0; fs_state = FULLSCREEN_NONE; fs_before_fs = next_maximized = -1; -@@ -12789,6 +12991,156 @@ - (id)accessibilityFocusedUIElement +@@ -12789,6 +12991,155 @@ - (id)accessibilityFocusedUIElement The existing elements carry cached state (modiff, point) from the previous redisplay cycle. Rebuilding first would create fresh elements with current values, making change detection impossible. */ @@ -720,7 +692,6 @@ index 209b8a0a1d..13696786ab 100644 + in the minibuffer. */ +- (void)announceChildFrameCompletion +{ -+ + /* Validate frame state --- child frames may be partially + initialized during creation. */ + if (!WINDOWP (emacsframe->selected_window))