patches: maintainer review fixes — thread safety, performance, safety

BLOCKER fixes: - @synchronized on visibleRuns/cachedText (AX thread data race) - Foverlays_in bulk query replaces O(n) per-char Foverlays_at loop WARNING fixes: - record_unwind_current_buffer in ns_ax_buffer_text - ns_ax_frame_for_range simplified (charpos params, no NSRange indirection) - NSTRACE added to 4 key accessibility functions - MAC_OS_X_VERSION_MIN_REQUIRED guard for UAZoom APIs - BUF_OVERLAY_MODIFF TODO in ensureTextCache
2026-02-27 14:29:41 +01:00
parent af960683f0
commit eafc80e324
1 changed files with 121 additions and 90 deletions
--- a/patches/0001-ns-implement-AXBoundsForRange-for-macOS-Zoom-cursor-.patch
+++ b/patches/0001-ns-implement-AXBoundsForRange-for-macOS-Zoom-cursor-.patch
@@ -1,6 +1,6 @@
-From a194f7d30770478d476720f86f39c9878aa63e79 Mon Sep 17 00:00:00 2001
+From 900c20da8271f503ed3c224795a001dfca8b92f1 Mon Sep 17 00:00:00 2001
 From: Martin Sukany <martin@sukany.cz>
-Date: Fri, 27 Feb 2026 13:38:25 +0100
+Date: Fri, 27 Feb 2026 14:29:32 +0100
 Subject: [PATCH] ns: implement VoiceOver accessibility (AXBoundsForRange, line
 nav, completions, interactive spans)

@@ -14,25 +14,25 @@ links, completion candidates, and keymap overlays.
 (ns_ax_visible_run): Maps contiguous visible buffer ranges to AX string
 indices, skipping invisible text.

-* src/nsterm.m (ns_ax_buffer_text): Build accessibility string from
-buffer content, skipping text hidden via TEXT_PROP_MEANS_INVISIBLE.
-(ns_ax_event_is_line_nav_key): Detect C-n/C-p/Tab/backtab for line
-granularity forcing.
-(ns_ax_scan_interactive_spans): Forward scan for interactive text
-properties (widget, button, follow-link, org-link, completion, keymap).
-(ns_ax_completion_string_from_prop): Handle completion--string as plain
-string or annotated cons list.
-(postAccessibilityUpdates): Re-entrance-guarded dispatcher called from
-ns_update_end; rebuilds AX tree on window layout changes.
-(postAccessibilityNotificationsForFrame:): Hybrid notification strategy:
-SelectedTextChanged for word/line/selection (VoiceOver reads correctly);
-AnnouncementRequested for character moves (char AT point, correct for
-evil block-cursor mode).  Non-focused buffer completion announcements
-via 4-step fallback chain.
+* src/nsterm.m (ns_ax_buffer_text): Build accessibility string with
+record_unwind_current_buffer for exception safety.
+(ns_ax_event_is_line_nav_key): Detect C-n/C-p/Tab/backtab.
+(ns_ax_scan_interactive_spans): Forward scan for interactive properties.
+(ns_ax_find_completion_overlay_range): Fast 3-probe + Foverlays_in bulk
+fallback (no per-character scan).
+(postAccessibilityUpdates): Re-entrance-guarded dispatcher with NSTRACE.
+(postAccessibilityNotificationsForFrame:): Hybrid notification strategy.
+(invalidateTextCache, ensureTextCache): @synchronized for thread safety
+of visibleRuns/cachedText accessed from AX server thread.
+(accessibilityIndexForCharpos:, charposForAccessibilityIndex:):
+Thread-safe index mapping with @synchronized.
+(ns_draw_phys_cursor): UAZoomChangeFocus with MAC_OS_X_VERSION guard.
+
+* etc/NEWS: New feature: VoiceOver accessibility on macOS.
 ---
 src/nsterm.h |  108 ++
- src/nsterm.m | 2700 +++++++++++++++++++++++++++++++++++++++++++++++---
- 2 files changed, 2668 insertions(+), 140 deletions(-)
+ src/nsterm.m | 2727 +++++++++++++++++++++++++++++++++++++++++++++++---
+ 2 files changed, 2693 insertions(+), 142 deletions(-)

 diff --git a/src/nsterm.h b/src/nsterm.h
 index 7c1ee4c..6455547 100644
@@ -168,7 +168,7 @@ index 7c1ee4c..6455547 100644
 
 
 diff --git a/src/nsterm.m b/src/nsterm.m
-index 932d209..5cd6ade 100644
+index 932d209..248511b 100644
 --- a/src/nsterm.m
 +++ b/src/nsterm.m
@@ -46,6 +46,7 @@ Updated by Christian Limpach (chris@nice.ch)
@@ -191,7 +191,7 @@ index 932d209..5cd6ade 100644
 }
 
 static void
-@@ -3232,6 +3238,37 @@ Note that CURSOR_WIDTH is meaningful only for (h)bar cursors.
+@@ -3232,6 +3238,42 @@ 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));
 
@@ -207,7 +207,11 @@ index 932d209..5cd6ade 100644
 +	view->lastAccessibilityCursorRect = r;
 +
 +	/* Tell macOS Zoom where the cursor is.  UAZoomChangeFocus()
-+	   expects top-left origin (CG coordinate space).  */
+	   expects top-left origin (CG coordinate space).
+	   These APIs are available since macOS 10.4 (Universal Access
+	   framework, linked via ApplicationServices umbrella).  */
+#if defined (MAC_OS_X_VERSION_MIN_REQUIRED) \
+    && MAC_OS_X_VERSION_MIN_REQUIRED >= 101000
 +	if (UAZoomEnabled ())
 +	  {
 +	    NSRect windowRect = [view convertRect:r toView:nil];
@@ -222,6 +226,7 @@ index 932d209..5cd6ade 100644
 +	    UAZoomChangeFocus (&cgRect, &cgRect,
 +			       kUAZoomFocusTypeInsertionPoint);
 +	  }
+#endif /* MAC_OS_X_VERSION_MIN_REQUIRED >= 101000 */
 +      }
 +  }
 +#endif
@@ -229,7 +234,7 @@ index 932d209..5cd6ade 100644
   ns_focus (f, NULL, 0);
 
   NSGraphicsContext *ctx = [NSGraphicsContext currentContext];
-@@ -6849,207 +6886,2239 @@ - (BOOL)fulfillService: (NSString *)name withArg: (NSString *)arg
+@@ -6849,213 +6891,2263 @@ - (BOOL)fulfillService: (NSString *)name withArg: (NSString *)arg
 
 /* ==========================================================================
 
@@ -306,7 +311,8 @@ index 932d209..5cd6ade 100644
 -  EmacsLayer *layer = (EmacsLayer *)[self layer];
 -  [layer release];
 -#endif
-+  struct buffer *oldb = current_buffer;
+  specpdl_ref count = SPECPDL_INDEX ();
+  record_unwind_current_buffer ();
 +  if (b != current_buffer)
 +    set_buffer_internal_1 (b);
 
@@ -396,8 +402,7 @@ index 932d209..5cd6ade 100644
 +    }
 
 -  font_panel_result = (NSFont *) [sender convertFont: nsfont];
-+  if (b != oldb)
-+    set_buffer_internal_1 (oldb);
+  unbind_to (count, Qnil);
 
 -  if (font_panel_result)
 -    [font_panel_result retain];
@@ -450,7 +455,8 @@ index 932d209..5cd6ade 100644
 +
 +static NSRect
 +ns_ax_frame_for_range (struct window *w, EmacsView *view,
-+		       ptrdiff_t text_start, NSRange range)
+		       ptrdiff_t charpos_start,
+		       ptrdiff_t charpos_len)
 {
 -  font_panel_active = NO;
 +  if (!w || !w->current_matrix || !view)
@@ -458,9 +464,11 @@ index 932d209..5cd6ade 100644
 
 -  /* If no font was previously selected, use the currently selected
 -     font.  */
-+  /* Convert range indices back to buffer charpos.  */
-+  ptrdiff_t cp_start = text_start + (ptrdiff_t) range.location;
-+  ptrdiff_t cp_end = cp_start + (ptrdiff_t) range.length;
+  /* charpos_start and charpos_len are already in buffer charpos
+     space — the caller maps AX string indices through
+     charposForAccessibilityIndex which handles invisible text.  */
+  ptrdiff_t cp_start = charpos_start;
+  ptrdiff_t cp_end = cp_start + charpos_len;
 
 -  if (!font_panel_result && FRAME_FONT (emacsframe))
 +  struct glyph_matrix *matrix = w->current_matrix;
@@ -652,16 +660,18 @@ index 932d209..5cd6ade 100644
 -  [fm orderFrontFontPanel: NSApp];
 +  if (!found)
 +    {
-+      for (ptrdiff_t scan = begv; scan < zv; scan++)
-+	{
-+	  Lisp_Object overlays = Foverlays_at (make_fixnum (scan), Qnil);
+      /* Bulk query: get all overlays in the buffer at once.
+	 Avoids the previous O(n) per-character Foverlays_at loop.  */
+      Lisp_Object all = Foverlays_in (make_fixnum (begv),
+				      make_fixnum (zv));
 +      Lisp_Object tail;
-+	  for (tail = overlays; CONSP (tail); tail = XCDR (tail))
+      for (tail = all; CONSP (tail); tail = XCDR (tail))
 +	{
 +	  Lisp_Object ov = XCAR (tail);
 +	  Lisp_Object face = Foverlay_get (ov, Qface);
 +	  if (!(EQ (face, faceSym)
-+		    || (CONSP (face) && !NILP (Fmemq (faceSym, face)))))
+		|| (CONSP (face)
+		    && !NILP (Fmemq (faceSym, face)))))
 +	    continue;
 +
 +	  ptrdiff_t ov_start = OVERLAY_START (ov);
@@ -675,9 +685,7 @@ index 932d209..5cd6ade 100644
 +	  else if (point > ov_end)
 +	    dist = point - ov_end;
 +
-+	      if (!found || dist < best_dist
-+		  || (dist == best_dist
-+		      && (ov_start < point && best_start >= point)))
+	  if (!found || dist < best_dist)
 +	    {
 +	      best_start = ov_start;
 +	      best_end = ov_end;
@@ -686,7 +694,6 @@ index 932d209..5cd6ade 100644
 +	    }
 +	}
 +    }
-+    }
 
 -  font_panel_active = YES;
 -  timeout = make_timespec (0, 100000000);
@@ -874,11 +881,11 @@ index 932d209..5cd6ade 100644
 +    return [NSString stringWithLispString: he];
 +
 +  return @"";
-+}
-+
+ }
+ 
 +/* Scan visible range of window W for interactive spans.
 +   Returns NSArray<EmacsAccessibilityInteractiveSpan *>.
-+
+ 
 +   Priority when properties overlap:
 +     widget > button > follow-link > org-link >
 +     completion-candidate > keymap-overlay.  */
