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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notifications from the main process #9269

Merged
merged 22 commits into from May 31, 2017

Conversation

Projects
None yet
9 participants
@MarshallOfSound
Member

MarshallOfSound commented Apr 23, 2017

This is a WIP of extracting the Notification logic that hooks into the Chromium notification model in the renderer process to a simpler API to use from the main process. It also adds a few extras like inline replies on macOS.

An example of what this API looks like (at least currently on macOS)

const n = new Notification({
  title: 'Hello World',
  body: 'This is the long and complicated body for this notification that just goes on and on and on and never really seems to stop',
  silent: true,
  icon: '/path/to/ninja.png',
  hasReply: true,
  replyPlaceholder: 'Type Here!!'
});

n.on('show', () => console.log('showed'));
n.on('click', () => console.info('clicked!!'));
n.on('reply', (e, reply) => console.log('Replied:', reply));

n.show();

Currently just supports macOS and Windows, this is just to get some early implementation feedback 馃憤

Platform Support Status:

  • - macOS
  • - Windows 10
  • - Windows 8
  • - Windows 7
  • - Linux

TODO:

  • - Set NativeImage as the Toast icon on Windows
  • - Explore Windows 10 inline reply possibility Not required for the first merge
  • - Test code works on Windows 8
  • - Port the new Windows 7 code (maybe don't even port it, just include and see if we can get it working)
  • - Linux 馃槃 (Would be great if I could get some help with this though I'm sure I'll figure it out eventually)
  • - Duration on platforms that support it Not required for the first merge
  • - Sticky (permanent) on platforms that support it Not required for the first merge

Fixes #3359

Note: This API is marked as Experimental in the docs 馃憤

inline HRESULT initialize() {
HINSTANCE LibShell32 = LoadLibrary(L"SHELL32.DLL");
HRESULT hr = loadFunctionFromLibrary(LibShell32, "SetCurrentProcessExplicitAppUserModelID", SetCurrentProcessExplicitAppUserModelID);

This comment has been minimized.

@felixrieseberg

felixrieseberg Apr 23, 2017

Member

This strikes me as potentially dangerous... is that something we really want to call?

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Apr 23, 2017

Member

@felixrieseberg Hmm, now that I think about it maybe not. app.setUserModelID probably handles this for us, will test on Windows 8 (where appUserModelID is strictly required)

@felixrieseberg

This comment has been minimized.

Member

felixrieseberg commented Apr 23, 2017

馃憦 A+ effort, thanks a ton! We should make sure that the behavior between renderer and main process is identical - for Windows, that would mean that the notifications here use the brand new Windows 7 notifications.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Apr 23, 2017

that would mean that the notifications here use the brand new Windows 7 notifications.

Ah yes, forgot about those 馃槅 Will add it to the list 馃槃

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Apr 23, 2017

Instead of reimplementing windows 7 notifications I just called the NotificationPresenter methods, I'm writing new code for the others though as we can write a lot nicer, expansible, notification API's if we aren't coupled to how Chromium manages notifications. (Case and point being the inline reply on macOS)

Also of note here, is if the Notification initialization fails on W8 or W10 we fall back to the W7 notification implementation so there will always be a Notification implementation ready to go 馃憤

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Apr 24, 2017

I pushed up core linux support, if I could get some assistance from someone getting the click event working (I couldn't make it play ball) that would be great.

/cc @electron/maintainers

@MarshallOfSound MarshallOfSound changed the title from [WIP] Notifications from the main process to Notifications from the main process May 20, 2017

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented May 20, 2017

This is ready for 馃憖 @electron/maintainers

MarshallOfSound added some commits Apr 23, 2017

@groundwater groundwater requested a review from alespergl May 23, 2017

@groundwater

This comment has been minimized.

Member

groundwater commented May 23, 2017

Pinged MS folks who may also want to make notifications 馃憤 馃憤

@sindresorhus sindresorhus referenced this pull request May 18, 2017

Closed

Plugin proposal #181

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented May 29, 2017

@MarshallOfSound How about using brightray::NotificationPresenter and brightray::Notification directly to implement api::Notification? The notification interface is independent from Chromium, we can just reuse the code instead of reimplement everything.

A few features like Reply would require adding new methods to brightray::Notification but it can be done easily.

Or is there anything preventing you from reusing the code?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented May 29, 2017

@zcbenz That's what I initially tried to do, but something (and now I can't remember what) kept crashing 馃槩 I can go back to trying to use the NotificationPresenter logic if you want

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented May 29, 2017

@MarshallOfSound I think we should try reusing NotificationPresenter logic, I can help solve the crashes.

MarshallOfSound added some commits May 29, 2017

MarshallOfSound added some commits May 29, 2017

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented May 29, 2017

@zcbenz Implementation updated to use NotificationPresenter

@@ -5,7 +5,9 @@ to the user. Electron conveniently allows developers to send notifications with
the [HTML5 Notification API](https://notifications.spec.whatwg.org/), using
the currently running operating system's native notification APIs to display it.
**Note:** Since this is an HTML5 API it is only available in the renderer process.
**Note:** Since this is an HTML5 API it is only available in the renderer process. If
you want to show Notifications in the main process please check out the

This comment has been minimized.

@YurySolovyov

YurySolovyov May 29, 2017

Contributor

it is not really "show in main process", but more like "Invoke/create notification from main process" IMO, just my $0.02 馃槃

@@ -26,7 +26,9 @@ class Notification {
const std::string& tag,
const GURL& icon_url,
const SkBitmap& icon,
const bool silent) = 0;
const bool silent,
const bool hasReply,

This comment has been minimized.

@zcbenz

zcbenz May 30, 2017

Contributor

There is no need to add const, since bool is passed by value here, also in C++ we use snake_case for variable names:

bool silent,
bool has_reply,
const bool silent) = 0;
const bool silent,
const bool hasReply,
const base::string16 replyPlaceholder) = 0;

This comment has been minimized.

@zcbenz

zcbenz May 30, 2017

Contributor

Should pass const reference for strings:

const base::string16& reply_placeholder
@@ -21,15 +21,20 @@ - (instancetype)initWithPresenter:(brightray::NotificationPresenterMac*)presente
- (void)userNotificationCenter:(NSUserNotificationCenter*)center
didDeliverNotification:(NSUserNotification*)notif {
auto notification = presenter_->GetNotification(notif);
if (notification)
if (notification)

This comment has been minimized.

@zcbenz

zcbenz May 30, 2017

Contributor

There is a trailing space.

@@ -16,6 +17,9 @@ class NotificationDelegate : public content::DesktopNotificationDelegate {
// Failed to send the notification.
virtual void NotificationFailed() {}
// Notification was replied to
virtual void NotificationReplied(std::string reply) {}

This comment has been minimized.

@zcbenz

zcbenz May 30, 2017

Contributor

Should pass const reference here:

NotificationReplied(const std::string& reply)
void Dismiss() override;
void NotificationDisplayed();
void NotificationReplied(std::string reply);

This comment has been minimized.

@zcbenz

zcbenz May 30, 2017

Contributor

Should pass const reference here:

NotificationReplied(const std::string& reply)
void NotifyPropsUpdated();
private:
base::string16 title_ = base::UTF8ToUTF16("");

This comment has been minimized.

@zcbenz

zcbenz May 30, 2017

Contributor

Strings are initialized to "" by default, there is no need to explicitly initialize it here.

Emit("close");
}
void Notification::NotifyPropsUpdated() {}

This comment has been minimized.

@zcbenz

zcbenz May 30, 2017

Contributor

This method seems redundant.

// Showing notifications
void Notification::Show() {
SkBitmap image = *(new SkBitmap);

This comment has been minimized.

@zcbenz

zcbenz May 30, 2017

Contributor

This variable is redundant, we can pass icon_.AsBitmap() to Show(...), when icon is empty it will just return an empty bitmap.

auto notif = presenter_->CreateNotification(adapter.get());
if (notif) {
ignore_result(adapter.release()); // it will release itself automatically.
GURL nullUrl = *(new GURL);

This comment has been minimized.

@zcbenz

zcbenz May 30, 2017

Contributor

This is redundant:

notif->Show(..., GURL(), ...);
bool initialized_ = false;
void Notification::OnInitialProps() {
if (!initialized_) {
presenter_ = brightray::NotificationPresenter::Create();

This comment has been minimized.

@zcbenz

zcbenz May 30, 2017

Contributor

The presenter can be received by calling BrowserClient::GetNotificationPresenter().

Also it should be noticed that not every platform supports notifications, so it might return null and this case should be guarded. We also need to add a new API like Notification.isSupported() to indicate whether notification is supported on current platform.

MarshallOfSound added some commits May 30, 2017

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented May 30, 2017

@zcbenz Updated as per feedback 馃槃

@zcbenz

zcbenz approved these changes May 31, 2017

zcbenz added some commits May 31, 2017

@MarshallOfSound

Woops 馃槅 Didn't want to leave that in 馃ぃ

@zcbenz zcbenz merged commit e82af41 into master May 31, 2017

9 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #6813219 succeeded in 134s
Details
electron-linux-ia32 Build #6813220 succeeded in 95s
Details
electron-linux-x64 Build #6813221 succeeded in 192s
Details
electron-mas-x64 Build #4367 succeeded in 11 min
Details
electron-osx-x64 Build #4372 succeeded in 10 min
Details
electron-win-ia32 Build #3342 succeeded in 9 min 42 sec
Details
electron-win-x64 Build #3319 succeeded in 8 min 48 sec
Details

@zcbenz zcbenz deleted the main-notifications branch May 31, 2017

@black-snow

This comment has been minimized.

black-snow commented Jun 9, 2017

+1 interwebs for you sir if this works nicely
Finally I can trash my ipc-notification-framework and reduce the amount of black magic in my project.

@alexanderepstein

This comment has been minimized.

alexanderepstein commented Jun 10, 2017

Is there any way I can use this in any other process then main? It would get rid of a dependency in my project and I imagine this looks cleaner. Nice work by the way

@black-snow

This comment has been minimized.

black-snow commented Jun 10, 2017

@alexanderepstein which process would that be? You can use it in any main process. And renderer had notifications all along.

@mfix22 mfix22 referenced this pull request Jun 10, 2017

Closed

Send desktop notification #16

@alexanderepstein

This comment has been minimized.

alexanderepstein commented Jun 11, 2017

@black-snow So from my index.html I include another javscript file that's not my main.js will I be able to call this by doing something like
const notification = require("electron").notification

And then try to use run the notification there?

Additionally do these notifications have buttons preferebly more then 1? I have it set up where one is a dismiss button and the other one performs an action

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jun 11, 2017

@alexanderepstein In the renderer process you can use the HTML5 Notification API.

new Notification('Hello')

This API is purely for the main process

@black-snow

This comment has been minimized.

black-snow commented Jun 11, 2017

@alexanderepstein also there usually ain't no buttons in notifications. If you need buttons a dialog is what you want. Of course you could also listen to click/dismiss on a notification.

@JBarna

This comment has been minimized.

JBarna commented Jul 5, 2017

@MarshallOfSound Really fantastic work! I found an issue with the icon notifications on windows. It only shows the first icon passed to the Notification module , so concurrent notifications always show the same icon. I tried to look in the code and see if I could spot a simple gap in logic, but I was unable to.

@naheller

This comment has been minimized.

naheller commented Jul 17, 2017

Thanks for everyone's hard work!

What is the status on support for Sticky notifications on macOS?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment