Reactive GUI#1572
Conversation
- Yield every 20 rendered rows in the parameter editor to keep the UI responsive during large table builds - Yield every 5 components in the component editor for the same reason
Also guard scroll handling when the canvas has no bbox yet.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Improve Tkinter UI responsiveness and robustness by yielding during long renders and preventing scroll-related crashes when the canvas has no items.
Changes:
- Prevent
ScrollFrame.on_mouse_wheel()from crashing whencanvas.bbox("all")isNone. - Yield periodically during parameter table rendering (
update_idletasks()every 20 rows). - Yield periodically during component frame population (
update_idletasks()every 5 components) and add tests for the new behaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
ardupilot_methodic_configurator/frontend_tkinter_scroll_frame.py |
Guard against bbox() returning None during mouse wheel scrolling. |
ardupilot_methodic_configurator/frontend_tkinter_parameter_editor_table.py |
Periodically call update_idletasks() while rendering many rows. |
ardupilot_methodic_configurator/frontend_tkinter_component_editor_base.py |
Periodically call update_idletasks() while creating many component widgets. |
tests/test_frontend_tkinter_scroll_frame.py |
Add regression test for scrolling before any canvas items exist. |
tests/test_frontend_tkinter_parameter_editor_table.py |
Add test asserting periodic yielding during rendering. |
tests/test_frontend_tkinter_component_editor_base.py |
Add test asserting periodic yielding during component population. |
| canvas_height = self.canvas.winfo_height() | ||
| rows_height = self.canvas.bbox("all")[3] | ||
| bbox = self.canvas.bbox("all") | ||
| rows_height = bbox[3] if bbox is not None else 0 |
There was a problem hiding this comment.
canvas.bbox('all') returns (x1, y1, x2, y2), so bbox[3] is a coordinate, not the content height. This can mis-detect overflow when y1 is not 0. Compute the height as (bbox[3] - bbox[1]) (and keep the None guard) to ensure scrolling is enabled/disabled correctly.
| rows_height = bbox[3] if bbox is not None else 0 | |
| rows_height = (bbox[3] - bbox[1]) if bbox is not None else 0 |
| should_try_to_display_bitmask_parameter_editor_usage = True | ||
| self._grid_column_widgets(row_widgets, i, show_upload_column) | ||
| if i % 20 == 0: | ||
| self.view_port.update_idletasks() # yield to the event loop periodically to keep the UI responsive |
There was a problem hiding this comment.
update_idletasks() only processes geometry redraw/idle tasks; it typically does not process pending input events, so it may not achieve the stated goal of yielding to the Tk event loop during long renders. If the intent is to keep the UI responsive to user actions, consider chunking the render via after(...) (preferred to avoid re-entrancy) or using update() with care (risk: re-entrant event handling while state is mid-update).
| self.view_port.update_idletasks() # yield to the event loop periodically to keep the UI responsive | |
| self.view_port.update() # process pending UI events periodically so the interface remains responsive during long renders |
| for i, (key, value) in enumerate(components.items(), 1): | ||
| self.add_widget(self.scroll_frame.view_port, key, value, []) | ||
| if i % 5 == 0: # yield to the event loop periodically to keep the UI responsive | ||
| self.scroll_frame.view_port.update_idletasks() |
There was a problem hiding this comment.
Calling update_idletasks() every 5 components can become a noticeable overhead for large component sets. Consider using a larger batch size and/or throttling based on elapsed time (e.g., yield every N ms) so performance scales better while still maintaining responsiveness.
| } | ||
|
|
||
| with ( | ||
| patch.object(parameter_editor_table, "_create_column_widgets", return_value=[MagicMock() for _ in range(7)]), |
There was a problem hiding this comment.
This patch returns the same list instance (and the same widget mocks) for every _create_column_widgets() call, which can make the test brittle if the production code mutates the returned list or expects distinct widget instances per row. Prefer using a side_effect that returns a fresh list of new mocks each call.
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Description
yield to Tk event loop during table/component rendering
Checklist
git commit --signoff)Testing
Describe how you tested these changes: