Merge lp:~charlesk/indicator-datetime/lp-1283142 into lp:indicator-datetime/14.04

[Charles Kerr]

When the notification engine is notify-osd, use bubble notifications instead of the phone's snap decisions.

When the notification engine is notify-osd, use bubble notifications instead of the phone's snap decisions.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:314
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/193/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-trusty-amd64-ci/73
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-trusty-armhf-ci/73
        deb: http://jenkins.qa.ubuntu.com/job/indicator-datetime-trusty-armhf-ci/73/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-datetime-ci/193/rebuild

review: Approve (continuous-integration)

[Sebastien Bacher (seb128)]

Thanks Charles! Looks fine to me but I've one small question

> is_notify_osd = !g_strcmp0(name, "notify-osd");

is special casing notify-osd really the right thing to do there? would it make more sense to use notify_get_server_caps() instead?

review: Needs Information

[Lars Karlitski (larsu)]

I agree with Seb, we should check for the capabilities instead of the server's name.

Why did you remove stop_alarm_sound() from the action callbacks? According to the spec[1], a server doesn't have to send CloseNotification in that case. notify-osd seems to to so, though.

show_notification() handles stopping of sound when the bubble type is snap decision, but not starting the sound. That's decided in notify(). It might make sense to put these together somehow.

I think having sound on the desktop would make a lot of sense as well. Is it gone because there's no easy way to dismiss it without buttons on notifications?

[1] https://developer.gnome.org/notification-spec/

[Charles Kerr (charlesk)]

>> is special casing notify-osd really the right thing to do there?
>> would it make more sense to use notify_get_server_caps() instead?
>
> I agree with Seb, we should check for the capabilities instead of the server's name.

That's reasonable. We could look to see if the notification server supports actions, which unity-notifications does and notify-osd does not. If actions are supported, then we would use the notify_notification_add_action() block of code in show_notification(). seb, larsu, what do you think of that?

> Why did you remove stop_alarm_sound() from the action callbacks?
> According to the spec[1], a server doesn't have to send CloseNotification
> in that case. notify-osd seems to to so, though.

The spec doesn't seem to say that; could you clarify?

Regardless, I don't mind re-adding the stop_alarm_sound() call to the
action callbacks as a safeguard.

> I think having sound on the desktop would make a lot of sense as well.
> Is it gone because there's no easy way to dismiss it without buttons on
> notifications?

Yes, that's the main issue. You don't want to have a looping sound that
you can't dismiss. :)

OTOH, maybe a one-time sound would be appropriate?

[Sebastien Bacher (seb128)]

> If actions are supported, then we would use the notify_notification_add_action() block of code in show_notification(). seb, larsu, what do you think of that?

That seems good to me

> OTOH, maybe a one-time sound would be appropriate?

I we have a sound played on the desktop I think we should have an UI somewhere to opt out from playing it...

[Charles Kerr (charlesk)]

I agree, there needs to be a user-visible way to opt out of audible feedback. We also have higher priority TODO items so close to LTS freeze.

Let's table the audible feedback on desktop for now.

[lp:~charlesk/indicator-datetime/lp-1283142]

315. By Charles Kerr on 2014-02-27

when deciding whether to do a bubble or snap notification, see if the server supports actions.

316. By Charles Kerr on 2014-02-27

when a notify action is activated, call stop_audio_loop() in case the notify server doesn't also send back a 'closed' signal.

[Charles Kerr (charlesk)]

> I agree with Seb, we should check for the capabilities instead of the server's name.

Fixed r315

> According to the spec[1], a server doesn't have to send CloseNotification

Re-added r316

[Lars Karlitski (larsu)]

Looks good to me now. Thanks!

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:316
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/194/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-trusty-amd64-ci/74
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-trusty-armhf-ci/74
        deb: http://jenkins.qa.ubuntu.com/job/indicator-datetime-trusty-armhf-ci/74/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-datetime-ci/194/rebuild

