qt: Clean system tray icon menu for '-disablewallet' mode #14383

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:20181003-disablewallet-systray changing 1 files +9 −7

hebasto commented at 4:18 PM on October 3, 2018: member

There is a Debug window leftover in the system tray icon menu after #3392 merging. This PR makes both the app menu and the systray icon menu consistent.

in src/qt/bitcoingui.cpp:631 in 92135c918a outdated

 626 | @@ -627,7 +627,10 @@ void BitcoinGUI::createTrayIconMenu()
 627 |      trayIconMenu->addAction(verifyMessageAction);
 628 |      trayIconMenu->addSeparator();
 629 |      trayIconMenu->addAction(optionsAction);
 630 | -    trayIconMenu->addAction(openRPCConsoleAction);
 631 | +    if(walletFrame)
 632 | +    {

MarcoFalke commented at 9:52 PM on October 3, 2018:

Don't forget to install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to preempt whitespace nitpicking.

hebasto commented at 10:21 PM on October 3, 2018:

Right. Done.

hebasto force-pushed on Oct 3, 2018
fanquake added the label GUI on Oct 3, 2018
DrahtBot commented at 5:30 AM on October 4, 2018: member



Coverage Change (pull 14383) Reference (master)

Lines -0.0132 % 87.0471 %

Functions +0.0000 % 84.1130 %

Branches -0.0104 % 51.5403 %
in src/qt/bitcoingui.cpp:630 in 7a002b4122 outdated
```
 626 | @@ -627,7 +627,9 @@ void BitcoinGUI::createTrayIconMenu()
 627 |      trayIconMenu->addAction(verifyMessageAction);
 628 |      trayIconMenu->addSeparator();
 629 |      trayIconMenu->addAction(optionsAction);
 630 | -    trayIconMenu->addAction(openRPCConsoleAction);
 631 | +    if (walletFrame) {
```
promag commented at 11:28 PM on October 4, 2018:

Why not if (enableWallet) {?

Sjors commented at 4:05 AM on October 5, 2018:

I would prefer if (enableWallet) as well. Using walletFrame seems to be an older convention: https://github.com/bitcoin/bitcoin/commit/b7f4b6d35da7b63529a728eb9333fe424b2a5c6d
promag commented at 11:30 PM on October 4, 2018: member

Tested ACK 7a002b4.
Sjors commented at 4:08 AM on October 5, 2018: member

Before and after screenshots are very useful for this kind of change, especially for concept ACK's (let's see if BitcoinACKS falls for this.

Before: <img width="190" alt="before" src="https://user-images.githubusercontent.com/10217/46515868-55d34880-c8a1-11e8-9109-3404d7d687c9.png">

After: <img width="168" alt="after" src="https://user-images.githubusercontent.com/10217/46515870-5b309300-c8a1-11e8-9fe4-98ba550f60f4.png">

I'm confused: why would you remove the debug window menu item? It would make more sense to me to remove the Send, Receive and Sign Message items (and verify, even though in theory that shouldn't need a wallet).
hebasto commented at 10:40 PM on October 5, 2018: member

@Sjors Thank you for your review.

I'm confused: why would you remove the debug window menu item?

In the node mode: https://github.com/bitcoin/bitcoin/blob/fa7e9694e1ed5406036620d4bf0b72a2aa984578/src/qt/bitcoingui.cpp#L114-L117

and https://github.com/bitcoin/bitcoin/blob/fa7e9694e1ed5406036620d4bf0b72a2aa984578/src/qt/bitcoingui.cpp#L343

effectively disable this slot: https://github.com/bitcoin/bitcoin/blob/fa7e9694e1ed5406036620d4bf0b72a2aa984578/src/qt/bitcoingui.cpp#L662-L668

The Debug window menu item is already removed from the main window Help menu for the node mode: https://github.com/bitcoin/bitcoin/blob/fa7e9694e1ed5406036620d4bf0b72a2aa984578/src/qt/bitcoingui.cpp#L400-L403 and this PR aims to align the system tray icon menu with the main window Help menu.
Clean systray icon menu for -disablewallet mode
Ref #3392
36323e2ac6
hebasto force-pushed on Oct 5, 2018
hebasto commented at 12:12 AM on October 6, 2018: member

All thoughts above are implemented.

Master [wallet mode]

This PR [wallet mode]

Master [node mode]

This PR [node mode] @promag @Sjors please re-review and test on macos.
hebasto renamed this:
~~qt: Extend -disablewallet mode to system tray icon~~
qt: Clean system tray icon menu for '-disablewallet' mode
on Oct 6, 2018
fanquake commented at 7:04 AM on October 9, 2018: member

tACK 36323e2 Tested on macOS 10.14

master (1d14174):

master (1d14174) -disablewallet:

PR (36323e2):

PR (36323e2) -disablwallet:
MarcoFalke assigned jonasschnelli on Oct 9, 2018
promag commented at 7:10 AM on October 9, 2018: member

utACK 36323e2.
Sjors commented at 2:54 AM on October 10, 2018: member

tACK 36323e2
fanquake added this to the "Mergeable" column in a project