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

Notification actions #9837

Merged
merged 7 commits into from Jun 28, 2017

Conversation

Projects
None yet
4 participants
@MarshallOfSound
Member

MarshallOfSound commented Jun 23, 2017

Adds support for "notification actions" to the main process Notification API.

The only supported action at the moment is "button" on macOS. In theory this could be expanded to support windows as well in a future PR along with other types of actions (like dropdowns in windows)

@MarshallOfSound MarshallOfSound requested review from zcbenz and kevinsawicki Jun 23, 2017

@ccnokes

This comment has been minimized.

Contributor

ccnokes commented Jun 23, 2017

Any plans on supporting the reply option for macOS notifications?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jun 24, 2017

@ccnokes The reply option is already supported on macOS check out the main process notification docs.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jun 24, 2017

This now includes a refactor to make it infinitely easier to add support for more options later down the line

// Prop Setters
void SetTitle(const base::string16& new_title);
void SetBody(const base::string16& new_body);
void SetSilent(bool new_silent);
void SetReplyPlaceholder(const base::string16& new_reply_placeholder);
void SetHasReply(bool new_has_reply);
void SetActions(const std::vector<brightray::NotificationAction> actions);

This comment has been minimized.

@zcbenz

zcbenz Jun 28, 2017

Contributor

Should pass const reference here: const std::vector<...>&.

NSUserNotification* notification() const { return notification_; }
private:
base::scoped_nsobject<NSUserNotification> notification_;
int actionIndex_;

This comment has been minimized.

@zcbenz

zcbenz Jun 28, 2017

Contributor

Should use snake_case in C++: action_index_.

[notification_ setHasActionButton:false];
for (size_t i = 0; i < options.actions.size(); i++) {
NotificationAction action = options.actions[i];

This comment has been minimized.

@zcbenz

zcbenz Jun 28, 2017

Contributor

You can use ranged for to save the extra copy:

for (const auto& action : options.actions) {
  ...
}
for (size_t i = 0; i < options.actions.size(); i++) {
NotificationAction action = options.actions[i];
if (action.type == base::UTF8ToUTF16("button")) {

This comment has been minimized.

@zcbenz

zcbenz Jun 28, 2017

Contributor

UTF8ToUTF16 is expensive, you should use action.type == base::ASCIIToUTF16("button").

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jun 28, 2017

@zcbenz Updated 😄

@zcbenz

zcbenz approved these changes Jun 28, 2017

@kevinsawicki kevinsawicki merged commit 0f83180 into master Jun 28, 2017

3 of 6 checks passed

electron-win-ia32 Build #3487 failed in 9 min 1 sec
Details
electron-win-x64 Build #3481 failed in 7 min 5 sec
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
electron-mas-x64 Build #4506 succeeded in 9 min 28 sec
Details
electron-osx-x64 Build #4512 succeeded in 10 min
Details

@kevinsawicki kevinsawicki deleted the notification-actions branch Jun 28, 2017

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jun 28, 2017

Thanks @MarshallOfSound 👍 🎀

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