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

Add support for soundName in main process notifications #10293

Merged
merged 4 commits into from Aug 21, 2017

Conversation

Projects
None yet
5 participants
@CharlieHess
Copy link
Contributor

CharlieHess commented Aug 17, 2017

In order for Slack to use the new main process notifications, we need support for the soundName property. Fortunately this is a cinch to add. To test this, open the default app and in the console, make a notification like:

remote = require('electron').remote;
Notification = remote.Notification;
new Notification({ title: 'Bonk', sound: 'Submarine' }).show();

You can also play custom sounds, but you'll need to copy the sound files into the app bundle or one of these locations:

  • ~/Library/Sounds
  • /Library/Sounds
  • /Network/Library/Sounds
  • /System/Library/Sounds
@@ -33,6 +33,7 @@ struct NotificationOptions {
bool silent;
bool has_reply;
base::string16 reply_placeholder;
base::string16 sound_name;

This comment has been minimized.

@felixrieseberg

felixrieseberg Aug 17, 2017

Member

How about calling this sound in accordance with https://developer.mozilla.org/en-US/docs/Web/API/Notification/sound?

@enlight

This comment has been minimized.

Copy link
Contributor

enlight commented Aug 18, 2017

You can also play custom sounds, but you'll need to copy the sound files into the app bundle or one of these locations:

~/Library/Sounds
/Library/Sounds
/Network/Library/Sounds
/System/Library/Sounds

Can you put that in the docs too?

@@ -37,6 +37,7 @@ Returns `Boolean` - Whether or not desktop notifications are supported on the cu
* `icon` [NativeImage](native-image.md) - (optional) An icon to use in the notification
* `hasReply` Boolean - (optional) Whether or not to add an inline reply option to the notification. _macOS_
* `replyPlaceholder` String - (optional) The placeholder to write in the inline reply input field. _macOS_
* `sound` String - (optional) The name of the sound file to play when the notification is shown. _macOS_

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Aug 18, 2017

Member

Should probably link here a list of the sounds names you can pass in. or at least linking to the NSSound docs

@@ -207,7 +217,9 @@ void Notification::BuildPrototype(v8::Isolate* isolate,
.SetProperty("hasReply", &Notification::GetHasReply,
&Notification::SetHasReply)
.SetProperty("actions", &Notification::GetActions,
&Notification::SetActions);
&Notification::SetActions)
.SetProperty("soundName", &Notification::GetSoundName,

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Aug 18, 2017

Member

Having the cpp variable as sound_ but the methods as soundName is a bit inconsistent, should probably choosing one and stick to it on both JS and cpp side of things

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Aug 18, 2017

Tbh my take away from this PR is what on earth does the "Submarine" sound make 😆

@zeke

This comment has been minimized.

Copy link
Member

zeke commented Aug 21, 2017

what on earth does the "Submarine" sound make

It's like an echoing sonar ping 📡

CharlieHess added some commits Aug 21, 2017

@CharlieHess

This comment has been minimized.

Copy link
Contributor

CharlieHess commented Aug 21, 2017

this should be all good to go. I'ma merge when CI goes green.

bitmoji

@CharlieHess CharlieHess merged commit f17bd04 into master Aug 21, 2017

3 of 5 checks passed

electron-mas-x64 Build #4904 failed in 11 min
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-osx-x64 Build #4894 succeeded in 10 min
Details
electron-win-ia32 Build #3917 succeeded in 12 min
Details

@CharlieHess CharlieHess deleted the notification-sounds branch Aug 21, 2017

felixrieseberg added a commit that referenced this pull request Aug 28, 2017

Merge pull request #10293 from electron/notification-sounds
Add support for soundName in main process notifications

jkleinsc added a commit that referenced this pull request Aug 29, 2017

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