Windows 7 notifications #279

alespergl · 2017-03-16T13:03:35Z

This is the implementation of desktop notifications for Windows 7.

alespergl · 2017-03-16T13:38:25Z

@electron/maintainers Looking for reviewers for this. Thanks

poiru · 2017-03-16T18:26:42Z

browser/win/notification_presenter_win.cc

@@ -35,6 +36,11 @@ bool SaveIconToPath(const SkBitmap& bitmap, const base::FilePath& path) {

 // static
 NotificationPresenter* NotificationPresenter::Create() {
+  auto version = base::win::GetVersion();
+  if (version < base::win::VERSION_WIN7)


Chrome 50 dropped support for Vista so this check isn't strictly needed.

poiru · 2017-03-16T18:28:25Z

browser/win/win32_desktop_notifications/common.h

+    DesktopNotificationController* controller = nullptr;
+
+    std::wstring caption;
+    std::wstring bodyText;


We use snake_case elsewhere in the codebase. Could snake_case for locals and struct members and snake_case_ for class members?

felixrieseberg · 2017-03-17T18:42:37Z

🙇 Thank you so much for this mountain of work. Tons of people will be very happy - I'll try to take a quick look on Monday.

zcbenz · 2017-03-20T22:17:35Z

Does the code under browser/win/win32_desktop_notifications come from an existing library? It seems that the implementation is glueing an library instead of writing everything from scratch.

alespergl · 2017-03-20T23:57:42Z

No, it's 100% new code. That part under the subdirectory is functionally independent, that's maybe why it looks like a separate library - it's structured that way.

felixrieseberg · 2017-03-23T22:19:36Z

This looks 👏 really good. As discussed in person, I'm so very happy we have this PR. Thank you so much! I have only three requests:

1️⃣ As mentioned by @poiru, snake_case would be a good change.
2️⃣ We frequently fail to send notifications using the native ToastNotifier in Windows 8, 8.1, and 10. I would love an option to use this notification object even from versions newer than Windows 7.
3️⃣ I'd love to be able to configure the background color.

Both 2 and 3 could easily be another PR in the future.

EDIT: Talked to @kevinsawicki, let's get this in - we can go off-standard on Notification in a separate PR, allowing us to, for instance, pass type: 'fallback' (or something like that.

felixrieseberg

snake_case, otherwise 👍 💯

alespergl · 2017-03-24T16:02:04Z

I will post an update with snake casing later. I also have a fix for a small DPI related bug that I discovered, but I still need to test it in a multi-monitor setup.

We should think more about the right approach for explicitly selecting the type of notification from the calling code. First thing that comes to mind is consistency across non-Windows OSes. But I do support the general idea.

The color is now derived from the one which the user has selected for windows in the Control Panel. I see some value in being able to change it (e.g. to add category classification). Still, we should give it a bit more thought to make sure the feature doesn't make the user's desktop a playground for every app developer's creativity.

Which brings me to an issue which should be addressed as well at some point - this currently doesn't consider a scenario where multiple Electron apps generate notifications at the same time. Right now these would simply overlap each other.

kevinsawicki · 2017-03-27T16:20:52Z

#285 has been merged which enables cpplint on CI builds so if you rebase/merge master it should pull in those changes.

zcbenz · 2017-03-20T20:40:06Z

browser/notification_presenter.cc

@@ -18,7 +18,8 @@ NotificationPresenter::~NotificationPresenter() {

 base::WeakPtr<Notification> NotificationPresenter::CreateNotification(
    NotificationDelegate* delegate) {
-  Notification* notification = Notification::Create(delegate, this);
+  Notification* notification = CreateNotificationObject(delegate);
+  if (!notification) return {};


notification can't be nullptr here, failed new would throw std::bad_alloc.

We're not calling new here and CreateNotificationObject could return null depending on the implementation, so it's safer to check for it. Alternatively (if we say the return value must not be null) there should at least be an assertion check.

Current design is, if a platform does not have a implementation then we do not create a NotificationPresenter at all (i.e. returning null in GetNotificationPresenter), and if an error happens when creating the native notification, NotificationFailed would be called.

So CreateNotificationObject is guaranteed to return a Notification object with new, and doing assertion would be unnecessary since failed new would simply crash.

akashnimare · 2017-04-01T21:09:42Z

Any progress on this @alespergl 🤗

…er` so that we can create different types of notifications based on runtime conditions.

…ace/line_length] [2]"

…ild/include] [4]"

…ould be: toast.h, c system, c++ system, other. [build/include_order] [4]"

…ng-declarations instead. [build/namespaces] [5]"

…evious line [whitespace/braces] [4]"

…ns] [5]"

…>(...) instead [readability/casting] [4]"

…preceding } [whitespace/newline] [4]"

…ace brightray" [readability/namespace] [5]"

…ss NotificationPresenterWin7 [whitespace/indent] [3]"

…ing in the class [readability/constructors] [3]"

…omments [whitespace/comments] [2]"

… be marked explicit. [runtime/explicit] [5]"

…whitespace/blank_line] [3]"

zcbenz · 2017-04-11T06:06:23Z

Thanks!

kafka-yu · 2018-11-08T09:03:47Z

Hello @alespergl,
Thanks for the great work on this feature.

Could this part of code ported to a single module that can be used in C#?

I'm working on another project in windows forms, which needs to send a notification on windows 7. And I found your solution is the greatest one.

It is very appreciated that if you could you help to give me some guides/tips to do that.
Thank you very much!

alespergl force-pushed the win7_notifications branch 3 times, most recently from 63f9a0f to 9407556 Compare March 16, 2017 16:24

poiru reviewed Mar 16, 2017

View reviewed changes

zcbenz self-assigned this Mar 16, 2017

felixrieseberg suggested changes Mar 23, 2017

View reviewed changes

zcbenz reviewed Mar 28, 2017

View reviewed changes

akashnimare mentioned this pull request Apr 3, 2017

Add desktop notifications support on Windows 7 zulip/zulip-desktop#16

Closed

alespergl added 13 commits April 5, 2017 14:30

Make Notification factory function a member of `NotificationPresent…

0c7f661

…er` so that we can create different types of notifications based on runtime conditions.

Added desktop notifications implementation for Windows 7 (and earlier)

d9eac82

Use NotificationPresenterWin7 on Windows 7

b95c44a

Changed member variable naming style to snake case

1c73635

Address cpplint issue "Lines should be <= 80 characters long [whitesp…

f96f251

…ace/line_length] [2]"

Address cpplint issue "Include the directory when naming .h files [bu…

723c447

…ild/include] [4]"

Address cpplint issue "Found C++ system header after other header. Sh…

7e60771

…ould be: toast.h, c system, c++ system, other. [build/include_order] [4]"

Address cpplint issue "Do not use namespace using-directives. Use usi…

24682e3

…ng-declarations instead. [build/namespaces] [5]"

Address cpplint issue "{ should almost always be at the end of the pr…

08e786c

…evious line [whitespace/braces] [4]"

Address cpplint issue "Missing space before ( in if( [whitespace/pare…

dec29a3

…ns] [5]"

Address cpplint issue "Using C-style cast. Use reinterpret_cast<BYTE*…

d492bc3

…>(...) instead [readability/casting] [4]"

Address cpplint issue "An else should appear on the same line as the …

e0e23e6

…preceding } [whitespace/newline] [4]"

Address cpplint issue "Namespace should be terminated with "// namesp…

e50f7b1

…ace brightray" [readability/namespace] [5]"

alespergl added 7 commits April 5, 2017 14:30

Address cpplint issue "public: should be indented +1 space inside cla…

8978882

…ss NotificationPresenterWin7 [whitespace/indent] [3]"

Address cpplint issue "DISALLOW_COPY_AND_ASSIGN should be the last th…

c7e02a8

…ing in the class [readability/constructors] [3]"

Address cpplint issue "At least two spaces is best between code and c…

bf50790

…omments [whitespace/comments] [2]"

Address cpplint issue "Constructors callable with one argument should…

61cc225

… be marked explicit. [runtime/explicit] [5]"

Address cpplint issue ""public:" should be preceded by a blank line […

552f3a2

…whitespace/blank_line] [3]"

Changed local variable and parameter names to snake case style

63bf90e

Addressed review feedback

8dc68da

alespergl force-pushed the win7_notifications branch from 9407556 to 8dc68da Compare April 5, 2017 12:33

zcbenz merged commit 28d713b into master Apr 11, 2017

zcbenz deleted the win7_notifications branch April 11, 2017 06:06

kevinsawicki mentioned this pull request Apr 11, 2017

Upgrade brightray for Windows 7 notifications electron/electron#9168

Merged

zeke mentioned this pull request May 2, 2017

Added notifications to the demo-application electron/electron-api-demos#304

Merged

NejcZdovc mentioned this pull request Sep 12, 2017

Clicking a desktop notification crashes the browser brave/browser-laptop#10848

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows 7 notifications #279

Windows 7 notifications #279

alespergl commented Mar 16, 2017

alespergl commented Mar 16, 2017

poiru Mar 16, 2017

poiru Mar 16, 2017 •

edited

Loading

felixrieseberg commented Mar 17, 2017

zcbenz commented Mar 20, 2017

alespergl commented Mar 20, 2017

felixrieseberg commented Mar 23, 2017 •

edited

Loading

felixrieseberg left a comment

alespergl commented Mar 24, 2017

kevinsawicki commented Mar 27, 2017

zcbenz Mar 20, 2017

alespergl Mar 28, 2017

zcbenz Mar 29, 2017

akashnimare commented Apr 1, 2017

zcbenz commented Apr 11, 2017

kafka-yu commented Nov 8, 2018 •

edited

Loading

Windows 7 notifications #279

Windows 7 notifications #279

Conversation

alespergl commented Mar 16, 2017

alespergl commented Mar 16, 2017

poiru Mar 16, 2017

Choose a reason for hiding this comment

poiru Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

felixrieseberg commented Mar 17, 2017

zcbenz commented Mar 20, 2017

alespergl commented Mar 20, 2017

felixrieseberg commented Mar 23, 2017 • edited Loading

felixrieseberg left a comment

Choose a reason for hiding this comment

alespergl commented Mar 24, 2017

kevinsawicki commented Mar 27, 2017

zcbenz Mar 20, 2017

Choose a reason for hiding this comment

alespergl Mar 28, 2017

Choose a reason for hiding this comment

zcbenz Mar 29, 2017

Choose a reason for hiding this comment

akashnimare commented Apr 1, 2017

zcbenz commented Apr 11, 2017

kafka-yu commented Nov 8, 2018 • edited Loading

poiru Mar 16, 2017 •

edited

Loading

felixrieseberg commented Mar 23, 2017 •

edited

Loading

kafka-yu commented Nov 8, 2018 •

edited

Loading