iframe-proxy

sethlu · 2018-01-16T19:34:14Z

🍎 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:

codebytere · 2018-01-17T05:01:06Z

alexeykuzmin

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

alexeykuzmin · 2018-01-17T15:55:11Z

  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.

sethlu · 2018-01-18T17:51:16Z

@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?)

sethlu · 2018-01-18T18:53:36Z

  prototype->SetClassName(mate::StringToV8(isolate, "Notification"));
-  mate::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
-      .MakeDestroyable()
+  auto _(mate::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate()));


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

alexeykuzmin · 2018-01-18T21:48:52Z

@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 · 2018-01-18T22:43:11Z

@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 · 2018-01-19T10:06:14Z

Would cd01eec address this?

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

alexeykuzmin · 2018-01-19T10:11:53Z

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 · 2018-01-19T10:47:34Z

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 · 2018-01-19T10:49:34Z

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 · 2018-01-21T02:46:34Z

@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 · 2018-01-21T08:34:08Z

@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 · 2018-01-21T09:00:08Z

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 · 2018-01-22T17:48:39Z

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

sethlu · 2018-01-22T18:53:25Z

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

alexeykuzmin · 2018-01-22T21:01:11Z

@sethlu Looks good to me. Already approved.

sethlu · 2018-02-07T06:17:42Z

zcbenz

👍

sethlu self-assigned this Jan 16, 2018

sethlu requested review from a team January 16, 2018 19:34

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

sethlu force-pushed the set-notification-close-button-text branch from 767b3a0 to 6f2c60b Compare January 16, 2018 20:09

MarshallOfSound approved these changes Jan 16, 2018

View reviewed changes

alexeykuzmin suggested changes Jan 17, 2018

View reviewed changes

jkleinsc requested a review from a team January 17, 2018 17:25

sethlu added platform/macOS component/notifications labels Jan 18, 2018

sethlu commented Jan 18, 2018

View reviewed changes

alexeykuzmin approved these changes Jan 19, 2018

View reviewed changes

MarshallOfSound requested changes Jan 19, 2018

View reviewed changes

sethlu force-pushed the set-notification-close-button-text branch from adb196a to 878c561 Compare January 22, 2018 18:50

sethlu added 4 commits February 10, 2018 03:19

Make it able to set close button text

be118d4

Update Notification API doc

0d4c6e3

Organize code

29f9929

Add Notification tests

38d2845

sethlu force-pushed the set-notification-close-button-text branch from 36703ac to 38d2845 Compare February 10, 2018 11:20

zcbenz approved these changes Feb 15, 2018

View reviewed changes

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

jwheare mentioned this pull request Jun 5, 2018

Notification actions irccloud/irccloud-desktop#64

Open

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

sethlu commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codebytere commented Jan 17, 2018

Uh oh!

alexeykuzmin left a comment

Choose a reason for hiding this comment

Uh oh!

alexeykuzmin commented Jan 17, 2018

Uh oh!

sethlu commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sethlu Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

alexeykuzmin commented Jan 18, 2018

Uh oh!

sethlu commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexeykuzmin commented Jan 19, 2018

Uh oh!

alexeykuzmin commented Jan 19, 2018

Uh oh!

MarshallOfSound commented Jan 19, 2018

Uh oh!

MarshallOfSound left a comment

Choose a reason for hiding this comment

Uh oh!

MarshallOfSound commented Jan 19, 2018

Uh oh!

sethlu commented Jan 21, 2018

Uh oh!

alexeykuzmin commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarshallOfSound commented Jan 21, 2018

Uh oh!

alexeykuzmin commented Jan 22, 2018

Uh oh!

sethlu commented Jan 22, 2018

Uh oh!

alexeykuzmin commented Jan 22, 2018

Uh oh!

sethlu commented Feb 7, 2018

Uh oh!

zcbenz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sethlu commented Jan 16, 2018 •

edited

Loading

sethlu commented Jan 18, 2018 •

edited

Loading

sethlu commented Jan 18, 2018 •

edited

Loading

alexeykuzmin commented Jan 21, 2018 •

edited

Loading