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

webkit desktop notification #1951

Merged
merged 1 commit into from Jul 29, 2014

Conversation

Projects
None yet
@jtg-gg
Member

jtg-gg commented Jun 10, 2014

Implement webkit dekstop notification (with image url and event dispatcher for click, close, display, error to javascript)
currently only implemented for Windows and MAC

screen shot 2014-06-09 at 6 59 08 pm

windowsnotification

the windows version will need some chrome source modification, I've made the pull request here:
https://github.com/rogerwang/chromium.src/pull/12

Show outdated Hide outdated src/nw_notification_manager.cc
#if defined(OS_MACOSX)
singleton_ = new NotificationManagerMac();
#elif defined(OS_WIN)
singleton_ = new NotificationManagerWin();

This comment has been minimized.

@mschaaf

mschaaf Jun 10, 2014

The indentation is not consistent in this.

@mschaaf

mschaaf Jun 10, 2014

The indentation is not consistent in this.

This comment has been minimized.

@jtg-gg

jtg-gg Jun 10, 2014

Member

sorry about that, I use Xcode and Vstudio, apparently Xcode uses space, Vstudio uses tab

@jtg-gg

jtg-gg Jun 10, 2014

Member

sorry about that, I use Xcode and Vstudio, apparently Xcode uses space, Vstudio uses tab

