Merge lp:~charlesk/indicator-datetime/lp-1581089-dont-open-clock-app-when-dismissing-alarms into lp:indicator-datetime

[Charles Kerr]

Differentiate between the 'OK' action in interactive and noninteractive notifications.

Differentiate between the 'OK' action in interactive and noninteractive notifications because we don't want to pop up clock-app when users hit 'OK' to dismiss an alarm.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:451
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/386/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-wily-amd64-ci/76/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-wily-armhf-ci/76/console

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

review: Needs Fixing (continuous-integration)

[lp:~charlesk/indicator-datetime/lp-1581089-dont-open-clock-app-when-dismissing-alarms]

452. By Charles Kerr on 2016-05-14

in tests/glib-fixture.h, tweaks to the new idle_add() and timeout_add() helpers

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:452
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/387/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-wily-amd64-ci/77/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-wily-armhf-ci/77/console

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

review: Needs Fixing (continuous-integration)

[Charles Kerr (charlesk)]

Jenkins isn't going to be happy tho:

 pbuilder-satisfydepends-dummy : Depends: libubuntu-app-launch2-dev (>= 0.9) but it is not going to be installed.

[Renato Araujo Oliveira Filho (renatofilho)]

looks good.

review: Approve

[lp:~charlesk/indicator-datetime/lp-1581089-dont-open-clock-app-when-dismissing-alarms]

453. By Charles Kerr on 2016-05-16

work around g++ 4.9.2 initializer list issue for vivid

454. By Charles Kerr on 2016-05-16

fix glib-fixture.h copyright year in header comment

=== modified file 'include/datetime/snap.h'
--- include/datetime/snap.h	2015-10-13 16:06:35 +0000
+++ include/datetime/snap.h	2016-05-16 21:30:46 +0000
@@ -44,11 +44,11 @@
          const std::shared_ptr<const Settings>& settings);
     virtual ~Snap();
 
-    typedef std::function<void(const Appointment&, const Alarm&)> appointment_func;
+    enum class Response { None, Snooze, ShowApp };
+    typedef std::function<void(const Appointment&, const Alarm&, const Response&)> response_func;
     void operator()(const Appointment& appointment,
                     const Alarm& alarm,
-                    appointment_func snooze,
-                    appointment_func ok);
+                    response_func on_response);
 
 private:
     class Impl;

=== modified file 'src/main.cpp'
--- src/main.cpp	2016-05-14 02:07:10 +0000
+++ src/main.cpp	2016-05-16 21:30:46 +0000
@@ -149,14 +149,20 @@
     auto sound_builder = std::make_shared<uin::DefaultSoundBuilder>();
     std::unique_ptr<Snap> snap (new Snap(notification_engine, sound_builder, state->settings));
     auto alarm_queue = create_simple_alarm_queue(state->clock, snooze_planner, engine, timezone_);