review: Approve (continuous-integration)

=== modified file 'src/snap.cpp'
--- src/snap.cpp	2014-02-05 23:04:00 +0000
+++ src/snap.cpp	2014-02-27 16:15:19 +0000
@@ -27,6 +27,9 @@
 #include <glib/gi18n.h>
 #include <glib.h>
 
+#include <set>
+#include <string>
+
 #define ALARM_SOUND_FILENAME "/usr/share/sounds/ubuntu/stereo/phone-incoming-call.ogg"
 
 namespace unity {
@@ -169,12 +172,57 @@
     data->dismiss(data->appointment);
 }
 
+void on_snap_closed(NotifyNotification*, gpointer)
+{
+    stop_alarm_sound();
+}
+
 void snap_data_destroy_notify(gpointer gdata)
 {
      delete static_cast<SnapData*>(gdata);
 }
 
-void show_snap_decision(SnapData* data)
+std::set<std::string> get_server_caps()
+{
+    std::set<std::string> caps_set;
+    auto caps_gl = notify_get_server_caps();
+    for(auto l=caps_gl; l!=nullptr; l=l->next)
+        caps_set.insert((const char*)l->data);
+    g_list_free_full(caps_gl, g_free);
+    return caps_set;
+}
+
+typedef enum
+{
+    // just a bubble... no actions, no audio
+    NOTIFY_MODE_BUBBLE,
+
+    // a snap decision popup dialog + audio
+    NOTIFY_MODE_SNAP
+}
+NotifyMode;
+
+NotifyMode get_notify_mode()
+{
+    static NotifyMode mode;
+    static bool mode_inited = false;
+
+    if (G_UNLIKELY(!mode_inited))
+    {
+        const auto caps = get_server_caps();
+
+        if (caps.count("actions"))
+            mode = NOTIFY_MODE_SNAP;
+        else
+            mode = NOTIFY_MODE_BUBBLE;
+
+        mode_inited = true;
+    }
+
+    return mode;
+}
+
+void show_notification (SnapData* data, NotifyMode mode)
 {
     const Appointment& appointment = data->appointment;
 
@@ -184,10 +232,14 @@
     const gchar* icon_name = "alarm-clock";
 
     auto nn = notify_notification_new(title, body.c_str(), icon_name);
-    notify_notification_set_hint_string(nn, "x-canonical-snap-decisions", "true");
-    notify_notification_set_hint_string(nn, "x-canonical-private-button-tint", "true");
-    notify_notification_add_action(nn, "show", _("Show"), on_snap_show, data, nullptr);
-    notify_notification_add_action(nn, "dismiss", _("Dismiss"), on_snap_dismiss, data, nullptr);
+    if (mode == NOTIFY_MODE_SNAP)
+    {
+        notify_notification_set_hint_string(nn, "x-canonical-snap-decisions", "true");
+        notify_notification_set_hint_string(nn, "x-canonical-private-button-tint", "true");
+        notify_notification_add_action(nn, "show", _("Show"), on_snap_show, data, nullptr);
+        notify_notification_add_action(nn, "dismiss", _("Dismiss"), on_snap_dismiss, data, nullptr);
+        g_signal_connect(G_OBJECT(nn), "closed", G_CALLBACK(on_snap_closed), data);
+    }
     g_object_set_data_full(G_OBJECT(nn), "snap-data", data, snap_data_destroy_notify);
 
     GError * error = nullptr;
@@ -215,8 +267,17 @@
     data->show = show;
     data->dismiss = dismiss;
 
-    play_alarm_sound();
-    show_snap_decision(data);
+    switch (get_notify_mode())
+    {
+        case NOTIFY_MODE_BUBBLE:
+            show_notification(data, NOTIFY_MODE_BUBBLE);
+            break;
+
+        default:
+            show_notification(data, NOTIFY_MODE_SNAP);
+            play_alarm_sound();
+            break;
+     }
 }
 
 } // unnamed namespace