Merge lp:~charlesk/indicator-datetime/lp-1320880-shorter-notifications-for-calendar-events into lp:indicator-datetime/rtm-14.09

Proposed by Charles Kerr on 2014-12-10
Status: Approved
Approved by: Ted Gould on 2014-12-16
Approved revision: 391
Proposed branch: lp:~charlesk/indicator-datetime/lp-1320880-shorter-notifications-for-calendar-events
Merge into: lp:indicator-datetime/rtm-14.09
Prerequisite: lp:~charlesk/indicator-datetime/lp-1387231-honor-x-canonical-disabled-tag
Diff against target: 60 lines (+14/-6)
2 files modified
src/snap.cpp (+5/-6)
tests/manual (+9/-0)
To merge this branch: bzr merge lp:~charlesk/indicator-datetime/lp-1320880-shorter-notifications-for-calendar-events
Reviewer Review Type Date Requested Status
Ted Gould (community) 2014-12-10 Approve on 2014-12-16
Michał Sawicz ux Needs Fixing on 2014-12-12
Review via email: mp+244250@code.launchpad.net

Commit message

Change notifications for calendar events s.t. the sound is nonrepeating and the notification is temporary, not requiring user interaction to disappear.

Description of the change

Same MP as proposed for Vivid @ https://code.launchpad.net/~charlesk/indicator-datetime/lp-1320880-shorter-notifications-for-calendar-events/+merge/243939

Description of the Change
=========================

This branch culls the bugfix for bug #1320880 from my development branch so that it's easier to review and to backport in isolation.

It's a fairly simple patch: it changes notifications for calendar events s.t. the sound is nonrepeating and the notification is temporary, not requiring user interaction to disappear.

Checklist
=========

> Are there any related MPs required for this MP to build/function as expected? Please list.

This MP stacks on top of lp:~charlesk/indicator-datetime/lp-1387231-honor-x-canonical-disabled-tag

> Is your branch in sync with latest trunk? (e.g. bzr pull lp:trunk -> no changes)

Yes

> Did the code build without warnings?

Yes

> Did the tests run successfully?

Yes

> Did you perform an exploratory manual test run of your code change and any related functionality?

Yes

> If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

N/A

> What device (or emulator) has your component test plan been executed successfully on?

Krillin running ubuntu-touch/ubuntu-rtm/14.09-proposed image 173

> What manual tests are relevant for this MP?

indicator-datetime/calendar-event-notification

> Did you include a link to the MR Review Checklist Template to make your reviewer's life easier?

https://wiki.ubuntu.com/Process/Merges/Checklists/indicator-datetime

To post a comment you must log in.
Ted Gould (ted) :
review: Approve
Michał Sawicz (saviq) wrote :

I believe these changes need a design review, see bug #1401802.

review: Needs Fixing (ux)
391. By Charles Kerr on 2014-12-12

Make calendar event notifications snap decisions that require manual dismissal, but without looping sound and without haptic feedback. This splits the difference between bug #1320880's complaint "calendar notifications are too intrusive" and bug #1401802's complaint "the new calendar notifications aren't intrusive enough."

Ted Gould (ted) wrote :

Works for me. We need to get comprehensive specs on this. Hopefully with notifications work.

review: Approve
Ted Gould (ted) wrote :

Also, we need an MR to merge r391 into trunk.

Unmerged revisions

391. By Charles Kerr on 2014-12-12

Make calendar event notifications snap decisions that require manual dismissal, but without looping sound and without haptic feedback. This splits the difference between bug #1320880's complaint "calendar notifications are too intrusive" and bug #1401802's complaint "the new calendar notifications aren't intrusive enough."

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 2014-12-12 18:03:01 +0000
3+++ src/snap.cpp 2014-12-12 18:03:02 +0000
4@@ -69,23 +69,20 @@
5 // create the sound...
6 const auto uri = get_alarm_uri(appointment, m_settings);
7 const auto volume = m_settings->alarm_volume.get();
8- const bool loop = m_engine->supports_actions();
9+ const bool loop = appointment.is_ubuntu_alarm() && m_engine->supports_actions();
10 auto sound = std::make_shared<uin::Sound>(uri, volume, loop);
11
12 // create the haptic feedback...
13- const auto haptic_mode = m_settings->alarm_haptic.get();
14+ const auto haptic_mode = appointment.is_ubuntu_alarm() ? m_settings->alarm_haptic.get() : "none";
15 std::shared_ptr<uin::Haptic> haptic;
16 if (haptic_mode == "pulse")
17 haptic = std::make_shared<uin::Haptic>(uin::Haptic::MODE_PULSE);
18
19 // show a notification...
20 const auto minutes = std::chrono::minutes(m_settings->alarm_duration.get());
21- const bool interactive = m_engine->supports_actions();
22 uin::Builder b;
23 b.set_body (appointment.summary);
24 b.set_icon_name ("alarm-clock");
25- b.add_hint (uin::Builder::HINT_SNAP);
26- b.add_hint (uin::Builder::HINT_AFFIRMATIVE_HINT);
27 b.add_hint (uin::Builder::HINT_NONSHAPED_ICON);
28
29 const char * timefmt;
30@@ -103,7 +100,9 @@
31 b.set_title (title);
32 g_free (title);
33 b.set_timeout (std::chrono::duration_cast<std::chrono::seconds>(minutes));
34- if (interactive) {
35+ if (m_engine->supports_actions()) {
36+ b.add_hint (uin::Builder::HINT_SNAP);
37+ b.add_hint (uin::Builder::HINT_AFFIRMATIVE_HINT);
38 b.add_action ("ok", _("OK"));
39 b.add_action ("snooze", _("Snooze"));
40 }
41
42=== modified file 'tests/manual'
43--- tests/manual 2014-12-12 18:03:01 +0000
44+++ tests/manual 2014-12-12 18:03:02 +0000
45@@ -48,6 +48,15 @@
46 <dd>When the alarm is enabled, the alarm icon should reappear.</dd>
47 </dl>
48
49+Test-case indicator-datetime/calendar-event-notification
50+<dl>
51+ <dt>In the calendar app, create and save a new upcoming calendar event that will occur in the next few minutes.</dt>
52+ <dd>The datetime indicator's event list should update itself to show this new event.</dd>
53+ <dd>Calendar events do not get the alarm icon, so no alarm icon should be shown in the header unless there is also an upcoming alarm set.</dd>
54+ <dt>Wait for the event's time to be reached</dt>
55+ <dd>A snap decision notification should appear; but unlike alarms, the sound should be nonrepeating and there should be no haptic feedback.</dd>
56+</dl>
57+
58 Test-case indicator-datetime/alarm-timezone
59 <dl>
60 <dt>In ubuntu-system-settings, change your timezone to a zone you're not in</dt>

Subscribers

People subscribed via source and target branches