-    auto on_snooze = [snooze_planner](const Appointment& appointment, const Alarm& alarm) {
-        snooze_planner->add(appointment, alarm);
-    };
-    auto on_ok = [actions](const Appointment& app, const Alarm&){
-        actions->open_appointment(app, app.begin);
-    };
-    auto on_alarm_reached = [&engine, &snap, &on_snooze, &on_ok](const Appointment& appointment, const Alarm& alarm) {
-        (*snap)(appointment, alarm, on_snooze, on_ok);
+    auto on_response = [snooze_planner, actions](const Appointment& appointment, const Alarm& alarm, const Snap::Response& response) {
+        switch(response) {
+            case Snap::Response::Snooze:
+                snooze_planner->add(appointment, alarm);
+                break;
+            case Snap::Response::ShowApp:
+                actions->open_appointment(appointment, appointment.begin);
+                break;
+            case Snap::Response::None:
+                break;
+        }
+    };
+    auto on_alarm_reached = [&engine, &snap, &on_response](const Appointment& appointment, const Alarm& alarm) {
+        (*snap)(appointment, alarm, on_response);
         engine->disable_ubuntu_alarm(appointment);
     };
     alarm_queue->alarm_reached().connect(on_alarm_reached);

=== modified file 'src/snap.cpp'
--- src/snap.cpp	2016-05-14 02:07:10 +0000
+++ src/snap.cpp	2016-05-16 21:30:46 +0000
@@ -82,8 +82,7 @@
 
     void operator()(const Appointment& appointment,
                     const Alarm& alarm,
-                    appointment_func snooze,
-                    appointment_func ok)
+                    response_func on_response)
     {
         // If calendar notifications are muted, don't show them
         if (!appointment.is_ubuntu_alarm() && calendar_events_are_muted()) {
@@ -151,28 +150,33 @@
         if (interactive) {
             b.add_hint (uin::Builder::HINT_SNAP);
             b.add_hint (uin::Builder::HINT_AFFIRMATIVE_HINT);
-            b.add_action ("ok", _("OK"));
-            b.add_action ("snooze", _("Snooze"));
+            b.add_action (ACTION_NONE, _("OK"));
+            b.add_action (ACTION_SNOOZE, _("Snooze"));
         } else {
             b.add_hint (uin::Builder::HINT_INTERACTIVE);
-            b.add_action ("ok", _("OK"));
+            b.add_action (ACTION_SHOW_APP, _("OK"));
         }
 
         // add 'sound', 'haptic', and 'awake' objects to the capture so
         // they stay alive until the closed callback is called; i.e.,
         // for the lifespan of the notficiation
-        b.set_closed_callback([appointment, alarm, snooze, ok, sound, awake, haptic]
+        b.set_closed_callback([appointment, alarm, on_response, sound, awake, haptic]
                               (const std::string& action){
-            if (action == "snooze")
-                snooze(appointment, alarm);
-            else if (action == "ok")
-                ok(appointment, alarm);
+            Snap::Response response;
+            if (action == ACTION_SNOOZE)
+                response = Snap::Response::Snooze;
+            else if (action == ACTION_SHOW_APP)
+                response = Snap::Response::ShowApp;
+            else
+                response = Snap::Response::None;
+
+            on_response(appointment, alarm, response);
         });
 
-        //TODO: we need to extend it to support alarms appoitments
+        //TODO: we need to extend it to support alarms appointments
         if (!appointment.is_ubuntu_alarm()) {
-            b.set_timeout_callback([appointment, alarm, ok](){
-                ok(appointment, alarm);
+            b.set_timeout_callback([appointment, alarm, on_response](){
+                on_response(appointment, alarm, Snap::Response::ShowApp);
             });
         }
 
@@ -266,6 +270,10 @@
     std::set<int> m_notifications;
     GCancellable * m_cancellable {nullptr};
     AccountsServiceSound * m_accounts_service_sound_proxy {nullptr};
+
+    static constexpr char const * ACTION_NONE {"none"};
+    static constexpr char const * ACTION_SNOOZE {"snooze"};
+    static constexpr char const * ACTION_SHOW_APP {"show-app"};
 };
 
 /***
@@ -286,10 +294,9 @@
 void
 Snap::operator()(const Appointment& appointment,
                  const Alarm& alarm,
-                 appointment_func show,
-                 appointment_func ok)
+                 response_func on_response)
 {
-  (*impl)(appointment, alarm, show, ok);
+  (*impl)(appointment, alarm, on_response);
 }
 
 /***

=== modified file 'tests/glib-fixture.h'
--- tests/glib-fixture.h	2016-04-12 17:26:42 +0000
+++ tests/glib-fixture.h	2016-05-16 21:30:46 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright 2013 Canonical Ltd.
+ * Copyright 2013-2016 Canonical Ltd.
  *
  * Authors:
  *   Charles Kerr <[e-mail]>
@@ -17,9 +17,9 @@
  * with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef INDICATOR_DATETIME_TESTS_GLIB_FIXTURE_H
-#define INDICATOR_DATETIME_TESTS_GLIB_FIXTURE_H
+#pragma once
 
+#include <chrono>
 #include <functional> // std::function
 #include <map>
 #include <memory> // std::shared_ptr
@@ -200,7 +200,29 @@
       ASSERT_FALSE(wait_for_name_owned(connection, name, timeout_msec, flags)) << "name: " << name;
     }
 
-    GMainLoop * loop;
+    using source_func = std::function<gboolean()>;
+
+    guint idle_add(source_func&& func)
+    {
+      return g_idle_add_full(
+        G_PRIORITY_DEFAULT_IDLE,
+        [](gpointer gf){return (*static_cast<source_func*>(gf))();},
+        new std::function<gboolean()>(func),
+        [](gpointer gf){delete static_cast<source_func*>(gf);}
+      );
+    }
+
+    guint timeout_add(source_func&& func, std::chrono::milliseconds msec)
+    {
+      return g_timeout_add_full(
+        G_PRIORITY_DEFAULT,
+        msec.count(),
+        [](gpointer gf){return (*static_cast<source_func*>(gf))();},
+        new std::function<gboolean()>(func),
+        [](gpointer gf){delete static_cast<source_func*>(gf);}
+      );
+    }
+
+    GMainLoop* loop {};
 };
 
-#endif /* INDICATOR_DATETIME_TESTS_GLIB_FIXTURE_H */

=== modified file 'tests/manual-test-snap.cpp'
--- tests/manual-test-snap.cpp	2016-03-31 18:51:29 +0000
+++ tests/manual-test-snap.cpp	2016-05-16 21:30:46 +0000
@@ -74,12 +74,14 @@
     a.alarms.push_back(Alarm{"Alarm Text", "", a.begin});
 
     auto loop = g_main_loop_new(nullptr, false);
-    auto on_snooze = [loop](const Appointment& appt, const Alarm&){
-        g_message("You clicked 'Snooze' for appt url '%s'", appt.summary.c_str());
-        g_idle_add(quit_idle, loop);
-    };
-    auto on_ok = [loop](const Appointment&, const Alarm&){
-        g_message("You clicked 'OK'");
+    auto on_response = [loop](const Appointment& appt, const Alarm&, const Snap::Response& response){
+        const char* str {""};
+        switch(response) {
+            case Snap::Response::ShowApp: str = "show-app"; break;
+            case Snap::Response::Snooze: str = "snooze"; break;
+            case Snap::Response::None: str = "no-action"; break;
+        };
+        g_message("You clicked '%s' for appt url '%s'", str, appt.summary.c_str());
         g_idle_add(quit_idle, loop);
     };
 
@@ -94,7 +96,7 @@
     auto notification_engine = std::make_shared<uin::Engine>("indicator-datetime-service");
     auto sound_builder = std::make_shared<uin::DefaultSoundBuilder>();
     Snap snap (notification_engine, sound_builder, settings);
-    snap(a, a.alarms.front(), on_snooze, on_ok);
+    snap(a, a.alarms.front(), on_response);
     g_main_loop_run(loop);
 
     g_main_loop_unref(loop);

=== modified file 'tests/test-notification.cpp'
--- tests/test-notification.cpp	2016-05-14 02:07:10 +0000
+++ tests/test-notification.cpp	2016-05-16 21:30:46 +0000
@@ -53,7 +53,7 @@
   auto settings = std::make_shared<Settings>();
   auto ne = std::make_shared<unity::indicator::notifications::Engine>(APP_NAME);
   auto sb = std::make_shared<unity::indicator::notifications::DefaultSoundBuilder>();
-  auto func = [this](const Appointment&, const Alarm&){g_idle_add(quit_idle, loop);};
+  auto func = [this](const Appointment&, const Alarm&, const Snap::Response&){g_idle_add(quit_idle, loop);};
 
   // combinatorial factor #1: event type
   struct {
@@ -143,7 +143,7 @@
 
     // run the test
     auto snap = create_snap(ne, sb, settings);
-    (*snap)(test_appt.appt, appt.alarms.front(), func, func);
+    (*snap)(test_appt.appt, appt.alarms.front(), func);
 
     // confirm that the notification was as expected
     if (expected_notify_called) {
@@ -195,3 +195,96 @@
   }
 }
 
+
+TEST_F(NotificationFixture,Response)
+{
+  // create the world
+  make_interactive();
+  auto ne = std::make_shared<unity::indicator::notifications::Engine>(APP_NAME);
+  auto sb = std::make_shared<unity::indicator::notifications::DefaultSoundBuilder>();
+  auto settings = std::make_shared<Settings>();
+  int next_notification_id {FIRST_NOTIFY_ID};
+
+  // set a response callback that remembers what response we got
+  bool on_response_called {};
+  Snap::Response response {};
+  auto on_response = [this, &on_response_called, &response]
+                     (const Appointment&, const Alarm&, const Snap::Response& r){
+    on_response_called = true;
+    response = r;
+    g_idle_add(quit_idle, loop);
+  };
+
+  // our tests!
+  const struct {
+    Appointment appt;
+    std::vector<std::string> expected_actions;
+    std::string action;
+    Snap::Response expected_response;
+  } tests[] = {
+    { appt,   {"show-app"},       "show-app", Snap::Response::ShowApp },
+    { ualarm, {"none", "snooze"}, "snooze",   Snap::Response::Snooze  },
+    { ualarm, {"none", "snooze"}, "none",     Snap::Response::None    }
+  };
+
+
+  // walk through the tests
+  for (const auto& test : tests)
+  {
+    // wait for previous iterations' bus noise to finish and reset the counters
+    GError* error {};
+    wait_msec(500);
+    dbus_test_dbus_mock_object_clear_method_calls(notify_mock, notify_obj, &error);
+    g_assert_no_error(error);
+    on_response_called = false;
+
+    // create a snap decision
+    auto snap = create_snap(ne, sb, settings);
+    (*snap)(test.appt, test.appt.alarms.front(), on_response);
+
+    // wait for the notification to show up
+    EXPECT_METHOD_CALLED_EVENTUALLY(notify_mock, notify_obj, METHOD_NOTIFY);
+
+    // test that Notify was called the right number of times
+    static constexpr int expected_num_notify_calls {1};
+    guint num_notify_calls {};
+    const auto notify_calls = dbus_test_dbus_mock_object_get_method_calls(
+      notify_mock,
+      notify_obj,
+      METHOD_NOTIFY,
+      &num_notify_calls,
+      &error);
+    g_assert_no_error(error);
+    EXPECT_EQ(expected_num_notify_calls, num_notify_calls);
+
+    // test that Notify was called with the correct list of actions
+    if (num_notify_calls > 0) {
+      std::vector<std::string> actions;
+      const gchar** as {nullptr};
+      g_variant_get_child(notify_calls[0].params, 5, "^a&s", &as);
+      for (int i=0; as && as[i]; i+=2) // actions are in pairs of (name, i18n), skip the i18n
+        actions.push_back(as[i]);
+      EXPECT_EQ(test.expected_actions, actions);
+    }
+
+    // make the notification mock tell the world that the user invoked an action
+    const auto notification_id = next_notification_id++;
+    idle_add([this, notification_id, test](){
+      GError* err {};
+      dbus_test_dbus_mock_object_emit_signal(notify_mock, notify_obj, "ActionInvoked",
+        G_VARIANT_TYPE("(us)"),
+        g_variant_new("(us)", guint(notification_id), test.action.c_str()),
+        &err);
+      dbus_test_dbus_mock_object_emit_signal(notify_mock, notify_obj, "NotificationClosed",
+        G_VARIANT_TYPE("(uu)"),
+        g_variant_new("(uu)", guint(notification_id), guint(NOTIFICATION_CLOSED_DISMISSED)),
+        &err);
+      g_assert_no_error(err);
+      return G_SOURCE_REMOVE;
+    });
+
+    // confirm that the response callback got the right response
+    EXPECT_TRUE(wait_for([&on_response_called](){return on_response_called;}));
+    EXPECT_EQ(int(test.expected_response), int(response)) << "notification_id " << notification_id;
+  }
+}

=== modified file 'tests/test-sound.cpp'
--- tests/test-sound.cpp	2016-05-14 02:07:10 +0000
+++ tests/test-sound.cpp	2016-05-16 21:30:46 +0000
@@ -62,8 +62,8 @@
   make_interactive();
 
   // call the Snap Decision
-  auto func = [this](const Appointment&, const Alarm&){g_idle_add(quit_idle, loop);};
-  (*snap)(appt, appt.alarms.front(), func, func);
+  auto func = [this](const Appointment&, const Alarm&, const Snap::Response&){g_idle_add(quit_idle, loop);};
+  (*snap)(appt, appt.alarms.front(), func);
 
   // confirm that Notify got called once
   guint len = 0;
@@ -148,7 +148,7 @@
   auto settings = std::make_shared<Settings>();
   auto ne = std::make_shared<unity::indicator::notifications::Engine>(APP_NAME);
   auto sb = std::make_shared<TestSoundBuilder>();
-  auto func = [this](const Appointment&, const Alarm&){g_idle_add(quit_idle, loop);};
+  auto func = [this](const Appointment&, const Alarm&, const Snap::Response&){g_idle_add(quit_idle, loop);};
 
   const struct {
       Appointment appointment;
@@ -163,7 +163,7 @@
   auto snap = create_snap(ne, sb, settings);
   for(const auto& test_case : test_cases)
   {
-    (*snap)(test_case.appointment, test_case.appointment.alarms.front(), func, func);
+    (*snap)(test_case.appointment, test_case.appointment.alarms.front(), func);
     EXPECT_EQ(test_case.expected_uri, sb->uri());
     EXPECT_EQ(test_case.expected_role, sb->role());
   }