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: Enable system tray icon by default if available #14228

Merged
merged 1 commit into from Nov 12, 2018

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Sep 15, 2018

Prevent a user from losing access to the main window by minimizing it to the tray on the systems which have not “system tray” or “notification area” available (e.g. GNOME 3.26+).

Tested on Fedora 28 + GNOME 3.28.

@fanquake fanquake added the GUI label Sep 16, 2018
@practicalswift
Copy link
Member

@practicalswift practicalswift commented Sep 16, 2018

Original PR title: "Qt: Don't use systray icon on inappropriate systems"

When reading the PR title I first thought you were referring to Windows platforms :-)

src/qt/optionsmodel.cpp Outdated
@@ -53,12 +54,16 @@ void OptionsModel::Init(bool resetSettings)
// These are Qt-only settings:

// Window
if (!settings.contains("fHideTrayIcon"))
if (!QSystemTrayIcon::isSystemTrayAvailable()) {
settings.setValue("fHideTrayIcon", true);

This comment has been minimized.

@luke-jr

luke-jr Sep 16, 2018
Member

Kinda of nasty to change the saved setting just because you run it once on a platform that doesn't support it. I suggest leaving it be in settings, greying the setting, and ignoring it.

@hebasto hebasto force-pushed the hebasto:unavailable-systray branch Sep 16, 2018
@hebasto
Copy link
Member Author

@hebasto hebasto commented Sep 16, 2018

@luke-jr
Thank you for your review. You're right. Fixed: the saved settings aren't touched.
Please re-review.

src/qt/optionsmodel.cpp Outdated
@@ -54,7 +55,7 @@ void OptionsModel::Init(bool resetSettings)

// Window
if (!settings.contains("fHideTrayIcon"))
settings.setValue("fHideTrayIcon", false);
settings.setValue("fHideTrayIcon", !QSystemTrayIcon::isSystemTrayAvailable());

This comment has been minimized.

@luke-jr

luke-jr Sep 16, 2018
Member

I'd leave this alone. Or at the very least, don't set any value if there's no systray.

This comment has been minimized.

@hebasto

hebasto Sep 16, 2018
Author Member

If it'll be leaved alone the first call of the OptionsModel constructor sets fHideTrayIcon to the false anyway.
Why cannot it be initialized to the true on the GNOME environment?

This comment has been minimized.

@promag

promag Sep 16, 2018
Member

I'd say that's another PR, like "gui: Enable system tray icon by default if available"

This comment has been minimized.

@luke-jr

luke-jr Sep 16, 2018
Member

@hebasto Because the environment is not necessarily a permanent thing. Someone might run it on GNOME, and then run it on KDE.

Systems without a systray should just behave as if the option doesn't exist at all IMO.

Copy link
Member

@promag promag left a comment

How about createTrayIconMenu?

src/qt/optionsmodel.cpp Outdated
@@ -54,7 +55,7 @@ void OptionsModel::Init(bool resetSettings)

// Window
if (!settings.contains("fHideTrayIcon"))
settings.setValue("fHideTrayIcon", false);
settings.setValue("fHideTrayIcon", !QSystemTrayIcon::isSystemTrayAvailable());

This comment has been minimized.

@promag

promag Sep 16, 2018
Member

I'd say that's another PR, like "gui: Enable system tray icon by default if available"

src/qt/bitcoingui.cpp Outdated
@@ -585,6 +585,9 @@ void BitcoinGUI::setWalletActionsEnabled(bool enabled)

void BitcoinGUI::createTrayIcon(const NetworkStyle *networkStyle)
{
if (!QSystemTrayIcon::isSystemTrayAvailable()) {

This comment has been minimized.

@promag

promag Sep 16, 2018
Member

I would prevent calls to createTrayIcon and maybe assert(QSystemTrayIcon::isSystemTrayAvailable()) here?

@hebasto
Copy link
Member Author

@hebasto hebasto commented Sep 16, 2018

@promag

Thank you for your review.

How about createTrayIconMenu?

if (!trayIcon)
return;

Is it not enough?

@hebasto hebasto force-pushed the hebasto:unavailable-systray branch Sep 16, 2018
@hebasto
Copy link
Member Author

@hebasto hebasto commented Sep 16, 2018

@promag
You've suggested a more robust code. Fixed. Please re-review.

@hebasto hebasto changed the title Qt: Don't use systray icon on inappropriate systems Qt: Enable system tray icon by default if available Sep 16, 2018
@hebasto
Copy link
Member Author

@hebasto hebasto commented Sep 16, 2018

This PR title has been changed as @promag proposed.
btw, gui prefix is not mentioned in the CONTRIBUTING.md. Is it valid? (answered by @laanwj on IRC)

@hebasto hebasto force-pushed the hebasto:unavailable-systray branch Sep 17, 2018
@hebasto
Copy link
Member Author

@hebasto hebasto commented Sep 17, 2018

@luke-jr

Someone might run it on GNOME, and then run it on KDE.
Systems without a systray should just behave as if the option doesn't exist at all IMO.

You're right. Fixed: settings are not touched at all on systems without a systray.

@laanwj
Copy link
Member

@laanwj laanwj commented Sep 23, 2018

utACK after squash

@hebasto hebasto force-pushed the hebasto:unavailable-systray branch Sep 23, 2018
@hebasto
Copy link
Member Author

@hebasto hebasto commented Sep 23, 2018

utACK after squash

@laanwj squashed

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Sep 25, 2018

lightly tested on OSes where the tray is available.
ACK 244774d9626f37e15d8e021de4e735302ae2d292.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Sep 25, 2018

Waiting on @promag's ack or further comments

@promag
Copy link
Member

@promag promag commented Sep 25, 2018

@jonasschnelli thanks for the reminder, will check tonight.

@promag
Copy link
Member

@promag promag commented Sep 26, 2018

Could guard these mappings
https://github.com/bitcoin/bitcoin/blob/244774d9626f37e15d8e021de4e735302ae2d292/src/qt/optionsdialog.cpp#L222-L223

but since these widgets are disabled and the options dialog is the only place that changes these options, the guard is not necessary.

nit, could fix the comment
https://github.com/bitcoin/bitcoin/blob/244774d9626f37e15d8e021de4e735302ae2d292/src/qt/optionsdialog.cpp#L220

utACK 244774d — tomorrow I can setup a system without tray to test.

@hebasto
Copy link
Member Author

@hebasto hebasto commented Sep 26, 2018

@promag

but since these widgets are disabled and the options dialog is the only place that changes these options, the guard is not necessary.

Agree. (see below)

nit, could fix the comment

I did not catch your idea. What should it be like?

@promag
Copy link
Member

@promag promag commented Sep 26, 2018

Should be "Non macOS"?

@promag
Copy link
Member

@promag promag commented Sep 26, 2018

BTW, I'm currently installing fedora..

@hebasto
Copy link
Member Author

@hebasto hebasto commented Sep 26, 2018

@promag

Should be "Non macOS"?

Ah, I see. The comment line /* Window */ just repeats the Options tab name.

@promag
Copy link
Member

@promag promag commented Sep 26, 2018

@hebasto right, disregard that comment then.

@hebasto
Copy link
Member Author

@hebasto hebasto commented Sep 27, 2018

@promag

but since these widgets are disabled and the options dialog is the only place that changes these options, the guard is not necessary.

In fact, the guard around mapper is necessary: addMapping effectively overwrites ui settings in the OptionsDialog constructor.

Thank you for this catch. I'm going to fix it.

Prevent a user from losing access to the main window by minimizing it to
the tray on some systems (e.g. GNOME 3.26+).
@hebasto hebasto force-pushed the hebasto:unavailable-systray branch to ec1201a Sep 27, 2018
@hebasto
Copy link
Member Author

@hebasto hebasto commented Sep 27, 2018

@promag's catch has been fixed.

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Sep 28, 2018

Coverage Change (pull 14228) Reference (master)
Lines +0.0287 % 87.0361 %
Functions +0.1235 % 84.1130 %
Branches +0.0085 % 51.5451 %
@hebasto
Copy link
Member Author

@hebasto hebasto commented Oct 13, 2018

@promag Would you mind re-reviewing this PR?

@laanwj
Copy link
Member

@laanwj laanwj commented Nov 12, 2018

utACK ec1201a

pull bot pushed a commit to jaschadub/bitcoin that referenced this pull request Nov 12, 2018
ec1201a Don't use systray icon on inappropriate systems (Hennadii Stepanov)

Pull request description:

  Prevent a user from losing access to the main window by minimizing it to the tray on the systems which have not “system tray” or “notification area” available (e.g. GNOME 3.26+).

  Tested on Fedora 28 + GNOME 3.28.

Tree-SHA512: c2dc26ff31c38a882dbd7d1ff71af99f1ba38a04a1c8b7fe7b99b93e4c0719f2916c7db0e620806a36582402d18939c635e1913c276b452ecbf939936067407b
@laanwj laanwj merged commit ec1201a into bitcoin:master Nov 12, 2018
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
@promag
Copy link
Member

@promag promag commented Nov 12, 2018

Sorry @hebasto, missed this. utACK ec1201a.

@hebasto hebasto deleted the hebasto:unavailable-systray branch Nov 12, 2018
hebasto added a commit to hebasto/bitcoin that referenced this pull request Dec 19, 2018
That bug was introduced in bitcoin#14228.
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018
Prevent a user from losing access to the main window by minimizing it to
the tray on some systems (e.g. GNOME 3.26+).

Github-Pull: bitcoin#14228
Rebased-From: ec1201a
MarcoFalke added a commit that referenced this pull request Jan 2, 2019
c8d9d90 Fix broken notificator on GNOME (Hennadii Stepanov)

Pull request description:

  Fix #14994; that bug was introduced in #14228 (that was my fault).

  ~Also this commit explicit separates~ There are two functions of the tray icon:
   - a system tray widget (`QSystemTrayIcon::isSystemTrayAvailable() == true`)
   - a high-level notificator via balloon messages (`QSystemTrayIcon::supportsMessages() == true`)

  ~These properties are mutually independent,~ e.g., on Fedora 29 + GNOME:
  ```
  QSystemTrayIcon::isSystemTrayAvailable() == false;
  QSystemTrayIcon::supportsMessages() == true;
  ```

  UPDATE:

  `supportsMessages()` makes no sense without `isSystemTrayAvailable()`: `QSystemTrayIcon::showMessage()` just not working on Fedora 29 + GNOME.

Tree-SHA512: 3e75ed2dfcef112bd64b8c329227ae68ba57f3be55769629f4eb3b1c52ef1f33db635f00bb5fd57c25f73a692971d6a847ea14c525f41c594fddde6e970a8ad8
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Jan 8, 2019
That bug was introduced in bitcoin#14228.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.