Merge lp:~renatofilho/indicator-datetime/fix-1533681 into lp:indicator-datetime/15.10

Proposed by Renato Araujo Oliveira Filho on 2016-03-30
Status: Merged
Approved by: Charles Kerr on 2016-04-04
Approved revision: 454
Merged at revision: 447
Proposed branch: lp:~renatofilho/indicator-datetime/fix-1533681
Merge into: lp:indicator-datetime/15.10
Prerequisite: lp:~renatofilho/indicator-datetime/fix-1508438
Diff against target: 217 lines (+49/-26)
7 files modified
include/datetime/appointment.h (+2/-0)
src/appointment.cpp (+11/-1)
src/engine-eds.cpp (+12/-4)
src/snap.cpp (+3/-1)
tests/notification-fixture.h (+10/-10)
tests/test-eds-ics-repeating-valarms.cpp (+8/-8)
tests/test-sound.cpp (+3/-2)
To merge this branch: bzr merge lp:~renatofilho/indicator-datetime/fix-1533681
Reviewer Review Type Date Requested Status
Charles Kerr (community) 2016-03-30 Approve on 2016-04-04
Arthur Mello (community) Approve on 2016-04-04
Review via email: mp+290520@code.launchpad.net

Commit message

Only play a sound alert if the event contains a SOUND reminder.

Description of the change

How to Test:

1 - Create an alarm on google wit a pop-up notification at event time
2 - Create an alarm on google with e-mail notification at event time
3 - Create an alarm on google without any notification at event time
5 - Create a event with a reminder at event time on your device
4 - Sync your device with google
5 - Make sure that the event with pop-up notification does not play any sound but shows a pop up when it starts
6 - Make sure that the event with e-mail notification does not show any notification and does not play any sound
7 - Make sure the event created on device shows a notification and play a sound when it starts.

To post a comment you must log in.
Charles Kerr (charlesk) wrote :

Basically the right idea, but implementation is much more complicated than necessary. See comments inline

review: Needs Fixing
449. By Renato Araujo Oliveira Filho on 2016-03-31

Remove type property from Alarm.

450. By Renato Araujo Oliveira Filho on 2016-03-31

Update tests.

451. By Renato Araujo Oliveira Filho on 2016-04-01

Does not play sound for events without a sound set.

452. By Renato Araujo Oliveira Filho on 2016-04-01

Parent merged.

453. By Renato Araujo Oliveira Filho on 2016-04-01

better code.

454. By Renato Araujo Oliveira Filho on 2016-04-01

Update tests.

Arthur Mello (artmello) wrote :

lgtm

review: Approve
Charles Kerr (charlesk) wrote :

to me, too

review: Approve
455. By Renato Araujo Oliveira Filho on 2016-04-01

Parent merged.

456. By Renato Araujo Oliveira Filho on 2016-04-06

Parent merged.

457. By Renato Araujo Oliveira Filho on 2016-04-06

Fixed sound file used by event alarms.

458. By Renato Araujo Oliveira Filho on 2016-04-14

Play event sound even if the event has only text reminders.

459. By Renato Araujo Oliveira Filho on 2016-04-15

revert last change.

460. By Renato Araujo Oliveira Filho on 2016-04-19

Parent merged.

461. By Renato Araujo Oliveira Filho on 2016-04-19

Only play sounds for alarms.

462. By Renato Araujo Oliveira Filho on 2016-04-19

Update sound test.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/datetime/appointment.h'
2--- include/datetime/appointment.h 2016-03-16 14:42:06 +0000
3+++ include/datetime/appointment.h 2016-04-19 16:49:39 +0000
4@@ -39,6 +39,8 @@
5 DateTime time;
6
7 bool operator== (const Alarm& that) const;
8+ bool has_sound() const;
9+ bool has_text() const;
10 };
11
12 /**
13
14=== modified file 'src/appointment.cpp'
15--- src/appointment.cpp 2015-04-05 22:27:52 +0000
16+++ src/appointment.cpp 2016-04-19 16:49:39 +0000
17@@ -31,7 +31,17 @@
18 {
19 return (text==that.text)
20 && (audio_url==that.audio_url)
21- && (this->time==that.time);
22+ && (this->time==that.time);
23+}
24+
25+bool Alarm::has_sound() const
26+{
27+ return !audio_url.empty();
28+}
29+
30+bool Alarm::has_text() const
31+{
32+ return !text.empty();
33 }
34
35 bool Appointment::operator==(const Appointment& that) const
36
37=== modified file 'src/engine-eds.cpp'
38--- src/engine-eds.cpp 2016-04-19 16:49:39 +0000
39+++ src/engine-eds.cpp 2016-04-19 16:49:39 +0000
40@@ -603,7 +603,7 @@
41 return ret;
42 }
43
44- static std::string get_alarm_sound_url(ECalComponentAlarm * alarm)
45+ static std::string get_alarm_sound_url(ECalComponentAlarm * alarm, const std::string & default_sound)
46 {
47 std::string ret;
48
49@@ -624,6 +624,8 @@
50
51 icalattach_unref(attach);
52 }
53+ if (ret.empty())
54+ ret = default_sound;
55 }
56
57 return ret;
58@@ -940,11 +942,14 @@
59 DateTime{gtz, ai->occur_end});
60 auto trigger_time = DateTime{gtz, ai->trigger};
61 auto& alarm = alarms[instance_time][trigger_time];
62-
63 if (alarm.text.empty())
64 alarm.text = get_alarm_text(a);
65+
66 if (alarm.audio_url.empty())
67- alarm.audio_url = get_alarm_sound_url(a);
68+ alarm.audio_url = get_alarm_sound_url(a, (baseline.is_ubuntu_alarm() ?
69+ "file://" ALARM_DEFAULT_SOUND :
70+ "file://" CALENDAR_DEFAULT_SOUND));
71+
72 if (!alarm.time.is_set())
73 alarm.time = trigger_time;
74
75@@ -958,7 +963,10 @@
76 appointment.end = i.first.second;
77 appointment.alarms.reserve(i.second.size());
78 for (auto& j : i.second)
79- appointment.alarms.push_back(j.second);
80+ {
81+ if (j.second.has_text() || j.second.has_sound())
82+ appointment.alarms.push_back(j.second);
83+ }
84 subtask->task->appointments.push_back(appointment);
85 }
86 }
87
88=== modified file 'src/snap.cpp'
89--- src/snap.cpp 2016-02-03 17:06:31 +0000
90+++ src/snap.cpp 2016-04-19 16:49:39 +0000
91@@ -102,7 +102,9 @@
92
93 // calendar events are muted in silent mode; alarm clocks never are
94 std::shared_ptr<uin::Sound> sound;
95- if (appointment.is_ubuntu_alarm() || !silent_mode()) {
96+ // FIXME: only play sounds for alarms for now, we should itegrate it with
97+ // system settings to decide if we should play sounds for calendar notification or not
98+ if (appointment.is_ubuntu_alarm()) {
99 // create the sound.
100 const auto role = appointment.is_ubuntu_alarm() ? "alarm" : "alert";
101 const auto uri = get_alarm_uri(appointment, alarm, m_settings);
102
103=== modified file 'tests/notification-fixture.h'
104--- tests/notification-fixture.h 2016-02-10 20:48:24 +0000
105+++ tests/notification-fixture.h 2016-04-19 16:49:39 +0000
106@@ -53,7 +53,7 @@
107 static constexpr char const * HAPTIC_METHOD_VIBRATE_PATTERN {"VibratePattern"};
108
109 static constexpr int SCREEN_COOKIE {8675309};
110- static constexpr char const * SCREEN_METHOD_KEEP_DISPLAY_ON {"keepDisplayOn"};
111+ static constexpr char const * SCREEN_METHOD_KEEP_DISPLAY_ON {"keepDisplayOn"};
112 static constexpr char const * SCREEN_METHOD_REMOVE_DISPLAY_ON_REQUEST {"removeDisplayOnRequest"};
113
114 static constexpr int POWERD_SYS_STATE_ACTIVE = 1;
115@@ -111,7 +111,7 @@
116 const auto christmas = unity::indicator::datetime::DateTime::Local(2015,12,25,0,0,0);
117 appt.begin = christmas.start_of_day();
118 appt.end = christmas.end_of_day();
119- appt.alarms.push_back(unity::indicator::datetime::Alarm{"Ho Ho Ho!", "", appt.begin});
120+ appt.alarms.push_back(unity::indicator::datetime::Alarm{"Ho Ho Ho!", CALENDAR_DEFAULT_SOUND, appt.begin});
121
122 // init an Ubuntu Alarm
123 ualarm.color = "red";
124@@ -165,8 +165,8 @@
125 NOTIFY_INTERFACE,
126 &error);
127 g_assert_no_error(error);
128-
129- // METHOD_GET_INFO
130+
131+ // METHOD_GET_INFO
132 str = g_strdup("ret = ('mock-notify', 'test vendor', '1.0', '1.1')");
133 dbus_test_dbus_mock_object_add_method(notify_mock,
134 notify_obj,
135@@ -196,7 +196,7 @@
136 g_assert_no_error (error);
137 g_free (str);
138
139- // METHOD_CLOSE
140+ // METHOD_CLOSE
141 str = g_strdup_printf("self.EmitSignal('%s', '%s', 'uu', [ args[0], %d ])",
142 NOTIFY_INTERFACE,
143 SIGNAL_CLOSED,
144@@ -223,8 +223,8 @@
145 BUS_POWERD_INTERFACE,
146 &error);
147 g_assert_no_error(error);
148-
149- str = g_strdup_printf ("ret = '%s'", POWERD_COOKIE);
150+
151+ str = g_strdup_printf ("ret = '%s'", POWERD_COOKIE);
152 dbus_test_dbus_mock_object_add_method(powerd_mock,
153 powerd_obj,
154 POWERD_METHOD_REQUEST_SYS_STATE,
155@@ -256,8 +256,8 @@
156 BUS_SCREEN_INTERFACE,
157 &error);
158 g_assert_no_error(error);
159-
160- str = g_strdup_printf ("ret = %d", SCREEN_COOKIE);
161+
162+ str = g_strdup_printf ("ret = %d", SCREEN_COOKIE);
163 dbus_test_dbus_mock_object_add_method(screen_mock,
164 screen_obj,
165 SCREEN_METHOD_KEEP_DISPLAY_ON,
166@@ -287,7 +287,7 @@
167 BUS_HAPTIC_PATH,
168 BUS_HAPTIC_INTERFACE,
169 &error);
170-
171+
172 dbus_test_dbus_mock_object_add_method(haptic_mock,
173 haptic_obj,
174 HAPTIC_METHOD_VIBRATE_PATTERN,
175
176=== modified file 'tests/test-eds-ics-repeating-valarms.cpp'
177--- tests/test-eds-ics-repeating-valarms.cpp 2016-04-19 16:49:39 +0000
178+++ tests/test-eds-ics-repeating-valarms.cpp 2016-04-19 16:49:39 +0000
179@@ -73,14 +73,14 @@
180 ASSERT_EQ(1, appts.size());
181 const auto& appt = appts.front();
182 ASSERT_EQ(8, appt.alarms.size());
183- EXPECT_EQ(Alarm({"Time to pack!", "", DateTime(gtz,2015,4,23,13,35,0)}), appt.alarms[0]);
184- EXPECT_EQ(Alarm({"Time to pack!", "", DateTime(gtz,2015,4,23,13,37,0)}), appt.alarms[1]);
185- EXPECT_EQ(Alarm({"Time to pack!", "", DateTime(gtz,2015,4,23,13,39,0)}), appt.alarms[2]);
186- EXPECT_EQ(Alarm({"Time to pack!", "", DateTime(gtz,2015,4,23,13,41,0)}), appt.alarms[3]);
187- EXPECT_EQ(Alarm({"Go to the airport!", "", DateTime(gtz,2015,4,24,10,35,0)}), appt.alarms[4]);
188- EXPECT_EQ(Alarm({"Go to the airport!", "", DateTime(gtz,2015,4,24,10,37,0)}), appt.alarms[5]);
189- EXPECT_EQ(Alarm({"Go to the airport!", "", DateTime(gtz,2015,4,24,10,39,0)}), appt.alarms[6]);
190- EXPECT_EQ(Alarm({"Go to the airport!", "", DateTime(gtz,2015,4,24,10,41,0)}), appt.alarms[7]);
191+ EXPECT_EQ(Alarm({"Time to pack!", "file://" CALENDAR_DEFAULT_SOUND, DateTime(gtz,2015,4,23,13,35,0)}), appt.alarms[0]);
192+ EXPECT_EQ(Alarm({"Time to pack!", "file://" CALENDAR_DEFAULT_SOUND, DateTime(gtz,2015,4,23,13,37,0)}), appt.alarms[1]);
193+ EXPECT_EQ(Alarm({"Time to pack!", "file://" CALENDAR_DEFAULT_SOUND, DateTime(gtz,2015,4,23,13,39,0)}), appt.alarms[2]);
194+ EXPECT_EQ(Alarm({"Time to pack!", "file://" CALENDAR_DEFAULT_SOUND, DateTime(gtz,2015,4,23,13,41,0)}), appt.alarms[3]);
195+ EXPECT_EQ(Alarm({"Go to the airport!", "file://" CALENDAR_DEFAULT_SOUND, DateTime(gtz,2015,4,24,10,35,0)}), appt.alarms[4]);
196+ EXPECT_EQ(Alarm({"Go to the airport!", "file://" CALENDAR_DEFAULT_SOUND, DateTime(gtz,2015,4,24,10,37,0)}), appt.alarms[5]);
197+ EXPECT_EQ(Alarm({"Go to the airport!", "file://" CALENDAR_DEFAULT_SOUND, DateTime(gtz,2015,4,24,10,39,0)}), appt.alarms[6]);
198+ EXPECT_EQ(Alarm({"Go to the airport!", "file://" CALENDAR_DEFAULT_SOUND, DateTime(gtz,2015,4,24,10,41,0)}), appt.alarms[7]);
199
200 // now let's try this out with AlarmQueue...
201 // hook the planner up to a SimpleAlarmQueue and confirm that it triggers for each of the reminders
202
203=== modified file 'tests/test-sound.cpp'
204--- tests/test-sound.cpp 2016-02-10 20:49:01 +0000
205+++ tests/test-sound.cpp 2016-04-19 16:49:39 +0000
206@@ -155,8 +155,9 @@
207 std::string expected_role;
208 std::string expected_uri;
209 } test_cases[] = {
210- { ualarm, "alarm", path_to_uri(ALARM_DEFAULT_SOUND) },
211- { appt, "alert", path_to_uri(CALENDAR_DEFAULT_SOUND) }
212+ { ualarm, "alarm", path_to_uri(ALARM_DEFAULT_SOUND) }
213+ // No sound for appointments
214+ // { appt, "alert", path_to_uri(CALENDAR_DEFAULT_SOUND) }
215 };
216
217 auto snap = create_snap(ne, sb, settings);

Subscribers

People subscribed via source and target branches