Show outdated Hide outdated src/nw_notification_manager.cc
NotificationManager* NotificationManager::getSingleton(){
if(singleton_ == nullptr){
#if defined(OS_MACOSX)

This comment has been minimized.

@mschaaf

mschaaf Jun 10, 2014

What happens for linux or other by node-webkit supported platforms?

@mschaaf

mschaaf Jun 10, 2014

What happens for linux or other by node-webkit supported platforms?

This comment has been minimized.

@jtg-gg

jtg-gg Jun 10, 2014

Member

all the notification entry point is from
void ShellContentBrowserClient::ShowDesktopNotification
if the singleton is null, it will do nothing

@jtg-gg

jtg-gg Jun 10, 2014

Member

all the notification entry point is from
void ShellContentBrowserClient::ShowDesktopNotification
if the singleton is null, it will do nothing

This comment has been minimized.

@mschaaf

mschaaf Jun 10, 2014

Does this mean the notifications implemented by your patch are only working for mac and win?

@mschaaf

mschaaf Jun 10, 2014

Does this mean the notifications implemented by your patch are only working for mac and win?

This comment has been minimized.

@jtg-gg

jtg-gg Jun 10, 2014

Member

yeah, as currently I'm working only for MAC and Windows application

@jtg-gg

jtg-gg Jun 10, 2014

Member

yeah, as currently I'm working only for MAC and Windows application

@Mithgol

This comment has been minimized.

Show comment
Hide comment
@Mithgol

Mithgol Jun 10, 2014

Contributor

@jtg-gg It looks gorgeous! 👏

Contributor

Mithgol commented Jun 10, 2014

@jtg-gg It looks gorgeous! 👏

@hachre

This comment has been minimized.

Show comment
Hide comment
@hachre

hachre commented Jun 10, 2014

@jtg-gg Thanks!

@dubcanada

This comment has been minimized.

Show comment
Hide comment
@dubcanada

dubcanada Jun 10, 2014

The indentation and spaces/tabs are all over the place, I do believe node-webkit uses google code style so 4 spaces.

dubcanada commented Jun 10, 2014

The indentation and spaces/tabs are all over the place, I do believe node-webkit uses google code style so 4 spaces.

@dubcanada

This comment has been minimized.

Show comment
Hide comment
@dubcanada

dubcanada Jun 10, 2014

You should make a placeholder for linux.

dubcanada commented Jun 10, 2014

You should make a placeholder for linux.

@rogerwang

This comment has been minimized.

Show comment
Hide comment
@rogerwang

rogerwang Jun 11, 2014

Member

That's for the contribution. It's cool. However, Linux support should be added before it can be merged into master. See also https://groups.google.com/d/msg/node-webkit/W261vvrL2jo/61H23GY2PZkJ

Member

rogerwang commented Jun 11, 2014

That's for the contribution. It's cool. However, Linux support should be added before it can be merged into master. See also https://groups.google.com/d/msg/node-webkit/W261vvrL2jo/61H23GY2PZkJ

@jtg-gg

This comment has been minimized.

Show comment
Hide comment
@jtg-gg

jtg-gg Jun 11, 2014

Member

@rogerwang , I'll fix the indent ( i just wondering, is 2 spaces or 4 ?) and make the place holder for linux
then if you can integrate to the master, then hopefully some one can help with the linux platform ?

Member

jtg-gg commented Jun 11, 2014

@rogerwang , I'll fix the indent ( i just wondering, is 2 spaces or 4 ?) and make the place holder for linux
then if you can integrate to the master, then hopefully some one can help with the linux platform ?

@rogerwang

This comment has been minimized.

Show comment
Hide comment
@rogerwang

rogerwang Jun 11, 2014

Member

it's 2 spaces in this repo.

A place holder might help passing build but it's not real implementation, which should be done before merging into master. Or we'll break the cross-platform promise.

I can look into the linux support after 0.10 release.

Member

rogerwang commented Jun 11, 2014

it's 2 spaces in this repo.

A place holder might help passing build but it's not real implementation, which should be done before merging into master. Or we'll break the cross-platform promise.

I can look into the linux support after 0.10 release.

@jtg-gg

This comment has been minimized.

Show comment
Hide comment
@jtg-gg

jtg-gg Jun 11, 2014

Member

ok thx @rogerwang I've commit the 2 spaces indentation fix and the linux placeholder (i haven't compile it as I don't have linux platform yet)

Member

jtg-gg commented Jun 11, 2014

ok thx @rogerwang I've commit the 2 spaces indentation fix and the linux placeholder (i haven't compile it as I don't have linux platform yet)

@hachre

This comment has been minimized.

Show comment
Hide comment
@hachre

hachre Jun 11, 2014

Given the fact that "Linux" has no unified desktop notification system it might be necessary to define exactly what you mean when you say Linux support. Ubuntu? Gnome? KDE? All 3? At least one of them? All 800 window managers you can possibly find? Do you require a workaround for WMs that don't have native desktop notifications? What exactly is the scope here?

hachre commented Jun 11, 2014

Given the fact that "Linux" has no unified desktop notification system it might be necessary to define exactly what you mean when you say Linux support. Ubuntu? Gnome? KDE? All 3? At least one of them? All 800 window managers you can possibly find? Do you require a workaround for WMs that don't have native desktop notifications? What exactly is the scope here?

@jtg-gg

This comment has been minimized.

Show comment
Hide comment
@jtg-gg

jtg-gg Jun 11, 2014

Member

sorry my bad, is it TOOLKIT_GTK for the linux platform ?

Member

jtg-gg commented Jun 11, 2014

sorry my bad, is it TOOLKIT_GTK for the linux platform ?

@hachre

This comment has been minimized.

Show comment
Hide comment
@hachre

hachre Jun 11, 2014

@jtg-gg: If you mean me, I was asking the project lead for clarification, not you :)

hachre commented Jun 11, 2014

@jtg-gg: If you mean me, I was asking the project lead for clarification, not you :)

@rogerwang

This comment has been minimized.

Show comment
Hide comment
@rogerwang

rogerwang Jun 11, 2014

Member

@hachre we're not trying to define this. We'll follow upstream.

Member

rogerwang commented Jun 11, 2014

@hachre we're not trying to define this. We'll follow upstream.

@dubcanada

This comment has been minimized.

Show comment
Hide comment
@dubcanada

dubcanada Jun 11, 2014

I'll take a look at Linux tonight/tomorrow.

dubcanada commented Jun 11, 2014

I'll take a look at Linux tonight/tomorrow.

@dufferzafar

This comment has been minimized.

Show comment
Hide comment
@dufferzafar

dufferzafar Jun 13, 2014

@jtg-gg I am not in a state to build and test this code myself, can you please provide me the binary (that you might already have) for windows?

dufferzafar commented Jun 13, 2014

@jtg-gg I am not in a state to build and test this code myself, can you please provide me the binary (that you might already have) for windows?

@jtg-gg

This comment has been minimized.

Show comment
Hide comment
@dufferzafar

This comment has been minimized.

