Split event handlers by wallet #13070

Currently it's not possible to disconnect an event handler of a particular wallet. With this change, event handlers are associated with the corresponding wallet. It is then possible to disconnect all handlers of a particular wallet. There is no behavior change. This is a small step towards dynamic wallet unloading.

@ryanofsky care to review?

@ryanofsky care to review?

Change seems harmless but I don't understand why it makes sense. Is it part of a larger set of changes that I can look at or part of a larger design that you can describe somewhere?

The map doesn't seem to make sense for unloading because wallet interfaces are created on the fly and you can't identify them by pointer. Maybe the map is supposed to be keyed by wallet name? The list of handlers doesn't seem to make sense because there is only one handler. I also think it would probably be better and less duplicative if splashscreen were not managing any interface pointers at all, but just creating a slot which wallets in m_wallet_models could send signals to.

Thanks @ryanofsky for the feedback.

I also think it would probably be better and less duplicative if splashscreen were not managing any interface pointers at all

The subscribeToCoreSignals/unsubscribeFromCoreSignals is used in several places. I can remove it from splash screen but let's discuss it a different PR or IRL.

you can't identify them by pointer

The list of handlers doesn't seem to make sense because there is only one handler

Consider the following example:

void SplashScreen::ConnectWallet(std::unique_ptr<interfaces::Wallet> wallet)
{
    auto i = m_connected_wallets.emplace(std::move(wallet), std::list<std::unique_ptr<interfaces::Handler>>()).first;
    i->second.emplace_back(wallet->handleShowProgress(boost::bind(ShowProgress, this, _1, _2, false)));
    i->second.emplace_back(wallet->handleAboutToRemove([this, i]() {
        for (auto& handler : i->second) {
            handler->disconnect();
        }
        m_connected_wallets.erase(i);
    }));
}

Which uses the iterator i in the callback. With this example does it make more sense?

Note that map iterators aren't invalidated after insertions or erasures, so this is safe.

Maybe the map is supposed to be keyed by wallet name?

Maybe, I don't know if that's acceptable.

With this example does it make more sense?

No, because there is no map lookup. It would seem more appropriate to use list<<pair<>> instead of map<> if there are no lookups comparing map keys. But it is hard to evaluate this when there is no description of the overall design motivating this change and no justification for why a splash screen wallet data structure should exist at all.

I don't think I can productively review these small factoring PRs without some kind of description of the larger picture they fit into. If it is not possible to describe smaller refactoring changes coherently, I'd prefer to just look at a larger PR or branch which implements real functionality, so I can understand how the changes fit together.

@ryanofsky thanks for the insights.

I'd prefer to just look at a larger PR

please see #13111.