Diagnostics update #1174
Merged
Diagnostics update #1174
Conversation
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
Add a Reset button and logic to the diagnostics popup to clear/ignore prior data and only display logs collected after the reset. Controller: import time, track reset_timestamp, add reset_plots() and _filter_log_since_reset() to filter performance logs, call populate_plots() after reset, and make plot initialization use the dynamic number of label frames. View: add Reset HoverButton and adjust grid columns/plot count/layout to accommodate the new button. Log functions: handle empty date_dirs in load_performance_log() by returning None early. UI/layout tweaks and minor defensive checks included.
Add a new "Performance Diagnostics" section to the user interface walkthrough (docs/source/02_user_guide/03_user_interface_walkthrough/user_interface_walkthrough.rst) and include the accompanying image (images/performance_diagnostics.png). The section documents launching the Performance Diagnostics window (File → Performance Diagnostics), the Update/Reset/Save Image/Close actions, and the timing distribution plots displayed (image acquisition, display, histogram population, DAQ trigger timing, stage position queries, Z/F stage moves, and serial communication).
Contributor
There was a problem hiding this comment.
Pull request overview
Improves the Performance Diagnostics popup by adding a plot reset capability and making screenshot capture more reliable on Windows multi-monitor setups.
Changes:
- Added a Reset button to the Diagnostics popup and corresponding controller logic to filter performance logs after a reset timestamp.
- Updated screenshot capture to temporarily lift/set
-topmostand, on Windows, useImageGrab.grab(all_screens=True)with a fallback forinclude_layered_windows. - Added tests for diagnostics popup layout and controller behaviors; added a small guard in performance log loading when no log directories exist; documented the UI.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/view/popups/test_diagnostics_popup.py | New UI/layout assertions for the Diagnostics popup (buttons + plot widgets). |
| test/controller/sub_controllers/test_diagnostics_popup.py | New controller tests for reset behavior, log filtering, close behavior, and no-data handling. |
| src/navigate/view/popups/diagnostics_popup.py | Added Reset button and adjusted layout/plot count to 7 panels. |
| src/navigate/log_files/log_functions.py | Returns None when no dated log directories exist (avoids max([]) crash). |
| src/navigate/controller/sub_controllers/diagnostics_popup.py | Added reset timestamp filtering and improved screenshot capture behavior for Windows/multi-monitor. |
| docs/source/02_user_guide/03_user_interface_walkthrough/user_interface_walkthrough.rst | Added “Performance Diagnostics” documentation section with screenshot and control descriptions. |
| .github/AGENTS.md | Updated recommended pytest invocation (PYTHONPATH-based). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+157
to
+202
| was_topmost = False | ||
| try: | ||
| try: | ||
| was_topmost = bool(int(self.view.popup.attributes("-topmost"))) | ||
| except Exception: | ||
| was_topmost = False | ||
|
|
||
| # Ensure the diagnostics popup is on top before capture. | ||
| self.view.popup.lift() | ||
| self.view.popup.attributes("-topmost", 1) | ||
| self.view.popup.update_idletasks() | ||
|
|
||
| # Get the window geometry | ||
| x = self.view.diagnostics_frame.winfo_rootx() | ||
| y = self.view.diagnostics_frame.winfo_rooty() | ||
| width = self.view.diagnostics_frame.winfo_width() | ||
| height = self.view.diagnostics_frame.winfo_height() | ||
|
|
||
| if width <= 1 or height <= 1: | ||
| self.view.popup.update_idletasks() | ||
| width = self.view.diagnostics_frame.winfo_width() | ||
| height = self.view.diagnostics_frame.winfo_height() | ||
|
|
||
| bbox = (x, y, x + width, y + height) | ||
| grab_kwargs = {} | ||
| if os.name == "nt": | ||
| grab_kwargs["all_screens"] = True | ||
| try: | ||
| grab_kwargs["include_layered_windows"] = True | ||
| screenshot = ImageGrab.grab(bbox=bbox, **grab_kwargs) | ||
| except TypeError: | ||
| grab_kwargs.pop("include_layered_windows", None) | ||
| screenshot = ImageGrab.grab(bbox=bbox, **grab_kwargs) | ||
| else: | ||
| screenshot = ImageGrab.grab(bbox=bbox) | ||
|
|
||
| screenshot.save(file_path) | ||
| logger.info(f"Diagnostics screenshot saved to: {file_path}") | ||
| except Exception: | ||
| logger.exception("Failed to capture diagnostics screenshot.") | ||
| finally: | ||
| if not was_topmost: | ||
| try: | ||
| self.view.popup.attributes("-topmost", 0) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
New screenshot logic in _take_screenshot (Windows all_screens / include_layered_windows fallback + temporary -topmost) isn’t covered by tests. Since test/controller/sub_controllers/test_diagnostics_popup.py already exercises this controller, add a unit test that patches PIL.ImageGrab.grab and asserts: (1) on os.name == "nt", all_screens=True is passed and the include_layered_windows TypeError fallback is used; (2) -topmost is restored to its prior value in finally; and (3) save() is called with the expected path without touching the real filesystem.
| try: | ||
| self.view.popup.attributes("-topmost", 0) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Suggested change
| pass | |
| logger.debug( | |
| "Failed to reset '-topmost' attribute on diagnostics popup.", | |
| exc_info=True, | |
| ) |
| try: | ||
| if controller.view.popup.winfo_exists(): | ||
| controller.view.popup.destroy() | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Suggested change
| except Exception: | |
| except Exception: | |
| # Ignore any errors during popup teardown to avoid spurious test failures. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1174 +/- ##
===========================================
+ Coverage 52.51% 52.85% +0.33%
===========================================
Files 197 197
Lines 23976 24023 +47
===========================================
+ Hits 12592 12698 +106
+ Misses 11384 11325 -59
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
PR Summary
This PR improves the Diagnostics popup screenshot capture, with a focus on Windows multi‑monitor behavior. On Windows, ImageGrab.grab() defaults to the primary display, which produced empty screenshots when the popup was moved to a secondary monitor, and the main GUI window could appear above the popup in the captured image.
What changed
Motivation
Users on Windows with multiple displays reported blank screenshots when the diagnostics popup was moved off the primary monitor, and screenshots that were partially obscured by the main window. This update makes capture reliable across multi‑monitor setups while keeping behavior unchanged on other platforms.