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

Proposed by Charles Kerr on 2016-09-12
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 on 2016-09-12
dobey (community) 2016-09-12 Approve on 2016-09-12
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.
dobey (dobey) wrote :

Looks ok to me.

review: Approve
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 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/snap.cpp'
2--- src/snap.cpp 2016-07-04 23:44:47 +0000
3+++ src/snap.cpp 2016-09-12 17:30:31 +0000
4@@ -86,7 +86,7 @@
5 {
6 // If calendar notifications are disabled, don't show them
7 if (!appointment.is_ubuntu_alarm() && !calendar_notifications_are_enabled()) {
8- g_debug("Skipping disabled calendar event '%s' notification", appointment.summary.c_str());
9+ g_debug("Skipping disabled calendar event '%s' notification '%s'", appointment.summary.c_str(), alarm.text.c_str());
10 return;
11 }
12
13@@ -127,7 +127,7 @@
14 // show a notification...
15 const auto minutes = std::chrono::minutes(m_settings->alarm_duration.get());
16 uin::Builder b;
17- b.set_body (appointment.summary);
18+ b.set_body (!alarm.text.empty() ? alarm.text : appointment.summary);
19 b.set_icon_name (appointment.is_ubuntu_alarm() ? "alarm-clock" : "calendar-app");
20 b.add_hint (uin::Builder::HINT_NONSHAPED_ICON);
21 b.set_start_time (appointment.begin.to_unix());
22
23=== modified file 'tests/test-notification.cpp'
24--- tests/test-notification.cpp 2016-06-24 12:26:06 +0000
25+++ tests/test-notification.cpp 2016-09-12 17:30:31 +0000
26@@ -145,7 +145,7 @@
27
28 // run the test
29 auto snap = create_snap(ne, sb, settings);
30- (*snap)(test_appt.appt, appt.alarms.front(), func);
31+ (*snap)(test_appt.appt, test_appt.appt.alarms.front(), func);
32
33 // confirm that the notification was as expected
34 if (expected_notify_called) {
35@@ -189,7 +189,7 @@
36 // test that Notify was called with the appointment's body
37 const gchar* body {nullptr};
38 g_variant_get_child(notify_calls[0].params, 4, "&s", &body);
39- ASSERT_STREQ(test_appt.appt.summary.c_str(), body);
40+ ASSERT_STREQ(test_appt.appt.alarms[0].text.c_str(), body);
41 }
42 }
43 }

Subscribers

People subscribed via source and target branches