emacs-doom/REVIEW-FINAL.md

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.