Fix: Connect cancel button to self.reject in ProgressDialog (Fixes #135)#1288
Fix: Connect cancel button to self.reject in ProgressDialog (Fixes #135)#1288MohamedSharfan wants to merge 1 commit intoenthought:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug in the Qt backend's ProgressDialog where the Cancel button would close the dialog window but fail to properly set the _user_cancelled flag, causing the cancellation to not be recognized by the application.
Changes:
- Modified the Cancel button connection to call the custom
self.rejectmethod instead of the defaultdialog.reject - Added an optional
eventparameter to therejectmethod to handle signal inputs
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| buttons.rejected.connect(dialog.reject) | ||
| buttons.rejected.connect(self.reject) | ||
| self._connections_to_remove.append( | ||
| (buttons.rejected, dialog.reject) |
There was a problem hiding this comment.
The handler stored in _connections_to_remove should be self.reject to match the actual connected handler on line 204. Currently, dialog.reject is stored, which means the signal disconnection in the destroy() method will attempt to disconnect the wrong handler, potentially leaving the connection active and causing memory leaks.
| (buttons.rejected, dialog.reject) | |
| (buttons.rejected, self.reject) |
| # ------------------------------------------------------------------------- | ||
|
|
||
| def reject(self, event): | ||
| def reject(self, event = None): |
There was a problem hiding this comment.
There should be no space before the equal sign in the default parameter assignment. According to PEP 8, the correct format is "event=None" not "event = None".
| if self.can_cancel: | ||
| buttons.rejected.connect(dialog.reject) | ||
| buttons.rejected.connect(self.reject) | ||
| self._connections_to_remove.append( | ||
| (buttons.rejected, dialog.reject) | ||
| ) |
There was a problem hiding this comment.
This PR fixes a critical bug where the Cancel button doesn't properly set the _user_cancelled flag, but there's no test coverage for this behavior. Consider adding a test that verifies clicking the cancel button properly sets _user_cancelled to True and that the update method returns the correct cancellation status.
Fixes issue #135 where the Cancel button in ProgressDialog (Qt backend) would close the window but fail to update the _user_cancelled flag.
Changes
Updated _create_buttons to connect rejected signal to self.reject instead of dialog.reject.
Updated reject method signature to accept optional event argument to handle signal inputs.
Verification Ran python -m unittest pyface.ui.qt4.tests.test_progress_dialog and confirmed tests pass.