Small follow-up to #751.
Fixes another crash cause during shutdown. Which occurs when the user hovers over the wallets list.
Future Note: This surely happen in other places as well, we should re-work the way we connect signals. Register lambas without any precaution can leave dangling pointers.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hebasto |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Future Note: This surely happen in other places as well, we should re-work the way we connect signals. Register lambas without any precaution can leave dangling pointers.
Wouldn’t be enough to use an QObject::connect overload with the additional context parameter, i.e.
0QObject::connect(const QObject* sender, PointerToMemberFunction signal, const QObject* context, Functor functor, Qt::ConnectionType type)
instead of
0QObject::connect(const QObject* sender, PointerToMemberFunction signal, Functor functor)
The former assumes that:
The connection will automatically disconnect if the sender or the context is destroyed. However, you should take care that any objects used within the functor are still alive when the signal is emitted.
Also see a discussion in #680.
Addressing potential crashes during shutdown. The most
noticeable one can be triggered by hovering over the
wallet list as the app shuts down.
Opening the top bar menu when the app is being destroyed
freezes the GUI shutdown process for no reason. No menu
action can be executed.
Note:
This behavior is consistent with how the tray icon menu
is cleared too.
Future Note: This surely happen in other places as well, we should re-work the way we connect signals. Register lambas without any precaution can leave dangling pointers.
Wouldn’t be enough to use an
QObject::connectoverload with the additionalcontextparameter, i.e.0QObject::connect(const QObject* sender, PointerToMemberFunction signal, const QObject* context, Functor functor, Qt::ConnectionType type)instead of
0QObject::connect(const QObject* sender, PointerToMemberFunction signal, Functor functor)The former assumes that:
The connection will automatically disconnect if the sender or the context is destroyed. However, you should take care that any objects used within the functor are still alive when the signal is emitted.
Also see a discussion in #680.
Tackled. Thanks.
I was a bit doubtful initially because we are manually deleting the wallet_controller field instead of letting QT do it for us.. but, thanks to the QObject::destroyed signal, this works fine.
Other than that, while testing the changes, I saw that the GUI shutdown process was being frozen when the user hovers over the non-triggerable top bar menu actions. So, the second commit also fixes this by clearing the menu (same as we do with the tray icon menu).
649@@ -650,7 +650,8 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel, interfaces::BlockAndH
650
651 m_mask_values_action->setChecked(_clientModel->getOptionsModel()->getOption(OptionsModel::OptionID::MaskValues).toBool());
652 } else {
653- if(trayIconMenu)
654+ // Shutdown requested, disable menus
655+ if (trayIconMenu)
656 {
style nit:
0 if (trayIconMenu) {