martin/emacs-doom
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

patches: address maintainer review findings (C1/C2/H1/H2/M5/M6)

Browse Source 
C1 - block_input ordering in ns_ax_buffer_text:
block_input() now called before record_unwind_protect_void(unblock_input).
Previously the unwind handler could have been called without a matching
block_input, corrupting the input-blocking reference count.

C2 - unbind_to missing in patch 0004:
unbind_to(blk_count, Qnil) moved from patch 0008 to patch 0004 so that
ns_ax_scan_interactive_spans has a complete block_input/unbind_to pair
when patches 0000-0004 are applied independently.

H1 - Zoom patch forward dependency on VoiceOver:
Removed forward declaration 'static bool ns_ax_face_is_selected' and
the delegation from ns_zoom_face_is_selected.  Restored standalone
implementation of ns_zoom_face_is_selected in the Zoom patch so patch
0000 compiles and links independently of the VoiceOver patches.

H2 - ns_accessibility_enabled removal undocumented:
Added comment to ns_zoom_track_completion explaining that Zoom cursor
tracking is gated only on ns_zoom_enabled_p(), not ns_accessibility_enabled.
Users running Zoom without VoiceOver must still get completion tracking.

M5 - childFrameLastBuffer GC safety undocumented:
Added comment at the assignment site explaining why BVAR(b, name) (an
interned symbol reachable from obarray) is GC-safe without staticpro.

M6 - FOR_EACH_FRAME without block_input:
Added block_input/unblock_input around the FOR_EACH_FRAME loop in
postAccessibilityUpdates that checks for visible child frames.
Vframe_list must not be modified by timers or process sentinels
during iteration.
This commit is contained in:
Daneel
2026-03-03 10:11:39 +01:00
parent 73563be72d
commit 3bb6c989c9
9 changed files with 80 additions and 91 deletions
Download Patch File Download Diff File Expand all files Collapse all files

122
patches/0008-ns-announce-child-frame-completion-candidates-for-Vo.patch
View File

