Refactor monitor handling in automation nodes for improved readabilit…#547
Refactor monitor handling in automation nodes for improved readabilit…#547felix-schultz merged 1 commit intoalphafrom
Conversation
…y and maintainability
There was a problem hiding this comment.
Code Review
This pull request updates CI environment dependencies and refactors several automation nodes—including screenshot, display, and RPA logic—to scope monitor enumeration within local blocks. It also introduces a SendSyncRustAutoGui wrapper to manually implement Send and Sync for the underlying GUI library. Feedback identifies a critical thread-safety concern regarding the manual Send/Sync implementation for GDI handles, which may lead to undefined behavior during thread migration. Additionally, there are performance concerns regarding redundant monitor enumeration inside loops and functional issues where provided monitor or display indices are being ignored in screenshot and region capture logic.
| unsafe impl Send for SendSyncRustAutoGui {} | ||
| #[cfg(feature = "execute")] | ||
| unsafe impl Sync for SendSyncRustAutoGui {} |
There was a problem hiding this comment.
The safety justification for manually implementing Send and Sync on SendSyncRustAutoGui is potentially insufficient. While the Mutex provides mutual exclusion, it does not guarantee thread affinity. If the underlying HDC and HBITMAP handles are thread-bound (as is common for GDI objects on Windows), accessing them from a different thread than the one they were created on can lead to undefined behavior. Since Tokio tasks can migrate between threads, this is a significant risk.
| let monitors = Monitor::all() | ||
| .map_err(|e| flow_like_types::anyhow!("Failed to enumerate monitors: {}", e))?; |
There was a problem hiding this comment.
Calling Monitor::all() inside the loop is inefficient as it performs a system-level enumeration of all connected displays on every iteration. It is recommended to move the monitor enumeration outside the loop and reuse the monitor reference.
| let monitor = monitors | ||
| .first() | ||
| .ok_or_else(|| flow_like_types::anyhow!("No monitors found"))?; |
There was a problem hiding this comment.
The monitor input index (evaluated as _monitor) is currently ignored. Use the provided index to select the correct display from the list.
| let monitor = monitors | |
| .first() | |
| .ok_or_else(|| flow_like_types::anyhow!("No monitors found"))?; | |
| let monitor = monitors | |
| .get(_monitor as usize) | |
| .ok_or_else(|| flow_like_types::anyhow!("Monitor index {} not found", _monitor))?; |
| let monitor = monitors | ||
| .first() | ||
| .ok_or_else(|| flow_like_types::anyhow!("No monitors found"))?; |
There was a problem hiding this comment.
The "region" capture type ignores the display_index input. It should respect the selected display index to allow capturing regions on secondary monitors.
| let monitor = monitors | |
| .first() | |
| .ok_or_else(|| flow_like_types::anyhow!("No monitors found"))?; | |
| let monitor = monitors.get(display_index as usize).ok_or_else(|| { | |
| flow_like_types::anyhow!("Display index {} not found", display_index) | |
| })?; |
Up to standards ✅🟢 Issues
|
…y and maintainability