New braille IPC APIs: BrailleMirror, DirectBrailleWindow, and named-pipe server#19856
New braille IPC APIs: BrailleMirror, DirectBrailleWindow, and named-pipe server#19856trypsynth wants to merge 6 commits intonvaccess:masterfrom
Conversation
|
Hi - please wait for feedback and triage on #19843 before opening a PR |
|
@seanbudd Just for clarity, this PR follows the design doc that was approved by the core team in a recent meeting. Since the implementation is ready for eyes, I’d prefer to keep this open (even as a Draft) so we have a central place for the technical review once your triage is complete. |
|
Hi @trypsynth - after re-confirming with the team, I am unclear where you got approval from. We have not discussed or approved the design yet, we will do that when reviewing #19843. When we are happy with the approach and for a PR to be opened, the triaged label will be applied to the issue. |
|
@seanbudd it appears to have been a cummunication error on my part, I initially emailed Quentin about this project to make sure NV Access was okay with it as a concept without us shouting it to the world in case they said no, and got confirmation that nV Access is okay with it after reviewing the design doc I sent over at a recent meeting. I got told to then create an issue to track it, and also for some reason thought a pr was mentioned, but I can't find it now, so quite possible I made that up. In any case, once the issue has been triaged, should we just reopen this existing pull request? This branch has just been sitting idle on my laptop while I work on RIM, and we're getting closer, and when I do need to make teeny fixes, I'm pushing to the same branch. |
|
@trypsynth - no problem with the confusion. We will probably need to re-discuss the approach to integration as part of reviewing and triaging the issue. Given the size of this PR, we will probably ask for it to be split into smaller separate PRs |
There was a problem hiding this comment.
Pull request overview
This PR introduces new braille IPC-facing core APIs (mirror + direct-braille window) and refactors NVDA Remote’s follower braille handling to use those APIs, alongside a new named-pipe server intended to expose braille output/input to external processes.
Changes:
- Add
braille.BrailleMirror,braille.DirectBrailleWindow, plusregisterMirror/unregisterMirrorandinjectGesture. - Add
braillePipeServernamed-pipe server for mirroring/display control and gesture injection. - Refactor
_remoteClientfollower-side braille forwarding to useBrailleMirror, and add unit tests for the new APIs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_braille/test_mirrorAndDirectWindow.py | New unit tests covering mirror lifecycle, display forwarding, dimension capping, injectGesture, and DirectBrailleWindow behaviors. |
| source/braillePipeServer.py | New named-pipe IPC server exposing mirror/direct-braille behaviors to external processes. |
| source/braille.py | Adds the new public braille APIs and wires pipe-server initialization/termination into braille init/shutdown. |
| source/_remoteClient/session.py | Refactors follower braille forwarding to use the new BrailleMirror API and adds a filter to force single-row output. |
| source/_remoteClient/localMachine.py | Uses braille.injectGesture to execute incoming remote braille gestures (removes direct inputCore calls and old size negotiation code). |
| # If braille is not yet initialised, queue registration. | ||
| if braille.handler is None: | ||
| pending_q.put(("mirror", handle, numCells)) | ||
| return | ||
| session = _MirrorSession(handle, numCells) | ||
| elif mtype == "register_direct_braille": | ||
| hwnd = int(msg.get("hwnd", 0)) | ||
| numCells = int(msg.get("numCells", 0)) | ||
| if braille.handler is None: | ||
| pending_q.put(("direct", handle, hwnd, numCells)) | ||
| return |
There was a problem hiding this comment.
When braille.handler is None, the connection handle is queued in pending_q and the function returns, but the finally block still runs and closes the handle. This leaves a closed/invalid handle in the pending queue and will immediately disconnect the client. If you keep the pending-registration path, ensure the handle is not closed in this case (or duplicate the handle for queuing).
| def __init__(self, handle: ctypes.wintypes.HANDLE, numCells: int) -> None: | ||
| self._numCells = numCells | ||
| self._writer = _AsyncWriter(handle) | ||
| braille.registerMirror(self) | ||
| braille.displaySizeChanged.register(self._handleDisplaySizeChanged) | ||
| if braille.handler: | ||
| self._writer.send({"type": "display_size", "numCols": braille.handler.displayDimensions.numCols}) | ||
|
|
||
| def numCells(self) -> int: | ||
| return self._numCells | ||
|
|
||
| def display(self, cells: list) -> None: | ||
| self._writer.send({"type": "display", "cells": cells}) | ||
|
|
||
| def _handleDisplaySizeChanged(self, displaySize: int, numRows: int, numCols: int) -> None: | ||
| self._writer.send({"type": "display_size", "numCols": numCols}) | ||
|
|
||
| def handleMessage(self, msg: dict) -> None: | ||
| mtype = msg.get("type") | ||
| if mtype == "inject_gesture": | ||
| gesture = _PipeBrailleGesture( | ||
| source=msg.get("source", ""), | ||
| model=msg.get("model", ""), | ||
| id_=msg.get("id", ""), | ||
| routingIndex=msg.get("routingIndex"), | ||
| dots=msg.get("dots", 0), | ||
| space=msg.get("space", False), | ||
| ) | ||
| wx.CallAfter(braille.injectGesture, gesture) | ||
| else: | ||
| log.debug(f"braillePipeServer: unexpected mirror message type {mtype!r}") | ||
|
|
||
| def close(self) -> None: | ||
| braille.displaySizeChanged.unregister(self._handleDisplaySizeChanged) | ||
| braille.unregisterMirror(self) | ||
| self._writer.stop() | ||
|
|
||
|
|
||
| class _DirectSession(braille.DirectBrailleWindow): | ||
| """DirectBrailleWindow that forwards gestures over the pipe.""" | ||
|
|
||
| def __init__(self, handle: ctypes.wintypes.HANDLE, hwnd: int, numCells: int) -> None: | ||
| super().__init__(hwnd=hwnd, numCells=numCells) | ||
| self._writer = _AsyncWriter(handle) | ||
| self.activate() | ||
|
|
There was a problem hiding this comment.
_MirrorSession/_DirectSession are constructed on per-connection threads, but they call into braille APIs that mutate global extension points (registerMirror, displaySizeChanged.register, DirectBrailleWindow.activate/deactivate). Those extension point handler lists are used on the main thread during braille rendering/input, so registering/unregistering from worker threads risks races. Consider marshaling these registrations/unregistrations to the main thread (e.g. via wxCallOnMain / wx.CallAfter + synchronization) and ensure session.close also runs the unregister steps on the main thread.
| def _isForeground(self) -> bool: | ||
| """Return True if our registered window is currently in the foreground.""" | ||
| fg = winUser.getForegroundWindow() | ||
| return fg == self._hwnd or winUser.isDescendantWindow(self._hwnd, fg) |
There was a problem hiding this comment.
DirectBrailleWindow._isForeground passes arguments to winUser.isDescendantWindow in the wrong order. isDescendantWindow expects (parentHwnd, childHwnd), and the common pattern elsewhere is isDescendantWindow(fg, windowHandle). As written, descendant windows of the foreground window won’t be recognized, so direct braille mode may not activate correctly when the target window is inside the foreground root. Swap the arguments so the foreground window is treated as the parent.
| return fg == self._hwnd or winUser.isDescendantWindow(self._hwnd, fg) | |
| return fg == self._hwnd or winUser.isDescendantWindow(fg, self._hwnd) |
| def _mirrorPreWriteCells(cells: List[int], **kwargs) -> None: | ||
| for mirror in _registeredMirrors: | ||
| mirror.display(cells) |
There was a problem hiding this comment.
_mirrorPreWriteCells forwards the mutable cells list directly to each mirror. Because this hook runs before the physical display write, any mirror that mutates the list (even accidentally) will change what NVDA outputs to the real display, which contradicts the expectation that a mirror is passive. Consider passing an immutable snapshot (e.g., a copy) to mirrors so they can’t affect the main output.
| """Named-pipe IPC server that exposes the Braille Mirror and Direct Braille Window APIs to external processes. | ||
|
|
||
| Two pipe names are used: | ||
|
|
||
| * ``\\\\.\\pipe\\screen_reader_braille`` – normal desktop instance | ||
| * ``\\\\.\\pipe\\screen_reader_braille_secure`` – secure desktop instance | ||
|
|
There was a problem hiding this comment.
The pipe names documented here (screen_reader_braille / screen_reader_braille_secure) don’t match the PR description / intended API names (e.g. \.\pipe\nvda_braille / \.\pipe\nvda_braille_secure). This mismatch will confuse integrators and can lead to clients connecting to the wrong pipe. Align the docstring (and/or the actual pipeName constants) with the agreed public pipe names.
| _kernel32 = ctypes.windll.kernel32 # type: ignore[attr-defined] | ||
| _advapi32 = ctypes.windll.advapi32 # type: ignore[attr-defined] | ||
|
|
There was a problem hiding this comment.
This module calls Win32 APIs via ctypes.windll without setting argtypes/restype. On 64-bit Python, HANDLE-returning functions (e.g. CreateNamedPipeW/CreateFileW) default to a 32-bit c_int restype, which can truncate handles and cause invalid handle usage/CloseHandle on the wrong value. Please define prototypes (restype=wintypes.HANDLE / BOOL etc.) for the kernel32/advapi32 functions you use, or use existing winBindings wrappers where available.
| def _writeAll(handle: ctypes.wintypes.HANDLE, data: bytes) -> bool: | ||
| """Write all of *data* to *handle*. Return False on pipe break.""" | ||
| offset = 0 | ||
| while offset < len(data): | ||
| written = ctypes.wintypes.DWORD(0) | ||
| ok = _kernel32.WriteFile( | ||
| handle, | ||
| ctypes.c_char_p(data[offset:]), | ||
| len(data) - offset, | ||
| ctypes.byref(written), | ||
| None, | ||
| ) | ||
| if not ok or written.value == 0: | ||
| return False | ||
| offset += written.value |
There was a problem hiding this comment.
_writeAll passes data to WriteFile using ctypes.c_char_p(data[offset:]). These frames include the 4-byte length prefix and can contain embedded NUL bytes, and c_char_p is intended for NUL-terminated strings. Use a raw buffer/pointer type (e.g. create_string_buffer / (c_char * n) / c_void_p) to avoid string semantics and to keep the buffer alive for the duration of the call.
| def _recvMessage(handle: ctypes.wintypes.HANDLE) -> Optional[dict]: | ||
| header = _readExact(handle, 4) | ||
| if header is None: | ||
| return None | ||
| (length,) = struct.unpack("<I", header) | ||
| body = _readExact(handle, length) | ||
| if body is None: |
There was a problem hiding this comment.
_recvMessage trusts the 32-bit length prefix and allocates a buffer of that size with no upper bound. A malicious/buggy client can request an extremely large length, causing memory exhaustion or very long blocking reads. Add a reasonable maximum frame size (and reject/close the connection when exceeded) before allocating/reading the body.
| @@ -0,0 +1,480 @@ | |||
| # A part of NonVisual Desktop Access (NVDA) | |||
| _kernel32 = ctypes.windll.kernel32 # type: ignore[attr-defined] | ||
| _advapi32 = ctypes.windll.advapi32 # type: ignore[attr-defined] |
There was a problem hiding this comment.
please make sure to use bindings from winBindings e.g. source\winBindings\kernel32.py rather than the windll directly
| _kernel32.CloseHandle(handle) | ||
|
|
||
|
|
||
| _server: Optional[_PipeServer] = None |
There was a problem hiding this comment.
please make sure to use modern types e.g.
| _server: Optional[_PipeServer] = None | |
| _server: _PipeServer | None = None |
| inputCore.registerGestureSource("br", BrailleDisplayGesture) | ||
|
|
||
|
|
||
| class BrailleMirror: |
There was a problem hiding this comment.
I think we should be avoiding adding substantial new code to source/braille.py (See #12772)
Could you please move the added classes and functions to a new file (e.g. source/_brailleMirror.py)?
|
Hi @trypsynth - I've performed a preliminary review, not a detailed on yet. |
seanbudd
added
the
conceptApproved
Link to issue number:
Closes #19843
Summary of the issue:
External processes (e.g. RIM's elevated service component) have no supported way to receive a live mirror of NVDA's braille output or to act as a direct braille display for a foreground window. The existing
pre_writeCells/filter_displayDimensions/decide_enabledextension points are not sufficient on their own: they require in-process code, have no HWND-scoping, and block the NVDA main thread if pipe I/O is done naively.Description of user facing changes:
None for end users. NVDA Remote users with braille displays can continue using braille normally; the refactor is transparent.
Description of developer facing changes:
Two new public APIs in
braille:BrailleMirror- abstract class. Register withbraille.registerMirror(); receives everydisplay()call and optionally caps the display width vianumCells(). Unregister withbraille.unregisterMirror().DirectBrailleWindow- HWND-scoped class. While its window is foreground, NVDA rendering is suspended (decide_enabled) and braille gestures are intercepted (decide_executeGesture) and forwarded toonGesture(). Subclass and callactivate()/deactivate().braille.injectGesture(gesture)- helper to execute aBrailleDisplayGestureviainputCore.manager, swallowingNoInputGestureAction.New module
braillePipeServerexposes both APIs over a named pipe (\.\pipe\nvda_braille/\.\pipe\nvda_braille_secure). Wire protocol: 4-byte LE uint32 length-prefix + UTF-8 JSON. Pipe DACL grantsNT AUTHORITY\SYSTEMread/write so RIM's elevated service can connect on the normal desktop.Description of development approach:
registerMirrorlazily hookspre_writeCellsandfilter_displayDimensions(shared single handler); unhooks when last mirror unregisters.DirectBrailleWindowhooksdecide_enabledandinputCore.decide_executeGestureper instance; optionally hooksfilter_displayDimensionsifnumCells > 0.braillePipeServeruses one accept-loop thread + one_AsyncWriterbackground thread per connection so pipe I/O never touches the NVDA main thread._remoteClientrefactored:FollowerSessionnow uses_FollowerBrailleMirrorinstead of directpre_writeCellswiring;localMachine.setBrailleDisplaySize/_handleFilterDisplayDimensionsremoved; pre-existingfilter_displayDimensionsleak on transport close fixed.Testing strategy:
30 unit tests added in
tests/unit/test_braille/test_mirrorAndDirectWindow.pycovering:BrailleMirrorregister/unregister lifecycle and extension-point hook/unhooknumCellscapping logic (zero, smaller, larger, smallest-wins)injectGesturehappy path andNoInputGestureActionswallowDirectBrailleWindowactivate/deactivate idempotency,decide_enabledanddecide_executeGesturerouting,filter_displayDimensionsregistration and cappingManual testing needed: NVDA Remote with a physical braille display on both sides. I've locally tested with a 20 cell on both sides individually, but am unable to test both at the same time
Known issues with pull request:
RIM client-side integration is not yet complete. Do not merge until the RIM release is coordinated.
LeaderSessionin NVDA Remote still wiresdecide_executeGesturedirectly rather than viaDirectBrailleWindow; should this be migrated in core as well? Needs discussion.Code Review Checklist: