Indicator Date and Time

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

  1. fix-1533681
  2. Merge into trunk.15.10
Proposed by Renato Araujo Oliveira Filho
Status: Merged
Approved by: Charles Kerr
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) Approve
Arthur Mello (community) Approve
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.
Revision history for this message
Charles Kerr (charlesk) wrote :

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

review: Needs Fixing
lp:~renatofilho/indicator-datetime/fix-1533681 updated
449. By Renato Araujo Oliveira Filho

Remove type property from Alarm.

450. By Renato Araujo Oliveira Filho

Update tests.

451. By Renato Araujo Oliveira Filho

Does not play sound for events without a sound set.

452. By Renato Araujo Oliveira Filho

Parent merged.

453. By Renato Araujo Oliveira Filho

better code.

454. By Renato Araujo Oliveira Filho

Update tests.

Revision history for this message
Arthur Mello (artmello) wrote :

lgtm

review: Approve
Revision history for this message
Charles Kerr (charlesk) wrote :

to me, too

review: Approve
lp:~renatofilho/indicator-datetime/fix-1533681 updated
455. By Renato Araujo Oliveira Filho

Parent merged.

456. By Renato Araujo Oliveira Filho

Parent merged.

457. By Renato Araujo Oliveira Filho

Fixed sound file used by event alarms.

458. By Renato Araujo Oliveira Filho

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

459. By Renato Araujo Oliveira Filho

revert last change.

460. By Renato Araujo Oliveira Filho

Parent merged.

461. By Renato Araujo Oliveira Filho

Only play sounds for alarms.

462. By Renato Araujo Oliveira Filho

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