patches: review fixes — specpdl protection, overlay_modiff tracking, binary search, enum cleanup

M1: accessibilityRangeForPosition uses specpdl unwind protection for
    block_input/unblock_input (consistent with all other methods).
M2: Track BUF_OVERLAY_MODIFF in ensureTextCache — overlay-only changes
    (timer-based completion highlight) now invalidate the text cache.
M3: Detect narrowing/widening by comparing cachedTextStart vs BUF_BEGV.
m1: Binary search (O(log n)) for visible runs in both
    accessibilityIndexForCharpos and charposForAccessibilityIndex.
m3: Add EmacsAXSpanTypeNone = -1 to enum instead of (EmacsAXSpanType)-1 cast.
m5: Add TODO comment in ns_ax_mode_line_text about non-CHAR_GLYPH limitation.
README: Remove resolved overlay_modiff limitation, document binary search
    and narrowing detection, update architecture section.
This commit is contained in:
2026-02-27 16:56:05 +01:00
parent 765725aaef
commit 65c799dc3f
2 changed files with 75 additions and 49 deletions

View File

@@ -88,7 +88,7 @@ Lisp calls, read only immutable NSString and scalar cache).
etc/NEWS | 11 + etc/NEWS | 11 +
src/nsterm.h | 108 ++ src/nsterm.h | 108 ++
src/nsterm.m | 2870 +++++++++++++++++++++++++++++++++++++++++++++++--- src/nsterm.m | 2870 +++++++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 2960 insertions(+), 149 deletions(-) 3 files changed, 2987 insertions(+), 149 deletions(-)
diff --git a/etc/NEWS b/etc/NEWS diff --git a/etc/NEWS b/etc/NEWS
index 7367e3cc..0e4480ad 100644 index 7367e3cc..0e4480ad 100644
@@ -160,6 +160,7 @@ index 7c1ee4cf..542e7d59 100644
+} +}
+@property (nonatomic, retain) NSString *cachedText; +@property (nonatomic, retain) NSString *cachedText;
+@property (nonatomic, assign) ptrdiff_t cachedTextModiff; +@property (nonatomic, assign) ptrdiff_t cachedTextModiff;
+@property (nonatomic, assign) ptrdiff_t cachedOverlayModiff;
+@property (nonatomic, assign) ptrdiff_t cachedTextStart; +@property (nonatomic, assign) ptrdiff_t cachedTextStart;
+@property (nonatomic, assign) ptrdiff_t cachedModiff; +@property (nonatomic, assign) ptrdiff_t cachedModiff;
+@property (nonatomic, assign) ptrdiff_t cachedPoint; +@property (nonatomic, assign) ptrdiff_t cachedPoint;
@@ -181,6 +182,7 @@ index 7c1ee4cf..542e7d59 100644
+/* Span types for interactive AX child elements. */ +/* Span types for interactive AX child elements. */
+typedef NS_ENUM (NSInteger, EmacsAXSpanType) +typedef NS_ENUM (NSInteger, EmacsAXSpanType)
+{ +{
+ EmacsAXSpanTypeNone = -1,
+ EmacsAXSpanTypeButton = 0, + EmacsAXSpanTypeButton = 0,
+ EmacsAXSpanTypeLink = 1, + EmacsAXSpanTypeLink = 1,
+ EmacsAXSpanTypeCompletionItem = 2, + EmacsAXSpanTypeCompletionItem = 2,
@@ -496,6 +498,10 @@ index 932d209f..ea2de6f2 100644
+ +
+/* ---- Helper: extract mode line text from glyph rows ---- */ +/* ---- Helper: extract mode line text from glyph rows ---- */
+ +
+/* TODO: Only CHAR_GLYPH characters (>= 32) are extracted. Image
+ glyphs, stretch glyphs, and composed glyphs are silently skipped.
+ Mode lines using icon fonts (e.g. doom-modeline with nerd-font)
+ will produce incomplete accessibility text. */
+static NSString * +static NSString *
+ns_ax_mode_line_text (struct window *w) +ns_ax_mode_line_text (struct window *w)
{ {
@@ -1018,7 +1024,7 @@ index 932d209f..ea2de6f2 100644
+ while (pos < vis_end) + while (pos < vis_end)
+ { + {
+ Lisp_Object plist = Ftext_properties_at (make_fixnum (pos), buf_obj); + Lisp_Object plist = Ftext_properties_at (make_fixnum (pos), buf_obj);
+ EmacsAXSpanType span_type = (EmacsAXSpanType) -1; + EmacsAXSpanType span_type = EmacsAXSpanTypeNone;
+ Lisp_Object limit_prop = Qnil; + Lisp_Object limit_prop = Qnil;
+ +
+ if (!NILP (Fplist_get (plist, Qns_ax_widget, Qnil))) + if (!NILP (Fplist_get (plist, Qns_ax_widget, Qnil)))
@@ -1070,7 +1076,7 @@ index 932d209f..ea2de6f2 100644
+ } + }
+ } + }
+ +
+ if ((NSInteger) span_type == -1) + if (span_type == EmacsAXSpanTypeNone)
+ { + {
+ /* Skip to the next position where any interactive property + /* Skip to the next position where any interactive property
+ changes. Try each scannable property in turn and take + changes. Try each scannable property in turn and take
@@ -1379,6 +1385,7 @@ index 932d209f..ea2de6f2 100644
+@implementation EmacsAccessibilityBuffer +@implementation EmacsAccessibilityBuffer
+@synthesize cachedText; +@synthesize cachedText;
+@synthesize cachedTextModiff; +@synthesize cachedTextModiff;
+@synthesize cachedOverlayModiff;
+@synthesize cachedTextStart; +@synthesize cachedTextStart;
+@synthesize cachedModiff; +@synthesize cachedModiff;
+@synthesize cachedPoint; +@synthesize cachedPoint;
@@ -1433,13 +1440,17 @@ index 932d209f..ea2de6f2 100644
+ return; + return;
+ +
+ ptrdiff_t modiff = BUF_MODIFF (b); + ptrdiff_t modiff = BUF_MODIFF (b);
+ ptrdiff_t overlay_modiff = BUF_OVERLAY_MODIFF (b);
+ ptrdiff_t pt = BUF_PT (b); + ptrdiff_t pt = BUF_PT (b);
+ NSUInteger textLen = cachedText ? [cachedText length] : 0; + NSUInteger textLen = cachedText ? [cachedText length] : 0;
+ /* TODO: Also track BUF_OVERLAY_MODIFF to catch overlay-only + /* Track both BUF_MODIFF and BUF_OVERLAY_MODIFF. Overlay-only
+ changes (e.g., timer-based completion highlight move without + changes (e.g., timer-based completion highlight move without
+ point change). Currently, overlay changes without text edits + text edit) bump overlay_modiff but not modiff. Also detect
+ are detected only when point also moves. */ + narrowing/widening which changes BUF_BEGV without bumping
+ either modiff counter. */
+ if (cachedText && cachedTextModiff == modiff + if (cachedText && cachedTextModiff == modiff
+ && cachedOverlayModiff == overlay_modiff
+ && cachedTextStart == BUF_BEGV (b)
+ && pt >= cachedTextStart + && pt >= cachedTextStart
+ && (textLen == 0 + && (textLen == 0
+ || [self accessibilityIndexForCharpos:pt] <= textLen)) + || [self accessibilityIndexForCharpos:pt] <= textLen))
@@ -1455,6 +1466,7 @@ index 932d209f..ea2de6f2 100644
+ [cachedText release]; + [cachedText release];
+ cachedText = [text retain]; + cachedText = [text retain];
+ cachedTextModiff = modiff; + cachedTextModiff = modiff;
+ cachedOverlayModiff = overlay_modiff;
+ cachedTextStart = start; + cachedTextStart = start;
+ +
+ if (visibleRuns) + if (visibleRuns)
@@ -1474,22 +1486,31 @@ index 932d209f..ea2de6f2 100644
+ thread invalidates the text cache concurrently. */ + thread invalidates the text cache concurrently. */
+ @synchronized (self) + @synchronized (self)
+ { + {
+ for (NSUInteger i = 0; i < visibleRunCount; i++) + if (visibleRunCount == 0)
+ return 0;
+
+ /* Binary search: runs are sorted by charpos (ascending). Find the
+ run whose [charpos, charpos+length) range contains the target,
+ or the nearest run after an invisible gap. O(log n) instead of
+ O(n) — matters for org-mode with many folded sections. */
+ NSUInteger lo = 0, hi = visibleRunCount;
+ while (lo < hi)
+ { + {
+ ns_ax_visible_run *r = &visibleRuns[i]; + NSUInteger mid = lo + (hi - lo) / 2;
+ if (charpos >= r->charpos && charpos < r->charpos + r->length) + ns_ax_visible_run *r = &visibleRuns[mid];
+ if (charpos < r->charpos)
+ hi = mid;
+ else if (charpos >= r->charpos + r->length)
+ lo = mid + 1;
+ else
+ { + {
+ /* Compute UTF-16 delta inside this run directly from cachedText + /* Found: charpos is inside this run. Compute UTF-16 delta
+ (an NSString built on the main thread) — no Lisp calls needed. */ + directly from cachedText — no Lisp calls needed. */
+ NSUInteger chars_in = (NSUInteger)(charpos - r->charpos); + NSUInteger chars_in = (NSUInteger)(charpos - r->charpos);
+ if (chars_in == 0 || !cachedText) + if (chars_in == 0 || !cachedText)
+ return r->ax_start; + return r->ax_start;
+ /* ax_start + UTF-16 units for the first chars_in chars of the run. */
+ NSUInteger run_end_ax = r->ax_start + r->ax_length; + NSUInteger run_end_ax = r->ax_start + r->ax_length;
+ NSUInteger scan = r->ax_start; + NSUInteger scan = r->ax_start;
+ /* Each visible Emacs char maps to 1 or 2 UTF-16 units.
+ Walk the NSString using rangeOfComposedCharacterSequenceAtIndex
+ which handles surrogates correctly. */
+ for (NSUInteger c = 0; c < chars_in && scan < run_end_ax; c++) + for (NSUInteger c = 0; c < chars_in && scan < run_end_ax; c++)
+ { + {
+ NSRange seq = [cachedText + NSRange seq = [cachedText
@@ -1498,18 +1519,12 @@ index 932d209f..ea2de6f2 100644
+ } + }
+ return (scan <= run_end_ax) ? scan : run_end_ax; + return (scan <= run_end_ax) ? scan : run_end_ax;
+ } + }
+ /* If charpos falls in an invisible gap before the next run,
+ map it to the start of the next visible run. */
+ if (charpos < r->charpos)
+ return r->ax_start;
+ } + }
+ /* Past end — return total length. */ + /* charpos falls in an invisible gap or past the end. */
+ if (visibleRunCount > 0) + if (lo < visibleRunCount)
+ { + return visibleRuns[lo].ax_start;
+ ns_ax_visible_run *last = &visibleRuns[visibleRunCount - 1]; + ns_ax_visible_run *last = &visibleRuns[visibleRunCount - 1];
+ return last->ax_start + last->ax_length; + return last->ax_start + last->ax_length;
+ }
+ return 0;
+ } /* @synchronized */ + } /* @synchronized */
+} +}
+ +
@@ -1521,17 +1536,25 @@ index 932d209f..ea2de6f2 100644
+ /* May be called from AX server thread — synchronize. */ + /* May be called from AX server thread — synchronize. */
+ @synchronized (self) + @synchronized (self)
+ { + {
+ for (NSUInteger i = 0; i < visibleRunCount; i++) + if (visibleRunCount == 0)
+ return cachedTextStart;
+
+ /* Binary search: runs are sorted by ax_start (ascending). */
+ NSUInteger lo = 0, hi = visibleRunCount;
+ while (lo < hi)
+ { + {
+ ns_ax_visible_run *r = &visibleRuns[i]; + NSUInteger mid = lo + (hi - lo) / 2;
+ if (ax_idx >= r->ax_start + ns_ax_visible_run *r = &visibleRuns[mid];
+ && ax_idx < r->ax_start + r->ax_length) + if (ax_idx < r->ax_start)
+ hi = mid;
+ else if (ax_idx >= r->ax_start + r->ax_length)
+ lo = mid + 1;
+ else
+ { + {
+ /* Found: ax_idx is inside this run. Walk composed character
+ sequences to count Emacs characters up to ax_idx. */
+ if (!cachedText) + if (!cachedText)
+ return r->charpos; + return r->charpos;
+
+ /* Walk forward through NSString composed character sequences to
+ count Emacs characters (= composed sequences) up to ax_idx. */
+ NSUInteger scan = r->ax_start; + NSUInteger scan = r->ax_start;
+ ptrdiff_t cp = r->charpos; + ptrdiff_t cp = r->charpos;
+ while (scan < ax_idx) + while (scan < ax_idx)
@@ -1545,7 +1568,7 @@ index 932d209f..ea2de6f2 100644
+ } + }
+ } + }
+ /* Past end — return last charpos. */ + /* Past end — return last charpos. */
+ if (visibleRunCount > 0) + if (lo > 0)
+ { + {
+ ns_ax_visible_run *last = &visibleRuns[visibleRunCount - 1]; + ns_ax_visible_run *last = &visibleRuns[visibleRunCount - 1];
+ return last->charpos + last->length; + return last->charpos + last->length;
@@ -2078,7 +2101,11 @@ index 932d209f..ea2de6f2 100644
+ return NSMakeRange (0, 0); + return NSMakeRange (0, 0);
+ +
+ /* Block input to prevent concurrent redisplay from modifying the + /* Block input to prevent concurrent redisplay from modifying the
+ glyph matrix while we traverse it. */ + glyph matrix while we traverse it. Use specpdl unwind protection
+ so block_input is always matched by unblock_input, even if
+ ensureTextCache triggers a Lisp signal (longjmp). */
+ specpdl_ref count = SPECPDL_INDEX ();
+ record_unwind_protect_void (unblock_input);
+ block_input (); + block_input ();
+ +
+ /* Find the glyph row at this y coordinate. */ + /* Find the glyph row at this y coordinate. */
@@ -2101,7 +2128,7 @@ index 932d209f..ea2de6f2 100644
+ +
+ if (!hit_row) + if (!hit_row)
+ { + {
+ unblock_input (); + unbind_to (count, Qnil);
+ return NSMakeRange (0, 0); + return NSMakeRange (0, 0);
+ } + }
+ +
@@ -2131,7 +2158,7 @@ index 932d209f..ea2de6f2 100644
+ if (cachedText && ax_idx > [cachedText length]) + if (cachedText && ax_idx > [cachedText length])
+ ax_idx = [cachedText length]; + ax_idx = [cachedText length];
+ +
+ unblock_input (); + unbind_to (count, Qnil);
+ return NSMakeRange (ax_idx, 1); + return NSMakeRange (ax_idx, 1);
+} +}
+ +

View File

@@ -77,7 +77,8 @@ ARCHITECTURE
text range, line/index/range conversions, frame-for-range, text range, line/index/range conversions, frame-for-range,
range-for-position, and insertion-point-line-number. range-for-position, and insertion-point-line-number.
- Maintains a text cache (cachedText / visibleRuns) keyed on - Maintains a text cache (cachedText / visibleRuns) keyed on
BUF_MODIFF. The cache is the single source of truth for all BUF_MODIFF, BUF_OVERLAY_MODIFF, and BUF_BEGV (narrowing).
The cache is the single source of truth for all
index-to-charpos and charpos-to-index mappings. index-to-charpos and charpos-to-index mappings.
- Detects buffer edits (modiff change), cursor movement (point - Detects buffer edits (modiff change), cursor movement (point
change), and mark changes, and posts the appropriate change), and mark changes, and posts the appropriate
@@ -282,14 +283,19 @@ TEXT CACHE AND VISIBLE RUNS
non-contiguous visible segments. The mapping array is stored in the non-contiguous visible segments. The mapping array is stored in the
EmacsAccessibilityBuffer ivar `visibleRuns' (C array, xmalloc'd). EmacsAccessibilityBuffer ivar `visibleRuns' (C array, xmalloc'd).
Index mapping (charpos <-> ax_index) does a linear scan of the run Index mapping (charpos <-> ax_index) uses binary search over the
array. Within a run, UTF-16 unit counting uses sorted run array — O(log n) per lookup. Within a run, UTF-16 unit
counting uses
rangeOfComposedCharacterSequenceAtIndex: to handle surrogate pairs rangeOfComposedCharacterSequenceAtIndex: to handle surrogate pairs
(emoji, rare CJK) correctly -- one Emacs character may occupy 2 (emoji, rare CJK) correctly -- one Emacs character may occupy 2
UTF-16 units. UTF-16 units.
Cache invalidation is triggered whenever BUF_MODIFF changes Cache invalidation is triggered whenever BUF_MODIFF or
(ensureTextCache compares cachedTextModiff). The cache is also BUF_OVERLAY_MODIFF changes (ensureTextCache compares both
cachedTextModiff and cachedOverlayModiff). Additionally,
narrowing/widening is detected by comparing cachedTextStart
against BUF_BEGV — these operations change the visible region
without bumping either modiff counter. The cache is also
invalidated when the window tree is rebuilt. NS_AX_TEXT_CAP = 100,000 invalidated when the window tree is rebuilt. NS_AX_TEXT_CAP = 100,000
UTF-16 units (~200 KB) caps total exposure; buffers larger than UTF-16 units (~200 KB) caps total exposure; buffers larger than
~50,000 lines are truncated for accessibility purposes. VoiceOver ~50,000 lines are truncated for accessibility purposes. VoiceOver
@@ -499,13 +505,6 @@ KEY DESIGN DECISIONS
KNOWN LIMITATIONS KNOWN LIMITATIONS
----------------- -----------------
- BUF_OVERLAY_MODIFF is not tracked. Overlay changes (e.g. moving
the completions-highlight overlay via Tab without changing buffer
text) do not bump BUF_MODIFF, so the text cache is not invalidated.
The notification logic detects point changes (cachedPoint) which
covers the common case, but overlay-only changes with a stationary
point would be missed. A future fix would compare overlay_modiff.
- Interactive span scan uses Fnext_single_property_change across - Interactive span scan uses Fnext_single_property_change across
multiple properties to skip non-interactive regions in bulk, but multiple properties to skip non-interactive regions in bulk, but
still visits every property-change boundary. For buffers with still visits every property-change boundary. For buffers with