feat(linux): port type_into_editable from pyatspi subprocess to native AT-SPI - #1853
Open
r33drichards wants to merge 1 commit into
Open
feat(linux): port type_into_editable from pyatspi subprocess to native AT-SPI#1853r33drichards wants to merge 1 commit into
r33drichards wants to merge 1 commit into
Conversation
…e AT-SPI Replace the python3/pyatspi -c subprocess in atspi::type_into_editable with a native Rust implementation in atspi::native: walk the target pid's tree over the existing AccessibilityConnection, pick the best editable via pick_editable, and call EditableText.set_text_contents directly (setTextContents semantics, unlike insert_text which inserts at the caret). Same contract as before: works on unfocused windows when the toolkit exposes EditableText (Qt6, GTK4); errs when none is exposed (Qt5 unfocused) so callers can run the focus workaround and retry. Also drop stale pyatspi references in comments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. 📝 WalkthroughWalkthroughThis PR migrates the ChangesNative AT-SPI Text Input
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/cua-driver/rust/crates/platform-linux/src/atspi/native.rs`:
- Around line 549-578: The async work in type_into_editable (the
runtime().block_on(async { ... }) block that calls AccessibilityConnection::new,
collect_visited, pick_editable, obtains editable_text and calls
set_text_contents via call(...).await) must be bounded by an overall timeout;
wrap that entire async operation with tokio::time::timeout (or equivalent
OP_TIMEOUT) and convert a timeout result into an anyhow error like
"type_into_editable timed out" so callers receive a clear timeout error instead
of a hung blocking worker; ensure existing per-step map_err branches remain but
return a timeout error when tokio::time::timeout returns Err.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a790977-ac62-43a7-a560-bd46968d88a7
📒 Files selected for processing (3)
libs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rslibs/cua-driver/rust/crates/platform-linux/src/atspi/native.rslibs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs
Comment on lines
+549
to
+578
| runtime().block_on(async { | ||
| let conn = AccessibilityConnection::new() | ||
| .await | ||
| .map_err(|e| anyhow!("AT-SPI connect failed: {e}"))?; | ||
| let visited = collect_visited(&conn, pid) | ||
| .await? | ||
| .ok_or_else(|| anyhow!("no AT-SPI application for pid {pid}"))?; | ||
| let target = pick_editable(&visited) | ||
| .ok_or_else(|| anyhow!("no editable element found for pid {pid}"))?; | ||
| dlog!( | ||
| "type_into_editable target: role={:?} focused={} in_web_doc={}", | ||
| target.role, target.focused, target.in_web_doc | ||
| ); | ||
|
|
||
| let et = target | ||
| .acc | ||
| .proxies() | ||
| .await | ||
| .map_err(|e| anyhow!("interface proxies unavailable: {e}"))? | ||
| .editable_text() | ||
| .await | ||
| .map_err(|e| anyhow!("EditableText unavailable: {e}"))?; | ||
| match call(et.set_text_contents(text)).await { | ||
| Some(Ok(true)) => Ok(()), | ||
| Some(Ok(false)) => Err(anyhow!("setTextContents rejected by toolkit")), | ||
| Some(Err(e)) => Err(anyhow!("setTextContents failed: {e}")), | ||
| None => Err(anyhow!("setTextContents timed out")), | ||
| } | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Add an overall timeout for type_into_editable to keep latency bounded.
Line 549 executes the full tree walk/write path without OP_TIMEOUT; on large/unresponsive trees this can block for a long time and tie up blocking workers.
⏱️ Suggested fix
pub fn type_into_editable(pid: u32, text: &str) -> Result<()> {
runtime().block_on(async {
- let conn = AccessibilityConnection::new()
- .await
- .map_err(|e| anyhow!("AT-SPI connect failed: {e}"))?;
- let visited = collect_visited(&conn, pid)
- .await?
- .ok_or_else(|| anyhow!("no AT-SPI application for pid {pid}"))?;
- let target = pick_editable(&visited)
- .ok_or_else(|| anyhow!("no editable element found for pid {pid}"))?;
- dlog!(
- "type_into_editable target: role={:?} focused={} in_web_doc={}",
- target.role, target.focused, target.in_web_doc
- );
-
- let et = target
- .acc
- .proxies()
- .await
- .map_err(|e| anyhow!("interface proxies unavailable: {e}"))?
- .editable_text()
- .await
- .map_err(|e| anyhow!("EditableText unavailable: {e}"))?;
- match call(et.set_text_contents(text)).await {
- Some(Ok(true)) => Ok(()),
- Some(Ok(false)) => Err(anyhow!("setTextContents rejected by toolkit")),
- Some(Err(e)) => Err(anyhow!("setTextContents failed: {e}")),
- None => Err(anyhow!("setTextContents timed out")),
+ let work = async {
+ let conn = AccessibilityConnection::new()
+ .await
+ .map_err(|e| anyhow!("AT-SPI connect failed: {e}"))?;
+ let visited = collect_visited(&conn, pid)
+ .await?
+ .ok_or_else(|| anyhow!("no AT-SPI application for pid {pid}"))?;
+ let target = pick_editable(&visited)
+ .ok_or_else(|| anyhow!("no editable element found for pid {pid}"))?;
+ dlog!(
+ "type_into_editable target: role={:?} focused={} in_web_doc={}",
+ target.role, target.focused, target.in_web_doc
+ );
+
+ let et = target
+ .acc
+ .proxies()
+ .await
+ .map_err(|e| anyhow!("interface proxies unavailable: {e}"))?
+ .editable_text()
+ .await
+ .map_err(|e| anyhow!("EditableText unavailable: {e}"))?;
+ match call(et.set_text_contents(text)).await {
+ Some(Ok(true)) => Ok(()),
+ Some(Ok(false)) => Err(anyhow!("setTextContents rejected by toolkit")),
+ Some(Err(e)) => Err(anyhow!("setTextContents failed: {e}")),
+ None => Err(anyhow!("setTextContents timed out")),
+ }
+ };
+ match tokio::time::timeout(OP_TIMEOUT, work).await {
+ Ok(r) => r,
+ Err(_) => Err(anyhow!("type_into_editable timed out for pid {pid}")),
}
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| runtime().block_on(async { | |
| let conn = AccessibilityConnection::new() | |
| .await | |
| .map_err(|e| anyhow!("AT-SPI connect failed: {e}"))?; | |
| let visited = collect_visited(&conn, pid) | |
| .await? | |
| .ok_or_else(|| anyhow!("no AT-SPI application for pid {pid}"))?; | |
| let target = pick_editable(&visited) | |
| .ok_or_else(|| anyhow!("no editable element found for pid {pid}"))?; | |
| dlog!( | |
| "type_into_editable target: role={:?} focused={} in_web_doc={}", | |
| target.role, target.focused, target.in_web_doc | |
| ); | |
| let et = target | |
| .acc | |
| .proxies() | |
| .await | |
| .map_err(|e| anyhow!("interface proxies unavailable: {e}"))? | |
| .editable_text() | |
| .await | |
| .map_err(|e| anyhow!("EditableText unavailable: {e}"))?; | |
| match call(et.set_text_contents(text)).await { | |
| Some(Ok(true)) => Ok(()), | |
| Some(Ok(false)) => Err(anyhow!("setTextContents rejected by toolkit")), | |
| Some(Err(e)) => Err(anyhow!("setTextContents failed: {e}")), | |
| None => Err(anyhow!("setTextContents timed out")), | |
| } | |
| }) | |
| } | |
| pub fn type_into_editable(pid: u32, text: &str) -> Result<()> { | |
| runtime().block_on(async { | |
| let work = async { | |
| let conn = AccessibilityConnection::new() | |
| .await | |
| .map_err(|e| anyhow!("AT-SPI connect failed: {e}"))?; | |
| let visited = collect_visited(&conn, pid) | |
| .await? | |
| .ok_or_else(|| anyhow!("no AT-SPI application for pid {pid}"))?; | |
| let target = pick_editable(&visited) | |
| .ok_or_else(|| anyhow!("no editable element found for pid {pid}"))?; | |
| dlog!( | |
| "type_into_editable target: role={:?} focused={} in_web_doc={}", | |
| target.role, target.focused, target.in_web_doc | |
| ); | |
| let et = target | |
| .acc | |
| .proxies() | |
| .await | |
| .map_err(|e| anyhow!("interface proxies unavailable: {e}"))? | |
| .editable_text() | |
| .await | |
| .map_err(|e| anyhow!("EditableText unavailable: {e}"))?; | |
| match call(et.set_text_contents(text)).await { | |
| Some(Ok(true)) => Ok(()), | |
| Some(Ok(false)) => Err(anyhow!("setTextContents rejected by toolkit")), | |
| Some(Err(e)) => Err(anyhow!("setTextContents failed: {e}")), | |
| None => Err(anyhow!("setTextContents timed out")), | |
| } | |
| }; | |
| match tokio::time::timeout(OP_TIMEOUT, work).await { | |
| Ok(r) => r, | |
| Err(_) => Err(anyhow!("type_into_editable timed out for pid {pid}")), | |
| } | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver/rust/crates/platform-linux/src/atspi/native.rs` around lines
549 - 578, The async work in type_into_editable (the runtime().block_on(async {
... }) block that calls AccessibilityConnection::new, collect_visited,
pick_editable, obtains editable_text and calls set_text_contents via
call(...).await) must be bounded by an overall timeout; wrap that entire async
operation with tokio::time::timeout (or equivalent OP_TIMEOUT) and convert a
timeout result into an anyhow error like "type_into_editable timed out" so
callers receive a clear timeout error instead of a hung blocking worker; ensure
existing per-step map_err branches remain but return a timeout error when
tokio::time::timeout returns Err.
Contributor
Linux visual regression artifactsMatrix jobs now run independently. Download visual artifacts from this workflow run.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #1789. Ports the last remaining pyatspi Python-subprocess path,
atspi::type_into_editable, to the native Rust AT-SPI implementation inatspi::native:AccessibilityConnection(nopython3 -csubprocess, no pyatspi runtime dependency for this path)pick_editableheuristicEditableText.set_text_contentsdirectly — setTextContents semantics, unlikeinsert_textwhich inserts at the caretBehaviour contract is unchanged: works on unfocused windows when the toolkit exposes EditableText (Qt6, GTK4); returns
Errwhen none is exposed (e.g. Qt5 unfocused) so callers can run the focus workaround and retry.Also drops stale pyatspi references in comments (
AtspiNode::element_key,resolve_element_local_coords).Test plan
cua-driver-linux-background-gui-*matrix (exercises the AT-SPI read/write paths end-to-end in NixOS VMs, incl. thetkfull entry which types via the native AT-SPI path)cua-driver-linux-background-terminal-gif/cua-driver-linux-cursor-click-gif🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Documentation