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

Proposed by Charles Kerr
Status: Needs review
Proposed branch: lp:~charlesk/indicator-datetime/lp-1622682-use-valarm-description-property-in-popups
Merge into: lp:indicator-datetime
Diff against target: 43 lines (+4/-4)
2 files modified
src/snap.cpp (+2/-2)
tests/test-notification.cpp (+2/-2)
To merge this branch: bzr merge lp:~charlesk/indicator-datetime/lp-1622682-use-valarm-description-property-in-popups
Reviewer Review Type Date Requested Status
unity-api-1-bot continuous-integration Approve
dobey (community) Approve
Review via email: mp+305503@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
dobey (dobey) wrote :

Looks ok to me.

review: Approve
Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

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

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

464. By Charles Kerr

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

463. By Charles Kerr

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
86 {86 {
87 // If calendar notifications are disabled, don't show them87 // If calendar notifications are disabled, don't show them
88 if (!appointment.is_ubuntu_alarm() && !calendar_notifications_are_enabled()) {88 if (!appointment.is_ubuntu_alarm() && !calendar_notifications_are_enabled()) {
89 g_debug("Skipping disabled calendar event '%s' notification", appointment.summary.c_str());89 g_debug("Skipping disabled calendar event '%s' notification '%s'", appointment.summary.c_str(), alarm.text.c_str());
90 return;90 return;
91 }91 }
9292
@@ -127,7 +127,7 @@
127 // show a notification...127 // show a notification...
128 const auto minutes = std::chrono::minutes(m_settings->alarm_duration.get());128 const auto minutes = std::chrono::minutes(m_settings->alarm_duration.get());
129 uin::Builder b;129 uin::Builder b;
130 b.set_body (appointment.summary);130 b.set_body (!alarm.text.empty() ? alarm.text : appointment.summary);
131 b.set_icon_name (appointment.is_ubuntu_alarm() ? "alarm-clock" : "calendar-app");131 b.set_icon_name (appointment.is_ubuntu_alarm() ? "alarm-clock" : "calendar-app");
132 b.add_hint (uin::Builder::HINT_NONSHAPED_ICON);132 b.add_hint (uin::Builder::HINT_NONSHAPED_ICON);
133 b.set_start_time (appointment.begin.to_unix());133 b.set_start_time (appointment.begin.to_unix());
134134
=== 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 @@
145145
146 // run the test146 // run the test
147 auto snap = create_snap(ne, sb, settings);147 auto snap = create_snap(ne, sb, settings);
148 (*snap)(test_appt.appt, appt.alarms.front(), func);148 (*snap)(test_appt.appt, test_appt.appt.alarms.front(), func);
149149
150 // confirm that the notification was as expected150 // confirm that the notification was as expected
151 if (expected_notify_called) {151 if (expected_notify_called) {
@@ -189,7 +189,7 @@
189 // test that Notify was called with the appointment's body189 // test that Notify was called with the appointment's body
190 const gchar* body {nullptr};190 const gchar* body {nullptr};
191 g_variant_get_child(notify_calls[0].params, 4, "&s", &body);191 g_variant_get_child(notify_calls[0].params, 4, "&s", &body);
192 ASSERT_STREQ(test_appt.appt.summary.c_str(), body);192 ASSERT_STREQ(test_appt.appt.alarms[0].text.c_str(), body);
193 }193 }
194 }194 }
195 }195 }

Subscribers

People subscribed via source and target branches