Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disconnect controller qt connections #4476

Merged
merged 5 commits into from
Nov 5, 2021
Merged

Disconnect controller qt connections #4476

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
146fcb3
Disconnect connections from controllers when deleting controller prefs
ywwg Oct 23, 2021
2c800c3
Controller disconnect: be more restrictive about what we disconnect
ywwg Oct 23, 2021
7b595c0
controller disconnect: store an explicit list of connections
ywwg Oct 23, 2021
ab40243
controller disconnect: use the list of controllers to determine what …
ywwg Oct 25, 2021
eb937bb
controller disconnect : Make sure to associate the connection with th…
ywwg Oct 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/controllers/dlgprefcontrollers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,10 @@ void DlgPrefControllers::rescanControllers() {
}

void DlgPrefControllers::destroyControllerWidgets() {
for (auto conn : m_controllerConnections) {
disconnect(conn);
}
m_controllerConnections.clear();
while (!m_controllerPages.isEmpty()) {
DlgPrefController* pControllerDlg = m_controllerPages.takeLast();
m_pDlgPreferences->removePageWidget(pControllerDlg);
Expand Down Expand Up @@ -171,11 +175,11 @@ void DlgPrefControllers::setupControllerWidgets() {

m_controllerPages.append(pControllerDlg);

connect(pController,
m_controllerConnections.append(connect(pController,
&Controller::openChanged,
[this, pControllerDlg](bool bOpen) {
slotHighlightDevice(pControllerDlg, bOpen);
});
}));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should always pass a context object as third parameter (this).

If we do that we can probably get rid of the list, and just call disconnect(pController, nullptr, this, nullptr) to remove all connections between a controller and the dialog.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that at first, but I was worried that the call to enumerate the list of controllers would be incorrect at the time of destruction. And since disconnect requires the sender object to be specified, we need to make sure that list is complete. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm so it looks like because we don't do hotplug, updateControllerList is only called once, so the list of controllers should be static. However once we go to hotplug, we will have big problems with inconsistency between dlgprefcontrollers and the controllermanager. (And these inconsistencies could easily cause crashes). For now, I'll eliminate the list and trust the the controller list doesn't change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. This is safe code for now, but may not be in the future. Added some comments as hints to future developers trying to add hotplug so they don't trip over this code. (There is no way to write this code currently that would be future-proof)

Copy link
Member Author

@ywwg ywwg Oct 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I understand your comment now. I don't know how to do what you describe though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh I figured out which third parameter you meant. got it


QTreeWidgetItem* pControllerTreeItem = new QTreeWidgetItem(
QTreeWidgetItem::Type);
Expand Down
1 change: 1 addition & 0 deletions src/controllers/dlgprefcontrollers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,5 @@ class DlgPrefControllers : public DlgPreferencePage, public Ui::DlgPrefControlle
QTreeWidgetItem* m_pControllersRootItem;
QList<DlgPrefController*> m_controllerPages;
QList<QTreeWidgetItem*> m_controllerTreeItems;
QList<QMetaObject::Connection> m_controllerConnections;
};