Skip to content

Commit f1ad680

Browse files
yashhzdamilcarlucas
authored andcommitted
fix(progress): use update_idletasks() to avoid reentrant event dispatch
ProgressWindow.update_progress_bar() and the inline progress bar in FlightControllerInfoWindow.update_progress_bar() both called self.progress_bar.update() to refresh the display. tkinter's update() pumps the entire event queue, including any user clicks that arrived against other windows while a long blocking call (parameter upload, FC connection, parameter download, motor reset) was holding the main thread. Those queued click handlers fire reentrantly while the caller is still mid-operation, which can: - retrigger an upload/download whose state machine is not yet idle - close or rebuild widgets the caller still holds references to - dispatch destructor paths on a window the caller will dereference Switch both call sites to update_idletasks(), which only flushes geometry/repaint events and does not run user-event handlers. The visible refresh of the bar and label is unchanged because progress-bar redraw is an idle task. Existing test_user_sees_progress_window_handle_widget_update_errors is updated to mock update_idletasks. Added test_progress_updates_do_not_pump_user_events as a regression guard so a future change cannot silently restore update(). Signed-off-by: Yash Goel <yashvardhan664@gmail.com>
1 parent cd88057 commit f1ad680

3 files changed

Lines changed: 40 additions & 6 deletions

File tree

ardupilot_methodic_configurator/frontend_tkinter_flightcontroller_info.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,12 @@ def update_progress_bar(self, current_value: int, max_value: int) -> None:
153153
progress_message = _("Downloaded {} of {} parameters").format(current_value, max_value)
154154
self.progress_label.config(text=progress_message)
155155

156-
# Update the display
157-
self.progress_bar.update()
156+
# Update the display. update_idletasks() repaints the bar/label
157+
# without re-entering the event loop; the plain update() variant
158+
# would pump the full event queue (including queued user clicks
159+
# against other windows) while the caller is still in the middle
160+
# of a blocking parameter download.
161+
self.progress_bar.update_idletasks()
158162
self.root.update_idletasks()
159163

160164
# Hide progress bar when complete

ardupilot_methodic_configurator/frontend_tkinter_progress_window.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,11 @@ def __init__( # pylint: disable=too-many-arguments, too-many-positional-argumen
7575
# Show the window now that it's properly positioned
7676
self.progress_window.lift()
7777
self._shown = True
78-
self.progress_bar.update()
78+
# Use update_idletasks() rather than update(): the latter pumps the
79+
# full event queue (including queued mouse clicks against other
80+
# windows) and can re-enter user-event handlers while the caller
81+
# is still in the middle of a blocking I/O operation.
82+
self.progress_bar.update_idletasks()
7983

8084
def _center_progress_window(self) -> None:
8185
"""
@@ -133,7 +137,11 @@ def update_progress_bar(self, current_value: int, max_value: int) -> None:
133137
# Update the progress message
134138
self.progress_label.config(text=self.message.format(current_value, max_value))
135139

136-
self.progress_bar.update()
140+
# update_idletasks() repaints the bar/label without re-entering
141+
# the event loop. The plain update() variant processes pending
142+
# user events (clicks, keypresses) which can fire callbacks on
143+
# other windows while a blocking upload/download is in flight.
144+
self.progress_bar.update_idletasks()
137145

138146
# Close the progress window when the process is complete
139147
if current_value == max_value:

tests/test_frontend_tkinter_progress_window.py

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -247,8 +247,8 @@ def test_user_sees_progress_window_handle_widget_update_errors(self, progress_wi
247247
WHEN: Progress updates fail due to widget errors
248248
THEN: Errors are logged but no exceptions are raised
249249
"""
250-
# Mock progress_bar.update to raise TclError
251-
progress_window.progress_bar.update = MagicMock(side_effect=tk.TclError("Widget destroyed"))
250+
# Mock progress_bar.update_idletasks to raise TclError
251+
progress_window.progress_bar.update_idletasks = MagicMock(side_effect=tk.TclError("Widget destroyed"))
252252

253253
with patch("ardupilot_methodic_configurator.frontend_tkinter_progress_window.logging_error") as mock_logging:
254254
# This should not raise an exception
@@ -258,6 +258,28 @@ def test_user_sees_progress_window_handle_widget_update_errors(self, progress_wi
258258
mock_logging.assert_called_once()
259259
assert "Updating progress widgets" in mock_logging.call_args[0][0]
260260

261+
def test_progress_updates_do_not_pump_user_events(self, progress_window) -> None:
262+
"""
263+
Progress updates must use update_idletasks(), not update().
264+
265+
GIVEN: A blocking I/O operation (param upload, FC connection, etc.) is
266+
running on the main thread and periodically calling
267+
update_progress_bar to refresh the bar.
268+
WHEN: The progress window is initialised and then updated.
269+
THEN: Only update_idletasks() is called on the progress bar. The
270+
full update() variant pumps the entire event queue and would
271+
re-dispatch user clicks that arrived during the blocking call,
272+
allowing reentrant button callbacks while the caller is still
273+
in the middle of upload/connection logic.
274+
"""
275+
progress_window.progress_bar.update = MagicMock()
276+
progress_window.progress_bar.update_idletasks = MagicMock()
277+
278+
progress_window.update_progress_bar(25, 100)
279+
280+
progress_window.progress_bar.update.assert_not_called()
281+
progress_window.progress_bar.update_idletasks.assert_called()
282+
261283
def test_user_sees_progress_window_handle_lazy_window_relift(self, progress_window) -> None:
262284
"""
263285
User sees progress window handle relifting for already shown windows.

0 commit comments

Comments
 (0)