New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Oct 16, 2018

Conversation

Projects
None yet
8 participants
@hebasto
Copy link
Contributor

hebasto commented Oct 3, 2018

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.

@@ -627,7 +627,10 @@ void BitcoinGUI::createTrayIconMenu()
trayIconMenu->addAction(verifyMessageAction);
trayIconMenu->addSeparator();
trayIconMenu->addAction(optionsAction);
trayIconMenu->addAction(openRPCConsoleAction);
if(walletFrame)
{

This comment has been minimized.

@MarcoFalke

MarcoFalke Oct 3, 2018

Member

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.

This comment has been minimized.

@hebasto

hebasto Oct 3, 2018

Contributor

Right. Done.

@hebasto hebasto force-pushed the hebasto:20181003-disablewallet-systray branch from 92135c9 to 7a002b4 Oct 3, 2018

@fanquake fanquake added the GUI label Oct 3, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 4, 2018

Coverage Change (pull 14383) Reference (master)
Lines -0.0132 % 87.0471 %
Functions +0.0000 % 84.1130 %
Branches -0.0104 % 51.5403 %
@promag
Copy link
Member

promag left a comment

Tested ACK 7a002b4.

@@ -627,7 +627,9 @@ void BitcoinGUI::createTrayIconMenu()
trayIconMenu->addAction(verifyMessageAction);
trayIconMenu->addSeparator();
trayIconMenu->addAction(optionsAction);
trayIconMenu->addAction(openRPCConsoleAction);
if (walletFrame) {

This comment has been minimized.

@promag

promag Oct 4, 2018

Member

Why not if (enableWallet) {?

This comment has been minimized.

@Sjors

Sjors Oct 5, 2018

Member

I would prefer if (enableWallet) as well. Using walletFrame seems to be an older convention: b7f4b6d

@Sjors
Copy link
Member

Sjors left a comment

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:
before

After:
after

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).

@@ -627,7 +627,9 @@ void BitcoinGUI::createTrayIconMenu()
trayIconMenu->addAction(verifyMessageAction);
trayIconMenu->addSeparator();
trayIconMenu->addAction(optionsAction);
trayIconMenu->addAction(openRPCConsoleAction);
if (walletFrame) {

This comment has been minimized.

@Sjors

Sjors Oct 5, 2018

Member

I would prefer if (enableWallet) as well. Using walletFrame seems to be an older convention: b7f4b6d

@hebasto

This comment has been minimized.

Copy link
Contributor

hebasto commented Oct 5, 2018

@Sjors
Thank you for your review.

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

In the node mode:

/* When compiled without wallet or -disablewallet is provided,
* the central widget is the rpc console.
*/
setCentralWidget(rpcConsole);

and

connect(openRPCConsoleAction, &QAction::triggered, this, &BitcoinGUI::showDebugWindow);

effectively disable this slot:

void BitcoinGUI::showDebugWindow()
{
rpcConsole->showNormal();
rpcConsole->show();
rpcConsole->raise();
rpcConsole->activateWindow();
}

The Debug window menu item is already removed from the main window Help menu for the node mode:

if(walletFrame)
{
help->addAction(openRPCConsoleAction);
}

and this PR aims to align the system tray icon menu with the main window Help menu.

@hebasto hebasto force-pushed the hebasto:20181003-disablewallet-systray branch from 7a002b4 to 36323e2 Oct 5, 2018

@hebasto

This comment has been minimized.

Copy link
Contributor

hebasto commented Oct 6, 2018

All thoughts above are implemented.

Master [wallet mode]
tray_wallet_before

This PR [wallet mode]
tray_wallet_after2

Master [node mode]
tray_node_before

This PR [node mode]
tray_node_after2

@promag @Sjors please re-review and test on macos.

@hebasto hebasto changed the title qt: Extend -disablewallet mode to system tray icon qt: Clean system tray icon menu for '-disablewallet' mode Oct 6, 2018

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Oct 9, 2018

tACK 36323e2
Tested on macOS 10.14

master (1d14174):
master

master (1d14174) -disablewallet:
master -disablewallet

PR (36323e2):
pr

PR (36323e2) -disablwallet:
pr - disablewallet

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 9, 2018

utACK 36323e2.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Oct 10, 2018

tACK 36323e2

@fanquake fanquake added this to Mergeable in High-priority for review Oct 10, 2018

@laanwj laanwj merged commit 36323e2 into bitcoin:master Oct 16, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ken2812221 pushed a commit to ken2812221/bitcoin that referenced this pull request Oct 16, 2018

Merge bitcoin#14383: qt: Clean system tray icon menu for '-disablewal…
…let' mode

36323e2 Clean systray icon menu for -disablewallet mode (Hennadii Stepanov)

Pull request description:

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

Tree-SHA512: c9ef58785fe2a54bc6f778140a16001748ed8c46da948656822b86fdc2e224203cd467857f71d00ce56fc73ff2590c46d8c234a54c261c1141d83039de6fee1e

@hebasto hebasto deleted the hebasto:20181003-disablewallet-systray branch Oct 16, 2018

@MeshCollider MeshCollider removed this from Mergeable in High-priority for review Oct 16, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

promag added a commit to promag/bitcoin that referenced this pull request Dec 30, 2018

laanwj added a commit that referenced this pull request Jan 3, 2019

Merge #15065: 0.17: GUI Backports #14123 #14133 #14383 #14597
27beb83 qt: All tray menu actions call showNormalIfMinimized (João Barbosa)
c470bbd qt: Use GUIUtil::bringToFront where possible (João Barbosa)
ac73c7d qt: Add GUIUtil::bringToFront (João Barbosa)
0c2fb87 Remove obj_c for macOS Dock icon menu (Hennadii Stepanov)
9034714 Use Qt signal for macOS Dock icon click event (Hennadii Stepanov)
4d4bc37 Remove obj_c for macOS Dock icon setting (Hennadii Stepanov)
d2ed162 Clean systray icon menu for -disablewallet mode (Hennadii Stepanov)
298dc15 gui: Favor macOS show / hide action in dock menu (João Barbosa)

Pull request description:

  Backport #14123 #14133 #14383 and #14597 to 0.17 branch to fix #13606 (comment).

Tree-SHA512: 543c80e7e2130870e801e0c9a69b06b9eea27c288478fc5dddeb662f7f3ec5b56b30916e5a9a629fced3fffcb8be77e2cd155e75cfd0a4392299add9730840f4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment