Merge lp:~charlesk/indicator-datetime/lp-1622682-use-valarm-description-property-in-popups into lp:indicator-datetime

[Charles Kerr]

Use the valarm 'description' property correctly when displaying alarm popups

Use the valarm 'description' property correctly when displaying alarm popups.

The code is currently the vevent's summary property in the popup notification, but should be displaying the valarm's description property instead. (RFC 2445: 'When the action is "DISPLAY", the [v]alarm MUST also include a "DESCRIPTION" property, which contains the text to be displayed when the alarm is triggered.')

After looking through the code history, I think this bug comes from the time when we used to only handle one valarm per vevent. This code didn't get fixed when event-to-alarm became a one-to-many relationship.

[dobey (dobey)]

Looks ok to me.

review: Approve

[unity-api-1-bot (unity-api-1-bot)]

PASSED: Continuous integration, rev:465
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-datetime-ci/1/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/603
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/609
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/433
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/433/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/433
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/433/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/433
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/433/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/433
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/433/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/433
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/433/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/433
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/433/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/433
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/433/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/433
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/433/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/433
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/433/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-indicator-datetime-ci/1/rebuild

review: Approve (continuous-integration)

Unmerged revisions

465. By Charles Kerr on 2016-09-12

sync tests to r463 to look for Alarm.description as the popup notification's text

464. By Charles Kerr on 2016-09-12

fix test-notification bug that invoked Snap() with the wrong Alarm argument

463. By Charles Kerr on 2016-09-12

use Alarm.text, rather than Appointment.summary, when showing popup notifications for an alarm.

=== modified file 'src/snap.cpp'
--- src/snap.cpp	2016-07-04 23:44:47 +0000
+++ src/snap.cpp	2016-09-12 17:30:31 +0000
@@ -86,7 +86,7 @@
     {
         // If calendar notifications are disabled, don't show them
         if (!appointment.is_ubuntu_alarm() && !calendar_notifications_are_enabled()) {
-            g_debug("Skipping disabled calendar event '%s' notification", appointment.summary.c_str());
+            g_debug("Skipping disabled calendar event '%s' notification '%s'", appointment.summary.c_str(), alarm.text.c_str());
             return;
         }
 
@@ -127,7 +127,7 @@
         // show a notification...
         const auto minutes = std::chrono::minutes(m_settings->alarm_duration.get());
         uin::Builder b;
-        b.set_body (appointment.summary);
+        b.set_body (!alarm.text.empty() ? alarm.text : appointment.summary);
         b.set_icon_name (appointment.is_ubuntu_alarm() ? "alarm-clock" : "calendar-app");
         b.add_hint (uin::Builder::HINT_NONSHAPED_ICON);
         b.set_start_time (appointment.begin.to_unix());

=== modified file 'tests/test-notification.cpp'
--- tests/test-notification.cpp	2016-06-24 12:26:06 +0000
+++ tests/test-notification.cpp	2016-09-12 17:30:31 +0000
@@ -145,7 +145,7 @@
 
     // run the test
     auto snap = create_snap(ne, sb, settings);
-    (*snap)(test_appt.appt, appt.alarms.front(), func);
+    (*snap)(test_appt.appt, test_appt.appt.alarms.front(), func);
 
     // confirm that the notification was as expected
     if (expected_notify_called) {
@@ -189,7 +189,7 @@
         // test that Notify was called with the appointment's body
         const gchar* body {nullptr};
         g_variant_get_child(notify_calls[0].params, 4, "&s", &body);
-        ASSERT_STREQ(test_appt.appt.summary.c_str(), body);
+        ASSERT_STREQ(test_appt.appt.alarms[0].text.c_str(), body);
     }
   }
   }