-
Notifications
You must be signed in to change notification settings - Fork 0
Fix zbus API changes in coldvox-text-injection #295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix zbus API changes in coldvox-text-injection #295
Conversation
The atspi-common crate changed ObjectRef from a struct with direct `name` and `path` fields to an enum with Null/Owned/Borrowed variants. Use the new accessor methods `name_as_str()` and `path_as_str()` instead of direct field access.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No Critical Issues Found
2 files reviewed | Confidence: 95% | Recommendation: Fix one warning before merge
Review Details
Files:
- crates/coldvox-text-injection/src/manager.rs (correctly updated)
- crates/coldvox-text-injection/src/prewarm.rs (1 warning)
Checked: Security, bugs, performance, error handling, API compatibility
Summary
The PR successfully migrates from direct field access to the new atspi-common 0.13.0 API with accessor methods. The changes in manager.rs are implemented correctly with proper null safety handling.
Issues Found:
- WARNING in prewarm.rs:
path_as_str()could returnNone, causing a panic when calling.to_string()directly. The suggested fix adds proper Option handling.
Positive Changes:
- Manager.rs properly uses pattern matching for safe Option handling
- Prewarm.rs correctly uses
.map()for the name field - Both files now use the recommended accessor methods from atspi-common 0.13.0
The migration follows the new API requirements and maintains backward compatibility while improving type safety.
| .await | ||
| { | ||
| if let Some(obj_ref) = matches.pop() { | ||
| if let Some(name) = obj_ref.name() { | ||
| if let Some(name) = obj_ref.name_as_str() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ GOOD: Properly handles the Option return type with pattern matching, ensuring null safety for the name field.
| if !name.is_empty() { | ||
| return Ok(name.to_string()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ GOOD: Correctly uses the accessor method and handles the Option return type properly for the path field.
|
|
||
| target_app = obj_ref.name().map(|n| n.to_string()); | ||
| window_id = Some(obj_ref.path().to_string()); | ||
| target_app = obj_ref.name_as_str().map(|s| s.to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path_as_str() returns Option<&str> and could be None, causing a panic when calling .to_string() on None. Fix:
| target_app = obj_ref.name().map(|n| n.to_string()); | |
| window_id = Some(obj_ref.path().to_string()); | |
| target_app = obj_ref.name_as_str().map(|s| s.to_string()); | |
| if let Some(path) = obj_ref.path_as_str() { | |
| window_id = Some(path.to_string()); | |
| } else { | |
| window_id = None; | |
| } |
| focused_node = Some("focused".to_string()); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ GOOD: Correctly handles the Option return type with .map() for the name field, avoiding potential null pointer dereference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adapts the coldvox-text-injection crate to handle breaking API changes in the atspi-common dependency, where ObjectRef was refactored from a struct with direct name and path fields to an enum requiring accessor methods name_as_str() and path_as_str().
Key changes:
- Updated ObjectRef field access to use new accessor methods in AT-SPI code paths
- Properly handles
name_as_str()returningOption<&str>(name can be absent) - Properly handles
path_as_str()returning&str(path is always present)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/coldvox-text-injection/src/prewarm.rs | Updated AT-SPI pre-warming to use name_as_str() and path_as_str() accessor methods when extracting application and window identifiers from focused elements |
| crates/coldvox-text-injection/src/manager.rs | Updated application ID detection to use new accessor methods with proper Option handling for optional name field and string manipulation for path field |
|
Closing as already fixed - the ObjectRef API migration from direct field access to method calls is already in main. Current main uses The fix was likely applied as part of PR #296 (feat: upgrade PyO3 0.27, stabilize CI, and fix real injection tests). |
User description
The atspi-common crate changed ObjectRef from a struct with direct
nameandpathfields to an enum with Null/Owned/Borrowed variants. Use the new accessor methodsname_as_str()andpath_as_str()instead of direct field access.Description
Type of Change
Changelog
docs/standards.mdChangelog Rubric)If changelog updated: Confirm entry follows Keep a Changelog format and is under
## [Unreleased]section with appropriate category (Added/Changed/Deprecated/Removed/Fixed/Security/Documentation/Dependencies).If no changelog: Briefly explain why this change is not user-visible:
Documentation
docs/MasterDocumentationPlaybook.md)Testing
Test details:
Checklist
cargo fmt,cargo clippy)Related Issues
Additional Context
PR Type
Bug fix
Description
Update ObjectRef API usage for atspi-common 0.13.0 compatibility
Replace direct field access with new accessor methods
Handle ObjectRef enum variants properly with Option types
Diagram Walkthrough
File Walkthrough
manager.rs
Update ObjectRef field access to use accessor methodscrates/coldvox-text-injection/src/manager.rs
obj_ref.namedirect access withobj_ref.name_as_str()methodcall
obj_ref.pathdirect access withobj_ref.path_as_str()methodcall
prewarm.rs
Migrate to ObjectRef accessor methods in prewarmcrates/coldvox-text-injection/src/prewarm.rs
obj_ref.namewithobj_ref.name_as_str()and map to Optionobj_ref.pathwithobj_ref.path_as_str()for accessor methodusage