Show comment
Hide comment
@dufferzafar

dufferzafar Jun 17, 2014

@jtg-gg Thanks a ton 😄

dufferzafar commented Jun 17, 2014

@jtg-gg Thanks a ton 😄

@mephux

This comment has been minimized.

Show comment
Hide comment
@mephux

mephux Jun 17, 2014

@jtg-gg does that include your setBadgeLabel code as well?

mephux commented Jun 17, 2014

@jtg-gg does that include your setBadgeLabel code as well?

@jtg-gg

This comment has been minimized.

Show comment
Hide comment
@jtg-gg

jtg-gg Jun 18, 2014

Member

@mephux Yes it does include the setBadgeLabel code

Member

jtg-gg commented Jun 18, 2014

@mephux Yes it does include the setBadgeLabel code

@elronalds

This comment has been minimized.

Show comment
Hide comment
@elronalds

elronalds Jun 20, 2014

@jtg-gg Have you thought about adding support for the new Windows 8/8.1 notifications using the Windows.UI.Notifications shown in the top right corner of the screen? That might be useful if the app isn't providing a system tray icon and if targeting the newer OS.

elronalds commented Jun 20, 2014

@jtg-gg Have you thought about adding support for the new Windows 8/8.1 notifications using the Windows.UI.Notifications shown in the top right corner of the screen? That might be useful if the app isn't providing a system tray icon and if targeting the newer OS.

@gpetrov

This comment has been minimized.

Show comment
Hide comment
@gpetrov

gpetrov Jun 22, 2014

@rogerwang could you please make sure this is added in nw 0.10.0

it is such an essential part! Even if it is only for OSX and Windows.
As outlined above on Linux there just too many ways of doing this and we can not wait.

Please merge asap!

gpetrov commented Jun 22, 2014

@rogerwang could you please make sure this is added in nw 0.10.0

it is such an essential part! Even if it is only for OSX and Windows.
As outlined above on Linux there just too many ways of doing this and we can not wait.

Please merge asap!

@octalmage

This comment has been minimized.

Show comment
Hide comment
@octalmage

octalmage Jun 22, 2014

Currently I use this on Mac, works great!

osascript -e 'display notification "Lorem ipsum dolor sit amet" with title "Title"'

octalmage commented Jun 22, 2014

Currently I use this on Mac, works great!

osascript -e 'display notification "Lorem ipsum dolor sit amet" with title "Title"'
@dufferzafar

This comment has been minimized.

Show comment
Hide comment
@dufferzafar

dufferzafar Jun 22, 2014

@jtg-gg The release you've supplied for windows does not work. This is the error I get when trying to execute nw.exe

Problem signature:
  Problem Event Name:   APPCRASH
  Application Name: nw.exe
  Application Version:  0.0.0.0
  Application Timestamp:    539ec0b4
  Fault Module Name:    nw.exe
  Fault Module Version: 0.0.0.0
  Fault Module Timestamp:   539ec0b4
  Exception Code:   80000003
  Exception Offset: 000a7610
  OS Version:   6.1.7601.2.1.0.768.2
  Locale ID:    1033
  Additional Information 1: 0a9e
  Additional Information 2: 0a9e372d3b4ad19135b953a78882e789
  Additional Information 3: 0a9e
  Additional Information 4: 0a9e372d3b4ad19135b953a78882e789

Read our privacy statement online:
  http://go.microsoft.com/fwlink/?linkid=104288&clcid=0x0409

If the online privacy statement is not available, please read our privacy statement offline:
  C:\Windows\system32\en-US\erofflps.txt

dufferzafar commented Jun 22, 2014

@jtg-gg The release you've supplied for windows does not work. This is the error I get when trying to execute nw.exe

Problem signature:
  Problem Event Name:   APPCRASH
  Application Name: nw.exe
  Application Version:  0.0.0.0
  Application Timestamp:    539ec0b4
  Fault Module Name:    nw.exe
  Fault Module Version: 0.0.0.0
  Fault Module Timestamp:   539ec0b4
  Exception Code:   80000003
  Exception Offset: 000a7610
  OS Version:   6.1.7601.2.1.0.768.2
  Locale ID:    1033
  Additional Information 1: 0a9e
  Additional Information 2: 0a9e372d3b4ad19135b953a78882e789
  Additional Information 3: 0a9e
  Additional Information 4: 0a9e372d3b4ad19135b953a78882e789

