macOS: Post user notifications using QSystemTrayIcon #114

hebasto · 2020-10-24T20:41:54Z

Pros:

drop deprecated NSUserNotificationCenter
get rid of some macOS-specific, i.e., Objective-C, code

Cons:

this approach does not support launching a just compiled binary from the Terminal (this affects developers rather end users)

Close #112

Note: no Growl is required :)

hebasto · 2020-10-24T22:29:32Z

cc @jonasschnelli

jonasschnelli · 2020-10-25T09:10:54Z

Will this still use macOS'es Notification Center?
How does this interact with non code signed and non "notarized" apps?

hebasto · 2020-10-25T09:34:11Z

Will this still use macOS'es Notification Center?

Yes, it will.

How does this interact with non code signed and non "notarized" apps?

Tested on non code signed apps: Bitcoin-Core.dmg and gitian bitcoin-717f9a25f414-osx-unsigned.dmg.

jonasschnelli · 2020-10-25T09:50:41Z

Concept ACK

Sjors · 2020-10-26T11:48:55Z

How do I produce a notification on macOS this way?

2020-10-26T11:43:32Z [main] System tray properties:
2020-10-26T11:43:32Z [main]  is available: yes
2020-10-26T11:43:32Z [main]  supports balloon messages: yes

But sending transactions back and forth doesn't trigger a notification. I tried making a DMG and installing from that locally.

I don't mind making things a bit more annoying for self-compilation on macOS if it allows dropping a whole bunch of deprecated code.

hebasto · 2020-10-26T14:54:25Z

@Sjors

How do I produce a notification on macOS this way?

But sending transactions back and forth doesn't trigger a notification. I tried making a DMG and installing from that locally.

Tested on macOS 10.15:

installed Bitcoin-Core.dmg which was cross compiled on Linux Mint 20
installed gitian built bitcoin-717f9a25f414-osx-unsigned.dmg

~~While compiling locally on macOS 10.15 I get an error:~~

% make deploy
...
+ Finalizing .dmg disk image +
hdiutil: convert failed - Resource temporarily unavailable
make: *** [Makefile:1279: Bitcoin-Core.dmg] Error 1

~~Application has actually been installed into Applications folder, but, indeed, has no interactions with the Notification Center.~~

~~Is make deploy broken?~~

fanquake · 2020-10-26T23:03:44Z

Is make deploy broken?

No.

Finalizing .dmg disk image +
hdiutil: convert failed - Resource temporarily unavailable
make: *** [Makefile:1279: Bitcoin-Core.dmg] Error 1

This error happens when you run make deploy, but there is already a Bitcoin-Core.dmg mounted.

I tested this branch and notifications seem to work fine, although I don't remember seeing the cloud with the exclamation point before:

hebasto · 2020-10-27T07:27:53Z

@Sjors @fanquake

What are your environments (macOS version, Qt version)?

fanquake · 2020-10-27T07:43:22Z

What are your environments (macOS version, Qt version)?

macOS 10.15.7. Qt 5.15.1.

hebasto · 2020-10-27T09:05:22Z

I've re-tested one more time.

Is make deploy broken?

No.

Correct. make deploy works fine. It was wrong usage by me :)

To make this PR work on macOS one should run Bitcoin Core.app (installation of Bitcoin-Qt.dmg is not required).

Some screenshots:

Catalina 10.15.7 (19H2)
Big Sur 11.0 Beta (20A5395g)

jonasschnelli · 2020-10-27T13:07:39Z

~~Tested ACK 717f9a2~~

jonasschnelli · 2020-10-27T13:54:18Z

Is the current notification support on macOS in master broken? If so, this qualifies as a bugfix I'd say.

hebasto · 2020-10-27T14:06:25Z

Is the current notification support on macOS in master broken?

No, it isn't.

Sjors · 2020-10-30T14:52:36Z

macOS 10.15.7 with QT 5.15.1 via homebrew. I tried using Bitcoin-Qt.app rather than the DMG route and it works fine.

The tray icon is a bit weird, because nothing happens when you click on it:

That's because trayIconActivated still has an exception for macOS. It's a bit annoying that QT requires this tray icon to be visible in order for notifications to work, but anyway.

