Fix multi-monitor detection: periodic monitor list refresh and monito… #35
Conversation
…r-aware screenshot collection
There was a problem hiding this comment.
Pull request overview
Improves robustness of multi-monitor event/screenshot association when displays are connected/disconnected by periodically refreshing the monitor list and preferring monitor-matching screenshots for events.
Changes:
- Refreshes the
screeninfomonitor list periodically during coordinate-to-monitor resolution. - Passes
monitor_indexthrough event aggregation so screenshot selection prefers the event’s monitor. - Updates screenshot selection helpers to filter candidates by
monitor_indexwhen available.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/record/models/event_queue.py |
Prefer screenshots from the same monitor as the input event when selecting “before” and “after” screenshots. |
src/record/handlers/input_event.py |
Periodically refresh the monitor list to handle monitor hot-plug/unplug scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._monitors = list(get_monitors()) | ||
| self._monitors_last_refresh = time.time() | ||
| self._monitors_refresh_interval = 5.0 |
There was a problem hiding this comment.
get_monitors() is called unguarded during initialization, but refreshes are wrapped in a try/except. If screeninfo fails or returns an empty list at startup, the handler can crash before it ever reaches the safer refresh logic. Consider wrapping the initial get_monitors() call similarly and ensuring _monitors is non-empty (fallback to a single synthetic monitor or a safe default).
| except Exception: | ||
| pass | ||
| self._monitors_last_refresh = now | ||
|
|
There was a problem hiding this comment.
The monitor refresh can leave self._monitors empty (e.g., get_monitors() returns []). The rest of _get_monitor assumes at least one monitor exists (uses self._monitors[0] later), which can raise IndexError. Add a guard after refresh (and before the loop) to handle the empty-list case safely.
| # If no monitors are available (e.g., get_monitors() returned an empty list), | |
| # return a safe default monitor description to avoid IndexError. | |
| if not self._monitors: | |
| return 0, {"left": 0, "top": 0, "width": 0, "height": 0} |
| Returns: | ||
| Monitor index (0-based) | ||
| """ | ||
| now = time.time() | ||
| if now - self._monitors_last_refresh > self._monitors_refresh_interval: |
There was a problem hiding this comment.
The _get_monitor docstring/type annotation says it returns a single int monitor index, but the implementation returns a tuple (idx, monitor_dict). Update the return type/docstring to match the actual return value to avoid misleading callers and type checkers.
| def _collect_screenshots(self, timestamp: float, monitor_index: int = None) -> Any: | ||
| """Get screenshot before timestamp, preferring the given monitor.""" | ||
| constants = constants_manager.get() |
There was a problem hiding this comment.
Type hint mismatch: monitor_index defaults to None but is annotated as int. Use Optional[int] (and propagate the same change to _collect_end_screenshot) to reflect actual usage and avoid confusing static analysis.
| if monitor_index is not None: | ||
| matching = [s for s in start_candidates if s.monitor_index == monitor_index] | ||
| if matching: | ||
| return matching[-1] | ||
| return start_candidates[-1] |
There was a problem hiding this comment.
This list comprehension allocates a new list of all matching screenshots just to pick the last one. You can avoid the extra allocation by iterating start_candidates in reverse and returning the first match, which is both faster and uses less memory.
| if monitor_index is not None: | ||
| matching = [s for s in exact_candidates if s.monitor_index == monitor_index] | ||
| if matching: | ||
| return matching[-1] | ||
| return exact_candidates[-1] |
There was a problem hiding this comment.
Same as above: building matching allocates a list when you only need a single element. Iterate exact_candidates in reverse and return the first matching monitor entry to reduce allocations.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as exc: | |
| # Keep using previously cached monitor information if refresh fails. | |
| print(f"Warning: Failed to refresh monitor information: {exc}") |
1 participant
fixes the case where displays are being connected and disconnected. Ties events to displays.