Skip to content
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: Fix start with the -min option #14517

Merged
merged 1 commit into from Jan 9, 2019

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Oct 19, 2018

From IRC:

2018-10-17T12:36:38 <Drakon> The option to minimize to system tray instead of the taskbar ist available, but doesn't have an effect if it is started with the -min option. If I start it via that option, I have to click on the program symbil on the taskbar and then minimize it again in order to get it minimized to system tray.
2018-10-17T12:37:28 <Drakon> That's annoying.
2018-10-17T13:51:19 <wumpus> can you open an issue for that please? https://github.com/bitcoin/bitcoin/issues/new
2018-10-17T13:53:24 <wumpus> (if there isn't one yet-)

This PR fixes this bug.

@fanquake fanquake added the GUI label Oct 19, 2018
@promag
Copy link
Member

@promag promag commented Oct 19, 2018

@hebasto
Copy link
Member Author

@hebasto hebasto commented Oct 19, 2018

@promag Thank you.

@laanwj
Copy link
Member

@laanwj laanwj commented Nov 12, 2018

I don't think this fixes the mentioned issue; this will prevent -min from doing anything at all in case of minimize to tray but there is no tray icon.
But the reported issue seems to be that -min doesn't do the right thing in case minimize-to-tray is enabled.

@hebasto
Copy link
Member Author

@hebasto hebasto commented Nov 13, 2018

@laanwj
Thank you for your review.

... this will prevent -min from doing anything at all in case of minimize to tray but there is no tray icon.

To be exact this will prevent -min from doing anything in case of minimize to tray and there is a tray icon. A user will get a tray icon on the desktop; the main window will be hidden, i.e., it'll be effectively minimized to the tray icon.

src/qt/bitcoingui.h Outdated
/** Get the tray icon status.
Some systems have not “system tray” or “notification area” available.
*/
bool getTrayIconStatus() { return trayIcon; }

This comment has been minimized.

@promag

promag Dec 5, 2018
Member

hasTrayIcon() or isTrayIconAvailable()?

This comment has been minimized.

@hebasto

hebasto Dec 5, 2018
Author Member

@promag hasTrayIcon(); to not be confused with QSystemTrayIcon::isSystemTrayAvailable().

src/qt/bitcoin.cpp Outdated
{
// If -min option passed, start window minimized or minimized to tray
if (gArgs.GetBoolArg("-min", false)) {
if (!window->getTrayIconStatus() || !clientModel->getOptionsModel()->getMinimizeToTray()) {

This comment has been minimized.

@promag

promag Dec 5, 2018
Member

What if there is a tray icon and getMinimizeToTray?

This comment has been minimized.

@hebasto

hebasto Dec 5, 2018
Author Member

@promag

What if there is a tray icon and getMinimizeToTray?

This is the case described in this PR's description!

With -min option and there is a tray icon and getMinimizeToTray:

  • current master: client hides the splash screen then shows the normal main window (that is an issue);
  • this PR: client hides the splash screen then does not show the main window as it is minimized to the tray icon (expected behaviour).
@hebasto hebasto force-pushed the hebasto:20181019-start-minimized-to-tray branch Dec 5, 2018
@hebasto
Copy link
Member Author

@hebasto hebasto commented Dec 5, 2018

@promag
Thank you for your review. Your comment has been addressed.

src/qt/bitcoin.cpp Outdated
else
{
// If -min option passed, start window minimized or minimized to tray
if (gArgs.GetBoolArg("-min", false)) {

This comment has been minimized.

@promag

promag Dec 6, 2018
Member

I think it would be more clear doing like:

if (!gArgs.GetBoolArg("-min", false)) {
    window->show();
} else if (window->hasTrayIcon() && clientModel->getOptionsModel()->getMinimizeToTray()) {
    // do nothing as the window is managed by the tray icon
} else {
    // show window minimized as requested
}
@hebasto hebasto force-pushed the hebasto:20181019-start-minimized-to-tray branch Dec 6, 2018
@hebasto
Copy link
Member Author

@hebasto hebasto commented Dec 6, 2018

@promag Your suggestion is obviously more clear. Implemented.

src/qt/bitcoingui.h Outdated
/** Get the tray icon status.
Some systems have not “system tray” or “notification area” available.
*/
bool hasTrayIcon() { return trayIcon; }

This comment has been minimized.

@promag

promag Dec 6, 2018
Member

nit. const.

@hebasto hebasto force-pushed the hebasto:20181019-start-minimized-to-tray branch Dec 6, 2018
@hebasto
Copy link
Member Author

@hebasto hebasto commented Dec 6, 2018

All comments have been addressed.
@promag @laanwj would you mind re-reviewing?

src/qt/bitcoingui.h Outdated
@@ -82,6 +82,11 @@ class BitcoinGUI : public QMainWindow
#endif // ENABLE_WALLET
bool enableWallet = false;

/** Get the tray icon status.
Some systems have not “system tray” or “notification area” available.

This comment has been minimized.

@promag

promag Dec 6, 2018
Member

nit, could use " instead?

This comment has been minimized.

@hebasto

hebasto Dec 6, 2018
Author Member

@promag sure!

When GUI starts with the `-min` option, the `Minimize to tray instead of
the taskbar` option works as expected now.
@hebasto hebasto force-pushed the hebasto:20181019-start-minimized-to-tray branch to 9300961 Dec 6, 2018
Copy link
Member

@luke-jr luke-jr left a comment

utACK

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018
When GUI starts with the `-min` option, the `Minimize to tray instead of
the taskbar` option works as expected now.

Github-Pull: bitcoin#14517
Rebased-From: 9300961
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Jan 5, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15101 (gui: Add WalletController by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@laanwj
Copy link
Member

@laanwj laanwj commented Jan 9, 2019

utACK 9300961

@laanwj laanwj merged commit 9300961 into bitcoin:master Jan 9, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jan 9, 2019
9300961 Fix start with the `-min` option (Hennadii Stepanov)

Pull request description:

  From IRC:

  > 2018-10-17T12:36:38  \<Drakon\> The option to minimize to system tray instead of the taskbar ist available, but doesn't have an effect if it is started with the -min option. If I start it via that option, I have to click on the program symbil on the taskbar and then minimize it again in order to get it minimized to system tray.
  > 2018-10-17T12:37:28  \<Drakon\> That's annoying.
  > 2018-10-17T13:51:19  \<wumpus\> can you open an issue for that please? https://github.com/bitcoin/bitcoin/issues/new
  > 2018-10-17T13:53:24  \<wumpus\> (if there isn't one yet-)

  This PR fixes this bug.

Tree-SHA512: c5a5521287b49b13859edc7c6bd1cd07cac14b84740450181dce00bf2781fc3dfc84476794baa16b0e26a2d004164617afdb61f829e629569703c5bcc45e2a4e
@hebasto hebasto deleted the hebasto:20181019-start-minimized-to-tray branch Jan 9, 2019
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 9, 2020
Summary:
PR description:
> When GUI starts with the `-min` option, the `Minimize to tray instead of
> the taskbar` option works as expected now.

Backport of Core [[bitcoin/bitcoin#14517 | PR14517]]

Test Plan:
```
ninja && ninja check
src/qt/bitcoin-qt -min
```

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7841
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants