Disconnect qt signals in qt implementation of pyface.tasks #542
Conversation
This file contains 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
added 2 commits
June 12, 2020 11:16
Codecov Report
@@ Coverage Diff @@
## master #542 +/- ##
==========================================
+ Coverage 36.99% 37.85% +0.85%
==========================================
Files 470 470
Lines 26027 26134 +107
Branches 3961 3971 +10
==========================================
+ Hits 9629 9892 +263
+ Misses 15977 15809 -168
- Partials 421 433 +12
Continue to review full report at Codecov.
|
kitchoi
reviewed
Jun 15, 2020
kitchoi
reviewed
Jun 15, 2020
kitchoi
reviewed
Jun 18, 2020
|
Looks like we have disturbed the balance of the forces again... :( There are a number of error traceback following some of the tests that have nothing to do with any of the widgets, e.g.: pyface/pyface/ui/qt4/util/tests/test_gui_test_assistant.py Lines 87 to 89 in 0104cf7 Those tests did not fail, though. :/ And there are a couple that actually register as test failure and cause the CI build to fail: |
|
I think both of the tracebacks are caused by |
|
Closing and reopening to restart CI jobs |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Part of #258
Distinct things this PR does:
DockPane- disconnects signals from its control. Low risk - low reward kind of change. Doesn't hurt to add this.Deletes connections to shortcuts via
_connections_to_removein classes providingIEditorAreaPane. It is good to disconnect these shortcuts because if something goes wrong and the objects are not deleted, the handlers can still be activated via keyboard events. So the reward is fairly high. The risk with current code is low because it is only disconnecting what is added to the list and there are no other disconnects. The problems might arise if someone decides to disconnect the shortcuts elsewhere and doesn't remove them from the list. But maybe that should be considered a case of "Don't do that".Removes reference to
active_tabwidgetfromSplitEditorAreaPane. This was preventing the widget from being deleted together with the main control. Low risk - high reward kind of change. Although not directly related to signals (and thus can be separated from this).EditorAreaPane- disconnects signals from its control. The handlers do work with UI elements accessible via control but then the signals are emitted from the same control. If control is set toNonebetween the signal was emitted and slot was executed, there could be problems. So I would say that this is a low risk - low medium reward kind of change.Removes reference to
active_editorfromAdvancedEditorAreaPane. The editor here is a python object so holding a reference to it doesn't really do anything (its control is destroyed separately). But other classes implementingIEditorAreaPaneare removing this reference viaremove_editormethod, so I added that line of code here as well. No risk - no reward kind of change. Done for consistency.AdvancedEditorAreaPane- tells its control to disconnect what it had connected. In this case it had connected a slot tofocusChangedsignal of the main application, so this is important to disconnect. Low risk - high reward kind of change.When
EditorAreaWidgetdestroys andeditor_widget, it also tells the widget to disconnect what it had connected. The signals and slots belong to the same object but in the slot the objects tries to access an attribute on the parent, which might cause problems. Low risk - medium reward.After disconnecting its own signals,
EditorWidgettells itstitlebarWidget(EditorTitleBarWidget) to disconnect what it had connected. The issue here is thatEditorTitleBarWidgetdoesn't keep a reference to the slot it connected its signal to. The slot can be accessed fromEditorWidgetso it is easy to pass that in. However, an assumption has been made here that theeditorof a certaineditor_widgetdoesn't change. Making assumptions here is quite risky and this disconnect doesn't cover the case where newEditorTitleBarWidgetis set as theTitleBarWidgetand the old one is discarded. I'm not sure how qt handles disposal in that case but I assume that it destroys the old object and by doing so destroys the connections as well. So given that this addeddisconnectcovers only a small case that is probably handled as expected by qt, this is a high risk - low benefit kind of change.Connections that this PR doesn't remove:
In
childEventmethod ofEditorAreaWidget(control ofAdvanced EditorAreaPane) a couple of slots are connected to a couple of signals of the child. I couldn't find an easy way to disconnect these signals, so I didn't add these changes. But given that this is a child signal connected to a parent slot and both are qt objects, qt should handle this situation well on its own.DraggableTabWidgetthat are children ofSplitEditorAreaPanecontrol make a couple of connections in their__init__. The control can have a tree hierarchy structure withDraggableTabWidgets so there's no easy way to remove these connections. But given that both signals and slots belong to the same object, they shouldn't cause any problems.In
ceate_empty_widgetofDraggableTabWidgetbuttons are created and their signals are connected to callbacks provided in topSplitEditorAreaPane. Again, there's no easy way to remove these connections. I think these connection can be problematic and would say that removing them would be a medium reward. But I think the code needed to disconnect them would be much riskier. I would like to hear more opinions on this.