Read our privacy statement online:
  http://go.microsoft.com/fwlink/?linkid=104288&clcid=0x0409

If the online privacy statement is not available, please read our privacy statement offline:
  C:\Windows\system32\en-US\erofflps.txt
@mephux

This comment has been minimized.

Show comment
Hide comment
@mephux

mephux Jun 22, 2014

@octalmage not sure how that would be a replacement for native notification support? How do you handle on click events?

mephux commented Jun 22, 2014

@octalmage not sure how that would be a replacement for native notification support? How do you handle on click events?

@octalmage

This comment has been minimized.

Show comment
Hide comment
@octalmage

octalmage Jun 23, 2014

@mephux Personally I only need to alert the user so for me it's a good work around until native notification support is implemented.

octalmage commented Jun 23, 2014

@mephux Personally I only need to alert the user so for me it's a good work around until native notification support is implemented.

@jamesmortensen

This comment has been minimized.

Show comment
Hide comment
@jamesmortensen

jamesmortensen Jun 23, 2014

@dufferzafar Are you sure you copied the nwsnapshot.exe and the *.dll's from an existing 0.9.2 build? @jtg-gg didn't ship all the files for Windows so you have to copy them yourself.

Hope this helps!

jamesmortensen commented Jun 23, 2014

@dufferzafar Are you sure you copied the nwsnapshot.exe and the *.dll's from an existing 0.9.2 build? @jtg-gg didn't ship all the files for Windows so you have to copy them yourself.

Hope this helps!

@dufferzafar

This comment has been minimized.

Show comment
Hide comment
@dufferzafar

dufferzafar Jun 23, 2014

@jamesmortensen Thanks. That worked :)

dufferzafar commented Jun 23, 2014

@jamesmortensen Thanks. That worked :)

@sishen

This comment has been minimized.

Show comment
Hide comment
@sishen

sishen Jul 22, 2014

@jtg-gg Thank you very much for the awesome work. I have a problem which I want to confirm whether belongs to Chromuium or node-webkit. So I'll try with the Chromium 35.0.1916.113. Does the notifications addon compatible with the v0.10.0-rc2? Is that possible to make one release, if yes it'll be very helpful, thank you!

sishen commented Jul 22, 2014

@jtg-gg Thank you very much for the awesome work. I have a problem which I want to confirm whether belongs to Chromuium or node-webkit. So I'll try with the Chromium 35.0.1916.113. Does the notifications addon compatible with the v0.10.0-rc2? Is that possible to make one release, if yes it'll be very helpful, thank you!

@rogerwang

This comment has been minimized.

Show comment
Hide comment
@rogerwang

rogerwang Jul 22, 2014

Member

@sishen I believe the patches should be updated to build with v0.10.0. @jtg-gg please update. Thanks.

Member

rogerwang commented Jul 22, 2014

@sishen I believe the patches should be updated to build with v0.10.0. @jtg-gg please update. Thanks.

@jtg-gg

This comment has been minimized.

Show comment
Hide comment
@jtg-gg

jtg-gg Jul 22, 2014

Member

Hi @sishen , @rogerwang will try to update, might need lots of works on my side
meanwhile I've been developing the linux ubuntu notification, using libnotify, here is the screen shot.
Ubuntu notification does not support click event, so I can't sent that event back to javascript

screen shot 2014-07-22 at 3 27 24 pm

Member

jtg-gg commented Jul 22, 2014

Hi @sishen , @rogerwang will try to update, might need lots of works on my side
meanwhile I've been developing the linux ubuntu notification, using libnotify, here is the screen shot.
Ubuntu notification does not support click event, so I can't sent that event back to javascript

screen shot 2014-07-22 at 3 27 24 pm

@sishen

This comment has been minimized.

Show comment
Hide comment
@sishen

sishen Jul 22, 2014

@jtg-gg That looks great! Thank you for all your efforts.

sishen commented Jul 22, 2014

@jtg-gg That looks great! Thank you for all your efforts.

@rogerwang

This comment has been minimized.

Show comment
Hide comment
@rogerwang

