Add access to the Peers tab from the network icon

[hebasto]

This PR add a small context menu to the network activity icon that provides an access to the Peers tab:

Closes #93.

[hebasto]

Updated 1f208f4 -> a7f5ea5 (pr309.02 -> pr309.03, diff):

• addressed @kristapsk's comment

[kristapsk]

ACK a7f5ea5

[jarolrod]

tACK a7f5ea5

Tested on macOS 11.3 Qt 5.15.2, Also tested with -disablewallet=1.

This is ok to merge as-is. But, wanted to document how I think this should be changed around as part of a follow-up.

The 'Peers Tab' is for seeing your connected Peers and interacting with them, such as banning or disconnecting a peer.

The idea of "Disabling Network Activity" should be tied to the 'Network Traffic Tab' not the 'Peers Tab'. This means we could add a "Disable Network Activity" button to the Network Traffic tab and a 'Network Traffic' widget next to the 'Connection Count' widget. Pressing this 'Network Traffic' widget would open the 'Network Traffic' tab where the user has the option of disabling network activity.

[hebasto]

@Sjors Do you mind looking into this PR?

[Sjors]

tACK a7f5ea5

[promag]

Tested ACK a7f5ea5.

[promag]

QMenu::exec is blocking. If you want to keep this approach then you could declare a local QMenu instance instead - this way you can ditch the class member and the call to .clear().

[hebasto]

@promag

Thanks!

QMenu::exec is blocking.

My bad! Fixed.

If you want to keep this approach then you could declare a local QMenu instance instead - this way you can ditch the class member and the call to .clear().

How the lifetime of local QMenu instances could be managed in a proper way?

[hebasto]

Updated a7f5ea5 -> d29ea72 (pr309.03 -> pr309.04, diff):

• addressed @promag's comment:

QMenu::exec is blocking.

[RandyMcMillan]

It would be great if a key combo was added to this functionality...
Maybe:

COMMAND + OPTION <W> or <N> would work.

[hebasto]

It would be great if a key combo was added to this functionality...
Maybe:

COMMAND + OPTION <W> or <N> would work.

For switching to the Peers tab we already have the Ctrl+P shortcut.
Adding a shortcut for enabling/disabling the network activity is out of this PR scope.

[kristapsk]

Adding a shortcut for enabling/disabling the network activity

I'm not even sure such shortcut is a good idea at all. Could accidentally hit that, the same that has happened to more with that icon without this PR.

[kristapsk]

re-ACK d29ea72

[promag]

Code review ACK d29ea72.

Not fond of the approach though. It seems it would be enough to update the menu item text on BitcoinGUI::updateNetworkState() instead of QMenu::clear() + QMenu::addAction() and the slot would just query the current network state.

It also seems more elegant to just call BitcoinGUI::updateNetworkState() in BitcoinGUI::setClientModel which already calls m_node.getNetworkActive().

Worth noting that when the toggle action is triggered it calls m_node.setNetworkActive() which results in destroying that same action due to QMenu::clear().

Don't mind my feedback, it's just another way of implementing the feature, thanks for adding the feature and addressing my previous comment.

[Sjors]

re-ACK d29ea72

[hebasto]

@promag

Many thanks for your valuable review and feedback!

Not fond of the approach though. It seems it would be enough to update the menu item text on BitcoinGUI::updateNetworkState() instead of QMenu::clear() + QMenu::addAction() and the slot would just query the current network state.

It also seems more elegant to just call BitcoinGUI::updateNetworkState() in BitcoinGUI::setClientModel which already calls m_node.getNetworkActive().

Worth noting that when the toggle action is triggered it calls m_node.setNetworkActive() which results in destroying that same action due to QMenu::clear().

Do you mind having a look at https://github.com/hebasto/gui/commits/210511-pr309-alt?