Add sound, sticky options to notify plugin #1053

brenns10 · 2017-08-19T17:43:17Z

The Notification API has a parameter called requireInteraction. When set to true, the notification will stay visible until the user interacts with it (clicking on settings or clicking the X button both count).

This pull request adds this option to the Notify plugin, so that if you don't notice the "kernel idle" notification, it will still be in your tray when you look. Unfortunately, sound is not yet supported in most browsers, otherwise I would have also added that feature.

brenns10 · 2017-08-22T15:50:10Z

Just a bump on this. Did I miss something from a contributor's guide? Screenshots, etc?

jcb91 · 2017-08-22T16:17:18Z

Just a bump on this. Did I miss something from a contributor's guide? Screenshots, etc?

Nope, sorry, just nobody got round to looking at it, I think 😄

Just a couple of things I'd like to address about config loading.

The first is that it gets much less messy if each nbextension keeps its own top-level object for config, to avoid similarly-named parameters from colliding. You can use dots in parameter names for the configurator, as in e.g.
collapsible_headings.yaml#L9. That'd be a good thing to do here, I think, especially since other options may be added in future.

Second, is how the config is loaded. Although you've used a mechanism which occurs in many of our existing nbextensions, there is a better way! At the moment, lots of nbextneions are independently loading instances of the main notebook config, but the notebook loads one already, which you can use without requiring extra server requests. You can also use jquery to simplify the loading, if you've got a top-level object as I mentioned in the first point.

Finally, your current implementation doesn't actually wait for the config to definitely have loaded before using the parameter values - to do it properly, you need to use the .then pattern as with the updating the parameters.

So, all together, it would look something like

var load_ipython_extension = function () {
    return Jupyter.notebook.config.loaded.then(function () {
        $.extend(true, params, Jupyter.notebook.config.data.notify);
        ensure_permission();
        setup_notifier();
    });
};

This also means you don't need to require utils or config modules 😸

Hope that makes sense - let me know if anything's unclear!

jcb91 · 2017-08-22T16:18:53Z

Also, thanks for your contribution, and apologies it took so long to look at this 😅

jcb91 · 2017-08-22T16:20:13Z

Another thought - although the sound property isn't yet supported, does specifying it cause problems? if not, it could be added now to make this future-proof, I guess 🚀

brenns10 · 2017-08-22T16:44:42Z

@jcb91 Thank you, all that advice makes a lot of sense. I'm testing an update that implements those suggestions.

Also, thanks for your contribution, and apologies it took so long to look at this 😅

Hope I wasn't rude with my bump! I know this is nobody's full-time job :)

Another thought - although the sound property isn't yet supported, does specifying it cause problems? if not, it could be added now to make this future-proof, I guess 🚀

You're right, there shouldn't be any problem implementing it "ahead of time". The only problem then would be that I'd need to serve an appropriately licensed static sound file. I've found the file, but I don't know the mechanisms for serving static files in this project. If you could point me to an example, I would be glad to include the functionality.

brenns10 · 2017-08-22T19:15:07Z

Okay, I implemented the config loading changes. I also found an example of static file hosting in the zenmode extension, and added the support for the sound option. I did test this by inserting a code snippet that will just play the sound (rather than sending the sound option to the Notification API), and it worked properly.

Finally, I added the icon option, so that the browser notification shows the Jupyter logo. This is a feature of jupyter-notify that I missed.

jcb91

Looks good, just some minor alterations to take care of

jcb91 · 2017-08-22T20:17:13Z

src/jupyter_contrib_nbextensions/nbextensions/notify/notify.js

-      var n = new Notification(IPython.notebook.notebook_name, {body: "Kernel is now idle\n(ran for " + Math.round(elapsed_time) + " secs)"});
+      var opts = {
+        body: "Kernel is now idle\n(ran for " + Math.round(elapsed_time) + " secs)",
+        icon: "/static/base/images/favicon.ico",


Neat, but to work correctly with non-default base_url (e.g. on jupyterhub), this should be

Jupyter.notebook.base_url + 'static/base/images/favicon.ico'

jcb91 · 2017-08-22T20:18:25Z

src/jupyter_contrib_nbextensions/nbextensions/notify/notify.js

  "use strict";

+  var params = {
+    sticky: false,
+    sound: false


You could support custom sounds by adding a url as a parameter. Maybe a name like play_sound might be better in case this gets added in future?

jcb91 · 2017-08-22T20:20:29Z

src/jupyter_contrib_nbextensions/nbextensions/notify/notify.js

+  "jquery",
+  "base/js/namespace",
+  "require",
+], function ($, IPython, require) {


It makes more sense to assign the namespace to the Jupyter variable, since that's what it's now called, and what you've used later on in the file. For now, both work since both IPython and Jupyter are defined in the global namespace, but it's less confusing for people reading this file if we stick to the new namespace which we definitely get from the define call...

brenns10 · 2017-08-22T22:15:55Z

Latest commit renames IPython -> Jupyter throughout, renames sound -> play_sound in the config, and fixes the url path.

brenns10 · 2017-08-22T22:47:22Z

One other question: would it be crazy to implement sound support using the Audio API as a fallback? It's like two lines (four if you put it behind a configuration option). That way you can get notification sounds when your kernel completes, even though browsers don't support it yet.

I have to believe that's how most apps (e.g. the Facebook Messenger webapp) do sound for notifications.

jcb91 · 2017-08-23T03:50:15Z

One other question: would it be crazy to implement sound support using the Audio API as a fallback? It's like two lines (four if you put it behind a configuration option). That way you can get notification sounds when your kernel completes, even though browsers don't support it yet.

I have to believe that's how most apps (e.g. the Facebook Messenger webapp) do sound for notifications.

Doesn't sound crazy to me, and as long as you make appropriate checks to ensure nothing breaks if it's not available/asupported, I can't see a reason not to do it 😄

jcb91 · 2017-08-23T03:53:15Z

src/jupyter_contrib_nbextensions/nbextensions/notify/notify.js

+        icon: Jupyter.notebook.base_url + "static/base/images/favicon.ico",
+        requireInteraction: params.sticky
+      };
+      if (params.sound) {


I think this should now be play_sound?

Nice catch. Javascript makes it so easy for these things to go unnoticed.

brenns10 · 2017-08-23T05:46:15Z

Okay, so it took a bit more than 4 lines :)

This last couple commits fixes the forgotten rename, and switches the notification sound mechanism to use the HTMLAudioElement API. This is well supported in HTML5, but shouldn't cause problems if unsupported. I also included the original implementation (the sound option of the Notification API) in comments so it would be easy to switch if browser support improves.

jcb91 · 2017-08-23T12:09:13Z

Looks good to me, thanks @brenns10!

brenns10 · 2017-08-23T16:06:07Z

Thank you! Any idea when a new PyPI release will be?

jcb91 · 2017-08-23T16:07:34Z

Any idea when a new PyPI release will be?

ASAP, I should hope, we (I) have been a bit lax and haven't made one for quite some time...

jcb91 · 2017-08-23T17:59:19Z

so, 0.3.0 should now be live on pypi, conda-forge to follow...

jfbercher · 2017-08-23T18:41:21Z

so, 0.3.0 should now be live on pypi, conda-forge to follow...

Wow!! 👍 👍

brenns10 · 2017-08-23T19:07:14Z

@jcb91 that's awesome, thanks!

Add sticky notification option to notify plugin.

1f4d660

Stephen Brennan added 2 commits August 22, 2017 11:49

Use IPython config object rather than loading our own.

4b37d26

Add support for sound in notify plugin.

13f88a0

brenns10 changed the title ~~Add sticky notification option to notify plugin~~ Add sound, sticky options to notify plugin Aug 22, 2017

jcb91 reviewed Aug 22, 2017

View reviewed changes

Renaming and url fixes within notify extension.

3a10efa

jcb91 reviewed Aug 23, 2017

View reviewed changes

brenns10 added 2 commits August 22, 2017 22:40

Implement notification sounds using HTMLAudioElement

cde7bd6

Update notify documentation.

42c28ac

jcb91 merged commit 3b74c45 into ipython-contrib:master Aug 23, 2017

pyup-bot mentioned this pull request Oct 30, 2017

Pin jupyter_contrib_nbextensions to latest version 0.3.3 zenotech/zCFDSuperBuild#99

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sound, sticky options to notify plugin #1053

Add sound, sticky options to notify plugin #1053

brenns10 commented Aug 19, 2017

brenns10 commented Aug 22, 2017

jcb91 commented Aug 22, 2017 •

edited

Loading

jcb91 commented Aug 22, 2017

jcb91 commented Aug 22, 2017

brenns10 commented Aug 22, 2017

brenns10 commented Aug 22, 2017

jcb91 left a comment •

edited

Loading

jcb91 Aug 22, 2017

jcb91 Aug 22, 2017

jcb91 Aug 22, 2017

brenns10 commented Aug 22, 2017

brenns10 commented Aug 22, 2017

jcb91 commented Aug 23, 2017

jcb91 Aug 23, 2017

brenns10 Aug 23, 2017

brenns10 commented Aug 23, 2017

jcb91 commented Aug 23, 2017

brenns10 commented Aug 23, 2017

jcb91 commented Aug 23, 2017

jcb91 commented Aug 23, 2017

jfbercher commented Aug 23, 2017

brenns10 commented Aug 23, 2017

Add sound, sticky options to notify plugin #1053

Add sound, sticky options to notify plugin #1053

Conversation

brenns10 commented Aug 19, 2017

brenns10 commented Aug 22, 2017

jcb91 commented Aug 22, 2017 • edited Loading

jcb91 commented Aug 22, 2017

jcb91 commented Aug 22, 2017

brenns10 commented Aug 22, 2017

brenns10 commented Aug 22, 2017

jcb91 left a comment • edited Loading

Choose a reason for hiding this comment

jcb91 Aug 22, 2017

Choose a reason for hiding this comment

jcb91 Aug 22, 2017

Choose a reason for hiding this comment

jcb91 Aug 22, 2017

Choose a reason for hiding this comment

brenns10 commented Aug 22, 2017

brenns10 commented Aug 22, 2017

jcb91 commented Aug 23, 2017

jcb91 Aug 23, 2017

Choose a reason for hiding this comment

brenns10 Aug 23, 2017

Choose a reason for hiding this comment

brenns10 commented Aug 23, 2017

jcb91 commented Aug 23, 2017

brenns10 commented Aug 23, 2017

jcb91 commented Aug 23, 2017

jcb91 commented Aug 23, 2017

jfbercher commented Aug 23, 2017

brenns10 commented Aug 23, 2017

jcb91 commented Aug 22, 2017 •

edited

Loading

jcb91 left a comment •

edited

Loading