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

feat: Set macOS notification close button title #11654

Merged
merged 4 commits into from Feb 15, 2018

Conversation

Projects
None yet
5 participants
@sethlu
Member

sethlu commented Jan 16, 2018

馃崕 This addition allows developers to set custom & localized title for macOS notifications.

Resolves: #11594


With the following config:

  • NSUserNotificationAlertStyle set to alert in Info.plist of the app bundle
  • Probably check if alerts are enabled for the app in System Preferences (not only banners, otherwise the close button won't show)

Using the following script:

// This doesn't produce a window...
// But should be enough to produce a native notification
const electron = require('electron')
electron.app.on('ready', function (event) {

  let n = new electron.Notification({
    title: 'Notification',
    closeButtonText: 'Dismiss 馃榾', // It does support some emoji
  })

  n.show()

})

Screenshot:

screen shot 2018-01-16 at 11 26 20 am

@sethlu sethlu self-assigned this Jan 16, 2018

@sethlu sethlu requested review from electron/notifications as code owners Jan 16, 2018

@sethlu sethlu changed the title from [WIP] feat: Set macOS notification close button title to feat: Set macOS notification close button title Jan 16, 2018

@codebytere

This comment has been minimized.

Member

codebytere commented Jan 17, 2018

@sethlu this is 馃挴 , dope work! gonna wait on that last build then i'll merge 馃榿

@alexeykuzmin

Shouldn't the whole feature be placed under the #if defined(OS_MACOSX) condition?

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Jan 17, 2018

  let n = new electron.Notification({
    title: 'Notification',
    closeButtonText: 'Dismiss 馃榾', // It does support some emoji
  })

@sethlu Can you please add a test that n.closeButtonText value is equal to the provided text?
And another test when closeButtonText is set to an empty string.

@jkleinsc jkleinsc requested a review from electron/reviewers as a code owner Jan 17, 2018

@sethlu

This comment has been minimized.

Member

sethlu commented Jan 18, 2018

@alexeykuzmin Thanks for reviewing! This change set introduces a new member to the existing NotificationOptions struct. On platforms other than macOS, this closeButtonText will not be checked while interacting with the system API calls. However, I could wrap some of the macOS-only properties for optimization? (This hasn't been done yet however, probably due to it being an experimental feature? On that I also can't seem to find the notifications under the spec folder?)

@@ -214,23 +179,28 @@ bool Notification::IsSupported() {
void Notification::BuildPrototype(v8::Isolate* isolate,
v8::Local<v8::FunctionTemplate> prototype) {
prototype->SetClassName(mate::StringToV8(isolate, "Notification"));
mate::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
.MakeDestroyable()
auto _(mate::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate()));

This comment has been minimized.

@sethlu

sethlu Jan 18, 2018

Member

Not sure of a better way to do this while keeping the items in order?

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Jan 18, 2018

@sethlu

On platforms other than macOS, this closeButtonText will not be checked while interacting with the system API calls. However, I could wrap some of the macOS-only properties for optimization?

I wasn't thinking about optimization, but about explicitly saying "these parts of code are Mac-specific".

I also can't seem to find the notifications under the spec folder

You are more than welcome to create a new file )
You can skip .closeButtonText tests on all platforms other than Mac by calling this.skip() in a before hook of a describe.

@sethlu

This comment has been minimized.

Member

sethlu commented Jan 18, 2018

@alexeykuzmin

I wasn't thinking about optimization, but about explicitly saying "these parts of code are Mac-specific".

Would cd01eec address this? The feature's intended for macOS; historical additions to the Notifications API aren't so sensitive with the platform settings?

You are more than welcome to create a new file )

I'll probably address that in a separate PR since the entire Notification submodule needs its tests--probably only checking if the values are passed correctly to the electron lib?

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Jan 19, 2018

Would cd01eec address this?

Looks good to me, but I would like to have someone else to review this too.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Jan 19, 2018

You are more than welcome to create a new file )

I'll probably address that in a separate PR

I just means that we risk to get some stuff broken in the master )

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jan 19, 2018

Would cd01eec address this?

I think we should discuss this more, not sure that I like the idea of pulling out these properties of Notification to be just on macOS. For instance there are PR's / WIP's to implement actions / replies on Windows. I'm all for being clear on where features are noops / implemented but the OS specific parts of Notification are in brightray currently and I believe having two sets of OS split files (brightray and atom_api_notification) will make dev harder / more confusing rather than easier.

@MarshallOfSound

requesting to discuss

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jan 19, 2018

To further explain my point above the atom_api_notification file could be considered a "proxy" for the internal brightray notification API. It shouldn't be aware what OS does what / supports rather just pass a complete NotificationOptions object to the API and let the OS specific stuff in brightray handle the options as they see fit 馃憤

@sethlu

This comment has been minimized.

Member

sethlu commented Jan 21, 2018

@MarshallOfSound That's a good point. I think it's probably better to have those options all available in NotificationOptions so different platform implementations could read the applicable entries.

@alexeykuzmin What do you think?

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Jan 21, 2018

@MarshallOfSound @sethlu

the atom_api_notification file could be considered a "proxy" for the internal brightray notification API. It shouldn't be aware what OS does what / supports rather just pass a complete NotificationOptions object to the API and let the OS specific stuff in brightray handle the options as they see fit

atom_api_notification is a user-facing interface, and it should match the documentation.
brightray is an implementation behind that interface, probably platform dependent.
We can have some stuff implemented in brightray without an API available for users,
but we can't have an API without implementation. Like if some API method is not available on a certain platform, the method shouldn't be noop, it should not exist at all.

That's why I think that we should have OS checks in the atom_api_notification.
I don't really care if some methods will end up in a separate file or not.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jan 21, 2018

Like if some API method is not available on a certain platform, the method shouldn't be noop, it should not exist at all.

This goes against how most of our current API's work afaik, OS specific getters and setters exist most of the time on all platforms but only take affect on some. This is so that you can write JS code for all platforms without doing platform checks. I.e. I as a user shouldn't have to do a if (process.platform === 'darwin') check just to see if I can get/set the hasReply property. We should make writing cross-platform as easy for users as possible (that's kind of our thing 馃槃 )

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Jan 22, 2018

@MarshallOfSound Maybe it's a good way to use API, if we have platform-specific properties, ok.

@sethlu

This comment has been minimized.

Member

sethlu commented Jan 22, 2018

@MarshallOfSound @alexeykuzmin I've just reverted the platform-specific code changes.
Let me know how it looks.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Jan 22, 2018

@sethlu Looks good to me. Already approved.

@sethlu

This comment has been minimized.

Member

sethlu commented Feb 7, 2018

@MarshallOfSound @alexeykuzmin I've been a little busy lately. But just got to add some test cases to the existing Notification API.

@zcbenz

zcbenz approved these changes Feb 15, 2018

馃憤

@zcbenz zcbenz merged commit af92b04 into electron:master Feb 15, 2018

7 of 8 checks passed

ci/circleci: electron-linux-x64 Your tests failed on CircleCI
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@jwheare jwheare referenced this pull request Jun 5, 2018

Open

Notification actions #64

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