@@ -1,4 +1,4 @@
From 0cd27cd398ebcbaadd526b404cf7d549bfe53a4a Mon Sep 17 00:00:00 2001
From 7cb9bca680a56848fe4bd4ddb51f643e8946e7b8 Mon Sep 17 00:00:00 2001
From: Daneel <daneel@sukany.cz>
Date: Mon, 2 Mar 2026 18:49:13 +0100
Subject: [PATCH 8/8] ns: announce child frame completion candidates for
@@ -33,8 +33,8 @@ area announcements.
doc/emacs/macos.texi | 14 +-
etc/NEWS | 18 +-
src/nsterm.h | 20 ++
src/nsterm.m | 511 +++++++++++++++++++++++++++++++++++++------
4 files changed, 489 insertions(+), 74 deletions(-)
src/nsterm.m | 507 +++++++++++++++++++++++++++++++++++++++----
4 files changed, 499 insertions(+), 60 deletions(-)
diff --git a/doc/emacs/macos.texi b/doc/emacs/macos.texi
index 8d4a7825d8..03a657f970 100644
@@ -149,54 +149,25 @@ index 21a93bc799..bdd40b8eb7 100644
@end
diff --git a/src/nsterm.m b/src/nsterm.m
index 8f744d1bf3..dc5b965468 100644
index 8ef344d9fe..5038f9830d 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -1126,24 +1126,19 @@ Uses CFAbsoluteTimeGetCurrent() (~5 ns, a VDSO read) for timing. */
ivy-current-match, etc. by checking the face symbol name.
Defined here so the Zoom patch compiles independently of the
VoiceOver patches. */
+/* Forward declaration --- ns_ax_face_is_selected is defined in the
+ VoiceOver section below; ns_zoom_face_is_selected delegates to it. */
+static bool ns_ax_face_is_selected (Lisp_Object face);
+
static bool
ns_zoom_face_is_selected (Lisp_Object face)
{
- if (SYMBOLP (face))
- {
- const char *name = SSDATA (SYMBOL_NAME (face));
- return (strstr (name, "current") != NULL
- || strstr (name, "selected") != NULL
- || strstr (name, "selection") != NULL);
- }
- if (CONSP (face))
- {
- Lisp_Object tail;
- for (tail = face; CONSP (tail); tail = XCDR (tail))
- if (ns_zoom_face_is_selected (XCAR (tail)))
- return true;
- }
- return false;
+ /* Forward to ns_ax_face_is_selected (defined in the VoiceOver section
+ below) so that Zoom and VoiceOver agree on what constitutes a
+ "selected" face. Identical logic in two places would diverge over
+ time; one canonical implementation is preferable.
+ The forward declaration appears in nsterm.h. */
+ return ns_ax_face_is_selected (face);
}
/* Scan overlay before-string / after-string properties in the
@@ -1275,7 +1270,7 @@ If a completion candidate is selected (overlay or child frame),
@@ -1275,7 +1275,13 @@ If a completion candidate is selected (overlay or child frame),
static void
ns_zoom_track_completion (struct frame *f, EmacsView *view)
{
- if (!ns_accessibility_enabled || !ns_zoom_enabled_p ())
+ /* Zoom cursor tracking is controlled exclusively by
+ ns_zoom_enabled_p (). We do NOT gate on ns_accessibility_enabled:
+ users can run Zoom without VoiceOver, and those users should still
+ get completion-candidate tracking. ns_accessibility_enabled is
+ only set when a screen reader (VoiceOver or similar) activates the
+ AX layer; it has no bearing on the Zoom feature. */
+ if (!ns_zoom_enabled_p ())
return;
if (!WINDOWP (f->selected_window))
return;
@@ -1393,7 +1388,7 @@ so the visual offset is (ov_line + 1) * line_h from
@@ -1393,7 +1399,7 @@ so the visual offset is (ov_line + 1) * line_h from
(zoomCursorUpdated is NO). */
#if defined (MAC_OS_X_VERSION_MIN_REQUIRED) \
&& MAC_OS_X_VERSION_MIN_REQUIRED >= 101000
@@ -205,7 +176,7 @@ index 8f744d1bf3..dc5b965468 100644
&& ns_zoom_enabled_p ()
&& !NSIsEmptyRect (view->lastCursorRect))
{
@@ -3571,7 +3566,7 @@ EmacsView pixels (AppKit, flipped, top-left origin)
@@ -3571,7 +3577,7 @@ EmacsView pixels (AppKit, flipped, top-left origin)
#if defined (MAC_OS_X_VERSION_MIN_REQUIRED) \
&& MAC_OS_X_VERSION_MIN_REQUIRED >= 101000
@@ -214,7 +185,7 @@ index 8f744d1bf3..dc5b965468 100644
{
NSRect windowRect = [view convertRect:r toView:nil];
NSRect screenRect
@@ -7407,6 +7402,112 @@ visual line index for Zoom (skip whitespace-only lines
@@ -7407,6 +7413,112 @@ visual line index for Zoom (skip whitespace-only lines
return nil;
}
@@ -327,7 +298,22 @@ index 8f744d1bf3..dc5b965468 100644
/* Build accessibility text for window W, skipping invisible text.
Populates *OUT_START with the buffer start charpos.
Populates *OUT_RUNS with an array of visible runs and *OUT_NRUNS
@@ -8605,6 +8706,11 @@ - (void)setAccessibilitySelectedTextRange:(NSRange)range
@@ -7440,9 +7552,13 @@ visual line index for Zoom (skip whitespace-only lines
return @"";
specpdl_ref count = SPECPDL_INDEX ();
+ /* block_input must precede record_unwind_protect_void (unblock_input):
+ if anything between SPECPDL_INDEX and block_input were to throw,
+ the unwind handler would call unblock_input without a matching
+ block_input, corrupting the input-blocking reference count. */
+ block_input ();
record_unwind_current_buffer ();
record_unwind_protect_void (unblock_input);
- block_input ();
if (b != current_buffer)
set_buffer_internal_1 (b);
@@ -8605,6 +8721,11 @@ - (void)setAccessibilitySelectedTextRange:(NSRange)range
[self ensureTextCache];
@@ -339,7 +325,7 @@ index 8f744d1bf3..dc5b965468 100644
specpdl_ref count = SPECPDL_INDEX ();
record_unwind_current_buffer ();
/* Ensure block_input is always matched by unblock_input even if
@@ -9053,20 +9159,38 @@ - (void)postFocusedCursorNotification:(ptrdiff_t)point
@@ -9053,20 +9174,38 @@ - (void)postFocusedCursorNotification:(ptrdiff_t)point
&& granularity
== ns_ax_text_selection_granularity_character);
@@ -388,7 +374,7 @@ index 8f744d1bf3..dc5b965468 100644
ns_ax_post_notification_with_info (
self,
NSAccessibilitySelectedTextChangedNotification,
@@ -9166,12 +9290,17 @@ user expectation ("w" jumps to next word and reads it). */
@@ -9166,12 +9305,17 @@ user expectation ("w" jumps to next word and reads it). */
}
}
@@ -411,7 +397,7 @@ index 8f744d1bf3..dc5b965468 100644
if (cachedText
&& granularity == ns_ax_text_selection_granularity_line)
{
@@ -9236,6 +9365,11 @@ - (void)postCompletionAnnouncementForBuffer:(struct buffer *)b
@@ -9236,6 +9380,11 @@ - (void)postCompletionAnnouncementForBuffer:(struct buffer *)b
block_input ();
specpdl_ref count2 = SPECPDL_INDEX ();
@@ -423,7 +409,7 @@ index 8f744d1bf3..dc5b965468 100644
record_unwind_protect_void (unblock_input);
record_unwind_current_buffer ();
if (b != current_buffer)
@@ -9412,12 +9546,29 @@ - (void)postAccessibilityNotificationsForFrame:(struct frame *)f
@@ -9412,12 +9561,29 @@ - (void)postAccessibilityNotificationsForFrame:(struct frame *)f
if (!b)
return;
@@ -453,7 +439,7 @@ index 8f744d1bf3..dc5b965468 100644
if (modiff != self.cachedModiff)
{
self.cachedModiff = modiff;
@@ -9431,6 +9582,7 @@ Text property changes (e.g. face updates from
@@ -9431,6 +9597,7 @@ Text property changes (e.g. face updates from
{
self.cachedCharsModiff = chars_modiff;
[self postTextChangedNotification:point];
@@ -461,7 +447,7 @@ index 8f744d1bf3..dc5b965468 100644
}
}
@@ -9453,8 +9605,15 @@ frameworks like Vertico bump BOTH BUF_MODIFF (via text property
@@ -9453,8 +9620,15 @@ 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.
@@ -479,7 +465,7 @@ index 8f744d1bf3..dc5b965468 100644
goto skip_overlay_scan;
int selected_line = -1;
@@ -9500,7 +9659,18 @@ frameworks like Vertico bump BOTH BUF_MODIFF (via text property
@@ -9500,7 +9674,18 @@ frameworks like Vertico bump BOTH BUF_MODIFF (via text property
self.cachedPoint = point;
self.cachedMarkActive = markActive;
@@ -499,7 +485,7 @@ index 8f744d1bf3..dc5b965468 100644
NSInteger direction = ns_ax_text_selection_direction_discontiguous;
if (point > oldPoint)
direction = ns_ax_text_selection_direction_next;
@@ -9512,6 +9682,7 @@ frameworks like Vertico bump BOTH BUF_MODIFF (via text property
@@ -9512,6 +9697,7 @@ frameworks like Vertico bump BOTH BUF_MODIFF (via text property
/* --- Granularity detection --- */
NSInteger granularity = ns_ax_text_selection_granularity_unknown;
@@ -507,7 +493,7 @@ index 8f744d1bf3..dc5b965468 100644
[self ensureTextCache];
if (cachedText && oldPoint > 0)
{
@@ -9526,7 +9697,18 @@ frameworks like Vertico bump BOTH BUF_MODIFF (via text property
@@ -9526,7 +9712,18 @@ frameworks like Vertico bump BOTH BUF_MODIFF (via text property
NSRange newLine = [cachedText lineRangeForRange:
NSMakeRange (newIdx, 0)];
if (oldLine.location != newLine.location)
@@ -527,7 +513,7 @@ index 8f744d1bf3..dc5b965468 100644
else
{
NSUInteger dist = (newIdx > oldIdx
@@ -9548,34 +9730,23 @@ frameworks like Vertico bump BOTH BUF_MODIFF (via text property
@@ -9548,34 +9745,23 @@ frameworks like Vertico bump BOTH BUF_MODIFF (via text property
granularity = ns_ax_text_selection_granularity_line;
}
@@ -575,7 +561,7 @@ index 8f744d1bf3..dc5b965468 100644
{
NSWindow *win = [self.emacsView window];
if (win)
@@ -9734,6 +9905,13 @@ - (NSRect)accessibilityFrame
@@ -9734,6 +9920,13 @@ - (NSRect)accessibilityFrame
if (vis_start >= vis_end)
return @[];
@@ -589,15 +575,7 @@ index 8f744d1bf3..dc5b965468 100644
block_input ();
specpdl_ref blk_count = SPECPDL_INDEX ();
record_unwind_protect_void (unblock_input);
@@ -9858,6 +10036,7 @@ than O(chars). Fall back to pos+1 as safety net. */
pos = span_end;
}
+ unbind_to (blk_count, Qnil);
return [[spans copy] autorelease];
}
@@ -10039,6 +10218,10 @@ - (void)dealloc
@@ -10040,6 +10233,10 @@ - (void)dealloc
#endif
[accessibilityElements release];
@@ -608,7 +586,7 @@ index 8f744d1bf3..dc5b965468 100644
[[self menu] release];
[super dealloc];
}
@@ -11488,6 +11671,9 @@ - (instancetype) initFrameFromEmacs: (struct frame *)f
@@ -11489,6 +11686,9 @@ - (instancetype) initFrameFromEmacs: (struct frame *)f
windowClosing = NO;
processingCompose = NO;
@@ -618,7 +596,7 @@ index 8f744d1bf3..dc5b965468 100644
scrollbarsNeedingUpdate = 0;
fs_state = FULLSCREEN_NONE;
fs_before_fs = next_maximized = -1;
@@ -12796,6 +12982,154 @@ - (id)accessibilityFocusedUIElement
@@ -12797,6 +12997,159 @@ - (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. */
@@ -711,6 +689,11 @@ index 8f744d1bf3..dc5b965468 100644
+ if (EQ (childFrameLastBuffer, BVAR (b, name))
+ && modiff == childFrameLastModiff)
+ return;
+ /* Store the buffer name symbol (an interned Lisp_Object from
+ obarray) rather than a raw pointer to struct buffer.
+ Interned symbols are reachable from obarray and will not be
+ garbage-collected, so no staticpro() registration is needed
+ for this ivar. */
+ childFrameLastBuffer = BVAR (b, name);
+ childFrameLastModiff = modiff;
+
@@ -773,7 +756,7 @@ index 8f744d1bf3..dc5b965468 100644
- (void)postAccessibilityUpdates
{
NSTRACE ("[EmacsView postAccessibilityUpdates]");
@@ -12806,11 +13140,64 @@ - (void)postAccessibilityUpdates
@@ -12807,11 +13160,69 @@ - (void)postAccessibilityUpdates
/* Re-entrance guard: VoiceOver callbacks during notification posting
can trigger redisplay, which calls ns_update_end, which calls us
@@ -808,6 +791,10 @@ index 8f744d1bf3..dc5b965468 100644
+ {
+ Lisp_Object tail, frame;
+ BOOL childStillVisible = NO;
+ /* block_input protects the FOR_EACH_FRAME iteration: the
+ frame list (Vframe_list) is a Lisp_Object chain and must not
+ be modified by a timer or process sentinel mid-iteration. */
+ block_input ();
+ FOR_EACH_FRAME (tail, frame)
+ if (FRAME_PARENT_FRAME (XFRAME (frame)) == emacsframe
+ && FRAME_VISIBLE_P (XFRAME (frame)))
@@ -815,6 +802,7 @@ index 8f744d1bf3..dc5b965468 100644
+ childStillVisible = YES;
+ break;
+ }
+ unblock_input ();
+
+ if (!childStillVisible)
+ {