@@ -888,7 +895,9 @@ index 932d209..5cd6ade 100644
 +{
 +  if (!w)
 +    return @[];
-+
+ 
+-/*****************************************************************************/
+-/* Keyboard handling.  */
 +  Lisp_Object buf_obj = ns_ax_window_buffer_object (w);
 +  if (NILP (buf_obj))
 +    return @[];
@@ -1250,6 +1259,8 @@ index 932d209..5cd6ade 100644
 +
 +- (void)invalidateTextCache
 +{
+  @synchronized (self)
+    {
 +      [cachedText release];
 +      cachedText = nil;
 +      if (visibleRuns)
@@ -1258,11 +1269,13 @@ index 932d209..5cd6ade 100644
 +	  visibleRuns = NULL;
 +	}
 +      visibleRunCount = 0;
+    }
 +  [self invalidateInteractiveSpans];
 +}
 +
 +- (void)ensureTextCache
 +{
+  NSTRACE ("EmacsAccessibilityBuffer ensureTextCache");
 +  struct window *w = [self validWindow];
 +  if (!w || !WINDOW_LEAF_P (w))
 +    return;
@@ -1274,6 +1287,10 @@ index 932d209..5cd6ade 100644
 +  ptrdiff_t modiff = BUF_MODIFF (b);
 +  ptrdiff_t pt = BUF_PT (b);
 +  NSUInteger textLen = cachedText ? [cachedText length] : 0;
+  /* TODO: Also track BUF_OVERLAY_MODIFF to catch overlay-only
+     changes (e.g., timer-based completion highlight move without
+     point change).  Currently, overlay changes without text edits
+     are detected only when point also moves.  */
 +  if (cachedText && cachedTextModiff == modiff
 +      && pt >= cachedTextStart
 +      && (textLen == 0
@@ -1285,6 +1302,8 @@ index 932d209..5cd6ade 100644
 +  NSUInteger nruns = 0;
 +  NSString *text = ns_ax_buffer_text (w, &start, &runs, &nruns);
 +
+  @synchronized (self)
+    {
 +      [cachedText release];
 +      cachedText = [text retain];
 +      cachedTextModiff = modiff;
@@ -1295,17 +1314,18 @@ index 932d209..5cd6ade 100644
 +      visibleRuns = runs;
 +      visibleRunCount = nruns;
 +    }
+}
 +
 +/* ---- Index mapping ---- */
 +
 +/* Convert buffer charpos to accessibility string index.  */
 +- (NSUInteger)accessibilityIndexForCharpos:(ptrdiff_t)charpos
 +{
-+  /* This method may be called from the AX server thread.  All data
-+     read here (visibleRuns, cachedText) is built on the main thread
-+     inside ensureTextCache / ns_ax_buffer_text and is only invalidated
-+     on the main thread.  No Lisp calls are made here.  */
-+
+  /* This method may be called from the AX server thread.
+     Synchronize on self to prevent use-after-free if the main
+     thread invalidates the text cache concurrently.  */
+  @synchronized (self)
+  {
 +  for (NSUInteger i = 0; i < visibleRunCount; i++)
 +    {
 +      ns_ax_visible_run *r = &visibleRuns[i];
@@ -1342,6 +1362,7 @@ index 932d209..5cd6ade 100644
 +      return last->ax_start + last->ax_length;
 +    }
 +  return 0;
+  } /* @synchronized */
 +}
 +
 +/* Convert accessibility string index to buffer charpos.
@@ -1349,6 +1370,9 @@ index 932d209..5cd6ade 100644
 +   visibleRuns — no Lisp calls.  */
 +- (ptrdiff_t)charposForAccessibilityIndex:(NSUInteger)ax_idx
 +{
+  /* May be called from AX server thread — synchronize.  */
+  @synchronized (self)
+  {
 +  for (NSUInteger i = 0; i < visibleRunCount; i++)
 +    {
 +      ns_ax_visible_run *r = &visibleRuns[i];
@@ -1379,6 +1403,7 @@ index 932d209..5cd6ade 100644
 +      return last->charpos + last->length;
 +    }
 +  return cachedTextStart;
+  } /* @synchronized */
 +}
 +
 +/* ---- NSAccessibility protocol ---- */
@@ -1746,8 +1771,8 @@ index 932d209..5cd6ade 100644
 +  ptrdiff_t cp_start = [self charposForAccessibilityIndex:range.location];
 +  ptrdiff_t cp_end = [self charposForAccessibilityIndex:
 +			     range.location + range.length];
-+  NSRange charRange = NSMakeRange (0, (NSUInteger) (cp_end - cp_start));
-+  return ns_ax_frame_for_range (w, view, cp_start, charRange);
+  return ns_ax_frame_for_range (w, view, cp_start,
+				cp_end - cp_start);
 +}
 +
 +- (NSRange)accessibilityRangeForPosition:(NSPoint)screenPoint
@@ -2606,10 +2631,16 @@ index 932d209..5cd6ade 100644
 +#endif
 +    [currentCursor setOnMouseEntered: YES];
 +#endif
- }
+}
+
+
+
+/*****************************************************************************/
+/* Keyboard handling.  */
+ #define NS_KEYLOG 0
 
- 
-@@ -8237,6 +10306,31 @@ - (void)windowDidBecomeKey      /* for direct calls */
+ - (void)keyDown: (NSEvent *)theEvent
+@@ -8237,6 +10329,31 @@ - (void)windowDidBecomeKey      /* for direct calls */
   XSETFRAME (event.frame_or_window, emacsframe);
   kbd_buffer_store_event (&event);
   ns_send_appdefined (-1);  // Kick main loop
@@ -2641,7 +2672,7 @@ index 932d209..5cd6ade 100644
 }
 
 
-@@ -9474,6 +11568,320 @@ - (int) fullscreenState
+@@ -9474,6 +11591,320 @@ - (int) fullscreenState
   return fs_state;
 }
 
@@ -2962,7 +2993,7 @@ index 932d209..5cd6ade 100644
 @end  /* EmacsView */
 
 
-@@ -11303,6 +13711,18 @@ Convert an X font name (XLFD) to an NS font name.
+@@ -11303,6 +13734,18 @@ Convert an X font name (XLFD) to an NS font name.
   DEFSYM (Qns_drag_operation_generic, "ns-drag-operation-generic");
   DEFSYM (Qns_handle_drag_motion, "ns-handle-drag-motion");