The exclamation mark in notifications could be avoided, by tweaking void Notificator::notifySystray:

{
    QSystemTrayIcon::MessageIcon sicon = QSystemTrayIcon::NoIcon;
    switch(cls) // Set icon based on class
    {
#ifdef Q_OS_MAC
    case Information: sicon = QSystemTrayIcon::NoIcon; break;
#else
    case Information: sicon = QSystemTrayIcon::Information; break;
#endif

hebasto · 2020-10-30T20:41:52Z

@Sjors

Thank you for reviewing!

The tray icon is a bit weird, because nothing happens when you click on it:

I think some behavior could be assigned to the tray icon, but it would be unusual for macOS UX, no?

The exclamation mark in notifications could be avoided, by tweaking void Notificator::notifySystray:

Do we need to avoid it?

Sjors · 2020-10-31T08:27:15Z

Given that we can't avoid the tray icon (afaik), it would be good if it shows / hides the window. That way at least it doesn't feel broken.

Typically on macOS these icons provide a menu with shortcuts to various functions. That might be better, if it's not too much work:

As for the explanation mark, I would prefer to avoid it, because it's very non standard. It could be interpreted as an error. That said, it's not the end of the world.

hebasto · 2020-10-31T08:50:20Z

Given that we can't avoid the tray icon (afaik), it would be good if it shows / hides the window. That way at least it doesn't feel broken.

Typically on macOS these icons provide a menu with shortcuts to various functions. That might be better, if it's not too much work:

I'd leave adding tray icon functionality for a follow up as it will probably raise another discussion.

As for the explanation mark, I would prefer to avoid it, because it's very non standard. It could be interpreted as an error. That said, it's not the end of the world.

Ok, Going to patch it.

johnsBeharry · 2020-10-31T11:25:17Z

Notification Title

Nitpick for the notification title. Since the logo is already included its not necessary to repeat "Bitcoin Core".

Context Menu

WRT the menu bar context menu options. Personally I'd like to be able to quickly start/stop syncing cause I'm not always on a consistent connection.

Network Activity: Enabled
Disable Network Activity
---
Receive
Send
---
Open Bitcoin Core

Menu Bar App: Concept

@GBKS took conceptual look at what the design for a menu bar app could look like. Perhaps he can give some input - it also focused on network activity switch.

-- source

Sjors · 2020-11-02T17:35:28Z

@johnsBeharry macOS notifications always have the application name in the title. It's also good for accessibility, think voice readers.

jonasschnelli · 2020-11-04T08:42:27Z

I missed the new tray icon in my tests and only focused on the actual notification.
The visible global menu icon is a NACK element to me.
Can we use the system notification without a visible global menu icon? Under the hood, they are two separated frameworks. But maybe Qt combines them as one.

jonasschnelli · 2020-11-04T08:48:56Z

This is the mac Qt code for further analysis: https://github.com/qt/qtbase/blob/81e09ae404b632a92e1e4c27f5875bdf027c5401/src/plugins/platforms/cocoa/qcocoasystemtrayicon.mm

hebasto · 2020-11-04T08:59:12Z

Can we use the system notification without a visible global menu icon?

No.

Under the hood, they are two separated frameworks. But maybe Qt combines them as one.

If a tray icon is hidden, m_statusItem == nil and QCocoaSystemTrayIcon::showMessage is noop.

hebasto · 2020-11-12T09:30:19Z

This is the mac Qt code for further analysis: https://github.com/qt/qtbase/blob/81e09ae404b632a92e1e4c27f5875bdf027c5401/src/plugins/platforms/cocoa/qcocoasystemtrayicon.mm

It uses the deprecated NSUserNotificationCenter, and in Qt community I've found no signs of moving from NSUserNotificationCenter to UNUserNotificationCenter.

luke-jr · 2020-11-13T18:51:14Z

While the menu app concept looks nice, I do think users should be able to disable it if they want, and shouldn't lose notifications by doing so...

DrahtBot · 2020-12-01T11:46:07Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

hebasto · 2020-12-01T20:50:06Z

@luke-jr

While the menu app concept looks nice, I do think users should be able to disable it if they want, and shouldn't lose notifications by doing so...

Only visible icon allows notifications via QSystemTrayIcon. Adding menu to that icon is not a goal of this PR.

Another approach is to use the new UNUserNotificationCenter, but I have no success to authorize bitcoin-qt for notifications.

I'll readily close this PR in favor of any alternative one.

RandyMcMillan · 2020-12-07T01:57:59Z

MacOS 10.14.6 (18G103)
Qt Creator 4.13.3
Based on Qt 5.15.2 (Clang 11.0 (Apple), 64 bit)
Built on Nov 13 2020 12:39:07
From revision 524cad144a

I agree with @jonasschnelli on the menu icon.
Until more functionality is added to the icon - it should be omitted (imo).

build command used...

rm -f src/bitcoind && rm -rf Bitcoin-Qt.app && rm -rf ~/Library/Saved\ Application\ State/org.bitcoinfoundation.Bitcoin-Qt.savedState && rm -rf ~/Library/Preferences/org.bitcoin.Bitcoin-Qt.plist && make appbundle && wait && ./Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt

hebasto · 2021-02-21T20:20:24Z

Closing as I cannot see the way to provide notifications in Qt way without the visible global menu icon.

hebasto · 2021-02-21T20:23:41Z

@jonasschnelli Could be labeled "Up for grabs" :)

hebasto added 3 commits October 24, 2020 23:18

qt: Add QSystemTrayIcon properties into LogQtInfo output

883883e

qt: Post user notifications using QSystemTrayIcon on macOS

9a84744

qt: Remove unused macnotificationhandler module

717f9a2

maflcko added the macOS label Oct 25, 2020

jonasschnelli added this to the 0.22.0 milestone Oct 27, 2020

hebasto mentioned this pull request Nov 19, 2020

refactor: Fix deprecation warnings when building against Qt 5.15 #46

Merged

DrahtBot added the Needs rebase label Dec 1, 2020

hebasto closed this Feb 21, 2021

maflcko added the Up for grabs label Feb 22, 2021

hebasto removed the Needs rebase label Apr 30, 2021

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

macOS: Post user notifications using QSystemTrayIcon #114

macOS: Post user notifications using QSystemTrayIcon #114

Uh oh!

Conversation

hebasto commented Oct 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Oct 24, 2020

Uh oh!

jonasschnelli commented Oct 25, 2020

Uh oh!

hebasto commented Oct 25, 2020

Uh oh!

jonasschnelli commented Oct 25, 2020

Uh oh!

Sjors commented Oct 26, 2020

Uh oh!

hebasto commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Oct 26, 2020

Uh oh!

hebasto commented Oct 27, 2020

Uh oh!

fanquake commented Oct 27, 2020

Uh oh!

hebasto commented Oct 27, 2020

Uh oh!

jonasschnelli commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Oct 27, 2020

Uh oh!

hebasto commented Oct 27, 2020

Uh oh!

Sjors commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Oct 30, 2020

Uh oh!

Sjors commented Oct 31, 2020

Uh oh!

hebasto commented Oct 31, 2020

Uh oh!

johnsBeharry commented Oct 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notification Title

Context Menu

Menu Bar App: Concept

Uh oh!

Sjors commented Nov 2, 2020

Uh oh!

jonasschnelli commented Nov 4, 2020

Uh oh!

jonasschnelli commented Nov 4, 2020

Uh oh!

hebasto commented Nov 4, 2020

Uh oh!

hebasto commented Nov 12, 2020

Uh oh!

luke-jr commented Nov 13, 2020

Uh oh!

DrahtBot commented Dec 1, 2020

Uh oh!

hebasto commented Dec 1, 2020

Uh oh!

RandyMcMillan commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

build command used...

Uh oh!

hebasto commented Feb 21, 2021

Uh oh!

hebasto commented Feb 21, 2021

Uh oh!

Uh oh!

hebasto commented Oct 24, 2020 •

edited

Loading

hebasto commented Oct 26, 2020 •

edited

Loading

jonasschnelli commented Oct 27, 2020 •

edited

Loading

Sjors commented Oct 30, 2020 •

edited

Loading

johnsBeharry commented Oct 31, 2020 •

edited

Loading

RandyMcMillan commented Dec 7, 2020 •

edited

Loading