rogerwang Jul 23, 2014

Member

@jtg-gg I found that upstream uses Balloon notification on Linux, similar with what you are using on windows, could your impl be based on that? see https://github.com/rogerwang/chromium.src/tree/nw/chrome/browser/ui/gtk/status_icons

Member

rogerwang commented Jul 23, 2014

@jtg-gg I found that upstream uses Balloon notification on Linux, similar with what you are using on windows, could your impl be based on that? see https://github.com/rogerwang/chromium.src/tree/nw/chrome/browser/ui/gtk/status_icons

[Notification] update copyright notice (+1 squashed commit)
Squashed commits:
[b3c450f] [Notification] handle empty body on windows (+1 squashed commits)

Squashed commits:

[ca3f634] [Notification] fix compiler error. missing base:: (+1 squashed commits)

Squashed commits:

[af8b22b] [Notification]fix compile error on linux (+1 squashed commit)
Squashed commits:
[32bc400] compile error fix (+1 squashed commit)
Squashed commits:
[2955c46] [linux] notification implementation (+1 squashed commit)
Squashed commits:
[edb7e83] [WIN] make the StatusTray a singleton, check for the StatusIcon if it has been created before, handle the destruction of StatusIcon and StatusObserver properly (+1 squashed commit)
Squashed commits:
[3a1f53b] [Linux] add placeholder for the desktop notification (+7 squashed commits)
Squashed commits:
[f784e4a] indentation and formatting fix
[c5ef03d] fix the indentation
[d1662e7] [MAC] add version check for the notification image, since it only supported by Mavericks
[18b2d38] [WIN] fix notification manager compile warnings (missing OVERRIDE)
[00177bb] [MAC] implement loading / showing image from url for desktop notification
[e8b8cce] fix compile warning for undefined constructor
[a3d73b2] Move notification image download callback to the parent class, preparation for MAC notification custom icon implementation (+1 squashed commit)
Squashed commits:
[ee010e2] [WIN] implement the icon image display from URL (+8 squashed commits)
Squashed commits:
[4844475] [MAC] implement desktop notification, display event dispatcher
[4c5f445] Remove unnecessary comments
[af4bd7e] Implement notification events dispatcher (close, display, error)
[d6662bc] [WIN] implement windows balloon tray notification, with application icon as the tray icon
[19c8965] [WIN]
AddDesktopNotification, CancelDesktopNotification function prototype adjustment with NotificationManager
[29816f5] [WIN]
implement windows notification
[7191831] [MACOSX]
implement desktop notification event handler (send the click back to java script)
implement "CancelDesktopNotification"
[c5b359d] add notification manager (nw_notification_manager) for desktop notification
implement MacOS notification manager

@jtg-gg jtg-gg closed this Jul 29, 2014

@jtg-gg jtg-gg deleted the jtg-gg:notification branch Jul 29, 2014

@jtg-gg

This comment has been minimized.

Show comment
Hide comment
@jtg-gg

jtg-gg Jul 29, 2014

Member

@rogerwang I've updated the pull request to build 0.10, now it includes linux implementation. However I've looked for the possibility to do linux balloon notification, however it needs lots of works, the tray implementation is broken for ubuntu, needs lots of customisation. hence I keep the libnotify implementation

this pull request will need the chrome source code modification, here https://github.com/rogerwang/chromium.src/pull/12

Member

jtg-gg commented Jul 29, 2014

@rogerwang I've updated the pull request to build 0.10, now it includes linux implementation. However I've looked for the possibility to do linux balloon notification, however it needs lots of works, the tray implementation is broken for ubuntu, needs lots of customisation. hence I keep the libnotify implementation

this pull request will need the chrome source code modification, here https://github.com/rogerwang/chromium.src/pull/12

@jtg-gg jtg-gg reopened this Jul 29, 2014

rogerwang added a commit that referenced this pull request Jul 29, 2014

@island205

This comment has been minimized.

Show comment
Hide comment
@island205

island205 Aug 6, 2014

nice work!

island205 commented Aug 6, 2014

nice work!

@1inus

This comment has been minimized.

Show comment
Hide comment
@1inus

1inus Jun 17, 2017

太棒了

1inus commented Jun 17, 2017

太棒了

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