Add access to the Peers tab from the network icon

hebasto commented at 8:28 pm on May 1, 2021: member

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

gui-network-icon

Closes #93.

hebasto added the label UX on May 1, 2021

hebasto added the label Feature on May 1, 2021

hebasto force-pushed on May 1, 2021

in src/qt/bitcoingui.cpp:957 in 1f208f4360 outdated

953+    m_network_context_menu->addAction(
954+        network_active ?
955+            //: A context menu item.
956+            tr("Disable network activity") :
957+            //: A context menu item. The network activity was disabled previously.
958+            tr("Enable network activity again"),

kristapsk commented at 11:30 pm on May 1, 2021:

Maybe not so important, but I think “again” is redundant here, could be just “Enable network activity”.

hebasto commented at 7:15 am on May 2, 2021:

Thanks! Updated.

hebasto force-pushed on May 2, 2021

hebasto commented at 7:15 am on May 2, 2021: member

Updated 1f208f4360df55f18afb89056eb4c18e10f9fd5f -> a7f5ea589fb7a6eddca3e060eade403b2dd6ff87 (pr309.02 -> pr309.03, diff):

addressed @kristapsk’s comment

kristapsk approved

kristapsk commented at 9:48 am on May 2, 2021: contributor

ACK a7f5ea589fb7a6eddca3e060eade403b2dd6ff87

jarolrod commented at 5:31 pm on May 4, 2021: member

tACK a7f5ea589fb7a6eddca3e060eade403b2dd6ff87

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 commented at 5:49 pm on May 4, 2021: member

@Sjors Do you mind looking into this PR?

hebasto requested review from Sjors on May 4, 2021

Sjors approved

Sjors commented at 12:13 pm on May 5, 2021: member

tACK a7f5ea589fb7a6eddca3e060eade403b2dd6ff87

in src/qt/bitcoingui.cpp:589 in a7f5ea589f outdated

583@@ -586,7 +584,10 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel, interfaces::BlockAndH
584         createTrayIconMenu();
585 
586         // Keep up to date with client
587-        updateNetworkState();
588+        setNetworkActive(m_node.getNetworkActive());
589+        connect(connectionsControl, &GUIUtil::ClickableLabel::clicked, [this] {
590+            m_network_context_menu->exec(QCursor::pos());

promag commented at 1:36 pm on May 5, 2021:

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 commented at 6:58 am on May 8, 2021:

@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?

promag commented at 1:36 pm on May 5, 2021: contributor

Tested ACK a7f5ea589fb7a6eddca3e060eade403b2dd6ff87.

gui: Add access to the Peers tab from the network icon d29ea72393

hebasto force-pushed on May 8, 2021

hebasto commented at 6:55 am on May 8, 2021: member

Updated a7f5ea589fb7a6eddca3e060eade403b2dd6ff87 -> d29ea72393ac1d9b32a6976062e9c9fb75876295 (pr309.03 -> pr309.04, diff):

addressed @promag’s comment:

QMenu::exec is blocking.

RandyMcMillan commented at 7:04 am on May 8, 2021: contributor

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

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

hebasto commented at 10:40 am on May 8, 2021: member

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 commented at 10:58 am on May 8, 2021: contributor

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 approved

kristapsk commented at 11:17 am on May 8, 2021: contributor

re-ACK d29ea72393ac1d9b32a6976062e9c9fb75876295

promag commented at 8:42 am on May 10, 2021: contributor

Code review ACK d29ea72393ac1d9b32a6976062e9c9fb75876295.

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.

hebasto renamed this:
~~gui: Add access to the Peers tab from the network icon~~
Add access to the Peers tab from the network icon
on May 10, 2021

Sjors commented at 4:53 pm on May 10, 2021: member

re-ACK d29ea72393ac1d9b32a6976062e9c9fb75876295

hebasto referenced this in commit 86e0f80604 on May 11, 2021

hebasto commented at 5:59 pm on May 11, 2021: member

@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?

hebasto referenced this in commit 5b20cf4236 on May 11, 2021

hebasto merged this on May 31, 2021

hebasto closed this on May 31, 2021

hebasto deleted the branch on May 31, 2021

sidhujag referenced this in commit eec08334c0 on Jun 1, 2021

gwillen referenced this in commit 91b060613d on Jun 1, 2022

bitcoin-core locked this on Aug 16, 2022

Add access to the Peers tab from the network icon #309