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 broken notificator on GNOME #15000

Merged
merged 1 commit into from Jan 2, 2019

Conversation

Projects
None yet
6 participants
@hebasto
Copy link
Member

commented Dec 19, 2018

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.

@fanquake fanquake added the GUI label Dec 19, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #11625 (Add BitcoinApplication & RPCConsole tests by ryanofsky)

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

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

Concept ACK, TIL that those are separate

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Dec 19, 2018

@hebasto hebasto changed the title qt: Fix broken notificator on GNOME [WIP] qt: Fix broken notificator on GNOME Dec 19, 2018

Fix broken notificator on GNOME
That bug was introduced in #14228.

@hebasto hebasto force-pushed the hebasto:20181218-fix-notificator branch to c8d9d90 Dec 19, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2018

@laanwj Thank you for your review.

Concept ACK, TIL that those are separate

TIL too: supportsMessages() makes no sense without isSystemTrayAvailable(). Sorry for confusing.

PR description and commit have been updated.

@hebasto hebasto changed the title [WIP] qt: Fix broken notificator on GNOME qt: Fix broken notificator on GNOME Dec 19, 2018

@fanquake

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

Tested c8d9d90 on Fedora 29.1-2. No longer causes a segfault when a notification appears.
Created notifications using:

getnewaddress
generatetoaddress 1 the_new_address

fedora - notifications

However, from a quick check, this seems to break notifications on macOS (10.14.2). i.e if I make deploy this PR, and run with -regtest, I no longer see incoming transaction notifications in the Notification Centre when using generatetoaddress. Be good if someone else could confirm.

@promag

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

@fanquake I can't reproduce your case. All notifications go to notification centre but they don't popup when calling from the console window.

@MarcoFalke MarcoFalke merged commit c8d9d90 into bitcoin:master Jan 2, 2019

2 checks passed

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

MarcoFalke added a commit that referenced this pull request Jan 2, 2019

Merge #15000: qt: Fix broken notificator on GNOME
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
@hebasto

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

@fanquake

... this seems to break notifications on macOS (10.14.2).

@promag

All notifications go to notification centre but they don't popup when calling from the console window.

See: #14976 (comment)

@hebasto hebasto deleted the hebasto:20181218-fix-notificator branch Jan 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.