Merge lp:~townsend/unity/fix-urgent-icon-wiggle into lp:unity

Proposed by Christopher Townsend
Status: Merged
Approved by: Marco Trevisan (Treviño)
Approved revision: no longer in the source branch.
Merged at revision: 3367
Proposed branch: lp:~townsend/unity/fix-urgent-icon-wiggle
Merge into: lp:unity
Diff against target: 287 lines (+204/-0)
3 files modified
launcher/Launcher.cpp (+123/-0)
launcher/Launcher.h (+10/-0)
tests/test_launcher.cpp (+71/-0)
To merge this branch: bzr merge lp:~townsend/unity/fix-urgent-icon-wiggle
Reviewer Review Type Date Requested Status
Marco Trevisan (Treviño) Approve
PS Jenkins bot (community) continuous-integration Approve
Brandon Schaefer (community) Approve
Review via email: mp+163800@code.launchpad.net

Commit message

Add functionality to wiggle urgent icons at certain time intervals when the Launcher is hidden.

Description of the change

= Issue =
Currently, when the Launcher is hidden and an urgent icon is displayed, the only notification a user sees that the initial pop out wiggle of the icon and then nothing else. IF the user misses this, then they may be unaware of an app that needs attention.

= Fix =
Add timers and logic to pop out and wiggle the icon at certain intervals as defined in bug #893196. Also, handle the wiggle icons once the Launcher is revealed.

= Test =
This is really a visual issue and no tests automated tests are identified to handle this.
A manual test would be to have an application set an icon urgent and watch for the pop out wiggle at the defined intervals. Also, reveal the Launcher to see the icon(s) wiggle one last time.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

Looks nice :)

review: Approve
Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

Yay

review: Approve
Revision history for this message
Christopher Townsend (townsend) wrote :

Setting the overall status to Needs Review since I'd like to get Marco's input on tests since he said it may be possible for automated tests.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

68 +void Launcher::HandleUrgentIcon(AbstractLauncherIcon::Ptr const& icon, struct timespec const& current)

Since the function is called at every redraw, why not just returning at the beginning of it in case of: if (options()->hide_mode == LAUNCHER_HIDE_NEVER) ?

187 + bool _urgent_acked;
Please include _urgent_acked in the initialization list as well. Also, even if it seems to work properly even if there's more than on icon, wouldn't probably be better to have a set of icons to wiggle instead?

124 + for (it = _model->main_begin(); it != _model->main_end(); ++it)

Using a for-each loop here is nicer...

127 + if (icon->GetQuirk(AbstractLauncherIcon::Quirk::URGENT) &&

Traling space here

153 + sources_.Remove(URGENT_TIMEOUT);

This is not needed since it's automatically done when returning false on this callback function (see line 238 of SourceManager::Add on GLibSource.cpp).

157 + SetUrgentTimer(_urgent_wiggle_time);

Mh, why the current timer is not deleted in case you set up a new timer with different period?

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

Ah, on the bug description there's written:
«- When the Launcher is visible, application alerts should should be indicated by a short wiggle (in addition to the existing blue pips). This short wiggle should not repeat.»

I wonder this applies to the case where the launcher is always visible, and it that case it should wiggle only when the urgency is set and not after a minute or so, right?

Revision history for this message
Christopher Townsend (townsend) wrote :

Hi Marco,

> 68 +void Launcher::HandleUrgentIcon(AbstractLauncherIcon::Ptr const&
> icon, struct timespec const& current)
>
> Since the function is called at every redraw, why not just returning at the
> beginning of it in case of: if (options()->hide_mode == LAUNCHER_HIDE_NEVER) ?

Yeah, that's a good point. I may put this in the if statement before calling HandleUrgentIcon().

>
> 187 + bool _urgent_acked;
> Please include _urgent_acked in the initialization list as well. Also, even if
> it seems to work properly even if there's more than on icon, wouldn't probably
> be better to have a set of icons to wiggle instead?

Sure, I can put it in the initialization list. Not sure what you really mean by your question about a set of icons though.

>
>
> 124 + for (it = _model->main_begin(); it != _model->main_end(); ++it)
>
> Using a for-each loop here is nicer...

Yes indeed.

>
> 127 + if (icon->GetQuirk(AbstractLauncherIcon::Quirk::URGENT) &&
>
> Traling space here

OK

>
>
> 153 + sources_.Remove(URGENT_TIMEOUT);
>
> This is not needed since it's automatically done when returning false on this
> callback function (see line 238 of SourceManager::Add on GLibSource.cpp).

Ah, didn't catch that.

>
>
> 157 + SetUrgentTimer(_urgent_wiggle_time);
>
> Mh, why the current timer is not deleted in case you set up a new timer with
> different period?

I thought the current time is automatically deleted when setting up a new timer with the same name.

Revision history for this message
Christopher Townsend (townsend) wrote :

> Ah, on the bug description there's written:
> «- When the Launcher is visible, application alerts should should be indicated
> by a short wiggle (in addition to the existing blue pips). This short wiggle
> should not repeat.»
>
> I wonder this applies to the case where the launcher is always visible, and it
> that case it should wiggle only when the urgency is set and not after a minute
> or so, right?

That is the way I understand this too which is the current behavior.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

Nice...

229 + EXPECT_FALSE(launcher_->_urgent_timer_running);

244 + EXPECT_EQ(launcher_->_urgent_wiggle_time, 0);
245 + EXPECT_EQ(launcher_->_urgent_finished_time.tv_sec, 0);
246 + EXPECT_EQ(launcher_->_urgent_finished_time.tv_nsec, 0);

Probably better to use ASSERT_EQ there^.

Is it now possible to also test that the icons wiggle once the launcher is shown?

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

I think we're fine now, thanks! :)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/Launcher.cpp'
2--- launcher/Launcher.cpp 2013-05-15 04:59:15 +0000
3+++ launcher/Launcher.cpp 2013-06-10 19:12:26 +0000
4@@ -96,9 +96,14 @@
5 const int SCROLL_AREA_HEIGHT = 24;
6 const int SCROLL_FPS = 30;
7
8+const int BASE_URGENT_WIGGLE_PERIOD = 60; // In seconds
9+const int MAX_URGENT_WIGGLE_DELTA = 960; // In seconds
10+const int DOUBLE_TIME = 2;
11+
12 const std::string START_DRAGICON_TIMEOUT = "start-dragicon-timeout";
13 const std::string SCROLL_TIMEOUT = "scroll-timeout";
14 const std::string ANIMATION_IDLE = "animation-idle";
15+const std::string URGENT_TIMEOUT = "urgent-timeout";
16 }
17
18
19@@ -145,6 +150,10 @@
20 , _launcher_drag_delta_min(0)
21 , _enter_y(0)
22 , _last_button_press(0)
23+ , _urgent_wiggle_time(0)
24+ , _urgent_acked(false)
25+ , _urgent_timer_running(false)
26+ , _urgent_ack_needed(false)
27 , _drag_out_delta_x(0.0f)
28 , _drag_gesture_ongoing(false)
29 , _last_reveal_progress(0.0f)
30@@ -194,6 +203,9 @@
31 _times[i].tv_nsec = 0;
32 }
33
34+ _urgent_finished_time.tv_sec = 0;
35+ _urgent_finished_time.tv_nsec = 0;
36+
37 ubus_.RegisterInterest(UBUS_OVERLAY_SHOWN, sigc::mem_fun(this, &Launcher::OnOverlayShown));
38 ubus_.RegisterInterest(UBUS_OVERLAY_HIDDEN, sigc::mem_fun(this, &Launcher::OnOverlayHidden));
39 ubus_.RegisterInterest(UBUS_LAUNCHER_LOCK_HIDE, sigc::mem_fun(this, &Launcher::OnLockHideChanged));
40@@ -1133,6 +1145,11 @@
41 {
42 RenderArg arg;
43 AbstractLauncherIcon::Ptr const& icon = *it;
44+
45+ if (icon->GetQuirk(AbstractLauncherIcon::Quirk::URGENT) && options()->hide_mode == LAUNCHER_HIDE_AUTOHIDE)
46+ {
47+ HandleUrgentIcon(icon, current);
48+ }
49 FillRenderArg(icon, arg, center, parent_abs_geo, folding_threshold, folded_size, folded_spacing,
50 autohide_offset, folded_z_distance, animation_neg_rads, current);
51 arg.colorify = colorify;
52@@ -1566,6 +1583,112 @@
53 }
54 }
55
56+void Launcher::SetUrgentTimer(int urgent_wiggle_period)
57+{
58+ sources_.AddTimeoutSeconds(urgent_wiggle_period, sigc::mem_fun(this, &Launcher::OnUrgentTimeout), URGENT_TIMEOUT);
59+}
60+
61+void Launcher::WiggleUrgentIcon(AbstractLauncherIcon::Ptr const& icon)
62+{
63+ icon->SetQuirk(AbstractLauncherIcon::Quirk::URGENT, false);
64+ icon->SetQuirk(AbstractLauncherIcon::Quirk::URGENT, true);
65+
66+ clock_gettime(CLOCK_MONOTONIC, &_urgent_finished_time);
67+}
68+
69+void Launcher::HandleUrgentIcon(AbstractLauncherIcon::Ptr const& icon, struct timespec const& current)
70+{
71+ struct timespec urgent_time = icon->GetQuirkTime(AbstractLauncherIcon::Quirk::URGENT);
72+ DeltaTime urgent_delta = unity::TimeUtil::TimeDelta(&urgent_time, &_urgent_finished_time);
73+
74+ // If the Launcher is hidden, then add a timer to wiggle the urgent icons at
75+ // certain intervals (1m, 2m, 4m, 8m, 16m, & 32m).
76+ if (_hidden && !_urgent_timer_running && urgent_delta > 0)
77+ {
78+ _urgent_timer_running = true;
79+ _urgent_ack_needed = true;
80+ SetUrgentTimer(BASE_URGENT_WIGGLE_PERIOD);
81+ }
82+ // If the Launcher is hidden, the timer is running, an urgent icon is newer than the last time
83+ // icons were wiggled, and the timer did not just start, then reset the timer since a new
84+ // urgent icon just showed up.
85+ else if (_hidden && _urgent_timer_running && urgent_delta > 0 && _urgent_wiggle_time != 0)
86+ {
87+ _urgent_wiggle_time = 0;
88+ SetUrgentTimer(BASE_URGENT_WIGGLE_PERIOD);
89+ }
90+ // If the Launcher is no longer hidden, then after the Launcher is fully revealed, wiggle the
91+ // urgent icon and then stop the timer (if it's running).
92+ else if (!_hidden && _urgent_ack_needed)
93+ {
94+ if (_last_reveal_progress > 0)
95+ {
96+ _urgent_acked = false;
97+ }
98+ else
99+ {
100+ if (!_urgent_acked && IconUrgentProgress(icon, current) == 1.0f)
101+ {
102+ WiggleUrgentIcon(icon);
103+ }
104+ else if (IconUrgentProgress(icon, current) < 1.0f)
105+ {
106+ if (_urgent_timer_running)
107+ {
108+ sources_.Remove(URGENT_TIMEOUT);
109+ _urgent_timer_running = false;
110+ }
111+ _urgent_acked = true;
112+ _urgent_ack_needed = false;
113+ }
114+ }
115+ }
116+}
117+
118+bool Launcher::OnUrgentTimeout()
119+{
120+ bool foundUrgent = false,
121+ continue_urgent = true;
122+
123+ if (options()->urgent_animation() == URGENT_ANIMATION_WIGGLE && _hidden)
124+ {
125+ // Look for any icons that are still urgent and wiggle them
126+ for (auto icon : *_model)
127+ {
128+ if (icon->GetQuirk(AbstractLauncherIcon::Quirk::URGENT))
129+ {
130+ WiggleUrgentIcon(icon);
131+
132+ foundUrgent = true;
133+ }
134+ }
135+ }
136+ // Update the time for when the next wiggle will occur.
137+ if (_urgent_wiggle_time == 0)
138+ {
139+ _urgent_wiggle_time = BASE_URGENT_WIGGLE_PERIOD;
140+ }
141+ else
142+ {
143+ _urgent_wiggle_time = _urgent_wiggle_time * DOUBLE_TIME;
144+ }
145+
146+ // If no urgent icons were found or we have reached the time threshold,
147+ // then let's stop the timer. Otherwise, start a new timer with the
148+ // updated wiggle time.
149+ if (!foundUrgent || (_urgent_wiggle_time > MAX_URGENT_WIGGLE_DELTA))
150+ {
151+ continue_urgent = false;
152+ _urgent_timer_running = false;
153+ }
154+ else
155+ {
156+ SetUrgentTimer(_urgent_wiggle_time);
157+ }
158+
159+ return continue_urgent;
160+}
161+
162 void Launcher::SetIconSize(int tile_size, int icon_size)
163 {
164 ui::IconRenderer::DestroyShortcutTextures();
165
166=== modified file 'launcher/Launcher.h'
167--- launcher/Launcher.h 2013-03-06 15:44:44 +0000
168+++ launcher/Launcher.h 2013-06-10 19:12:26 +0000
169@@ -223,6 +223,11 @@
170 bool StartIconDragTimeout(int x, int y);
171 bool OnScrollTimeout();
172
173+ void SetUrgentTimer(int urgent_wiggle_period);
174+ void WiggleUrgentIcon(AbstractLauncherIcon::Ptr const& icon);
175+ void HandleUrgentIcon(AbstractLauncherIcon::Ptr const& icon, struct timespec const& current);
176+ bool OnUrgentTimeout();
177+
178 void SetMousePosition(int x, int y);
179 void SetIconUnderMouse(AbstractLauncherIcon::Ptr const& icon);
180
181@@ -380,6 +385,10 @@
182 int _enter_y;
183 int _last_button_press;
184 int _drag_icon_position;
185+ int _urgent_wiggle_time;
186+ bool _urgent_acked;
187+ bool _urgent_timer_running;
188+ bool _urgent_ack_needed;
189 float _drag_out_delta_x;
190 bool _drag_gesture_ongoing;
191 float _last_reveal_progress;
192@@ -396,6 +405,7 @@
193 Atom _selection_atom;
194
195 struct timespec _times[TIME_LAST];
196+ struct timespec _urgent_finished_time;
197
198 BaseTexturePtr launcher_sheen_;
199 BaseTexturePtr launcher_pressure_effect_;
200
201=== modified file 'tests/test_launcher.cpp'
202--- tests/test_launcher.cpp 2013-04-02 18:47:58 +0000
203+++ tests/test_launcher.cpp 2013-06-10 19:12:26 +0000
204@@ -108,6 +108,11 @@
205 using Launcher::IconStartingPulseValue;
206 using Launcher::HandleBarrierEvent;
207 using Launcher::SetHidden;
208+ using Launcher::HandleUrgentIcon;
209+ using Launcher::SetUrgentTimer;
210+ using Launcher::_urgent_timer_running;
211+ using Launcher::_urgent_finished_time;
212+ using Launcher::_urgent_wiggle_time;
213
214 void FakeProcessDndMove(int x, int y, std::list<std::string> uris)
215 {
216@@ -618,6 +623,72 @@
217 launcher_->ProcessDndMove(0,0,{});
218 }
219
220+TEST_F(TestLauncher, UrgentIconWiggleTimerStart)
221+{
222+ MockMockLauncherIcon::Ptr icon(new MockMockLauncherIcon);
223+ timespec current;
224+
225+ launcher_->SetHidden(true);
226+ icon->SetQuirk(AbstractLauncherIcon::Quirk::URGENT, true);
227+
228+ clock_gettime(CLOCK_MONOTONIC, &current);
229+
230+ ASSERT_FALSE(launcher_->_urgent_timer_running);
231+
232+ launcher_->HandleUrgentIcon(icon, current);
233+
234+ EXPECT_TRUE(launcher_->_urgent_timer_running);
235+}
236+
237+TEST_F(TestLauncher, UrgentIconWiggleTimerTimeout)
238+{
239+ MockMockLauncherIcon::Ptr icon(new MockMockLauncherIcon);
240+
241+ model_->AddIcon(icon);
242+ launcher_->SetHidden(true);
243+ icon->SetQuirk(AbstractLauncherIcon::Quirk::URGENT, true);
244+
245+ ASSERT_EQ(launcher_->_urgent_wiggle_time, 0);
246+ ASSERT_EQ(launcher_->_urgent_finished_time.tv_sec, 0);
247+ ASSERT_EQ(launcher_->_urgent_finished_time.tv_nsec, 0);
248+
249+ launcher_->SetUrgentTimer(1);
250+
251+ // Make sure the Urgent Timer expires before checking
252+ Utils::WaitForTimeout(2);
253+
254+ EXPECT_THAT(launcher_->_urgent_wiggle_time, Gt(0));
255+ EXPECT_THAT(launcher_->_urgent_finished_time.tv_sec, Gt(0));
256+ EXPECT_THAT(launcher_->_urgent_finished_time.tv_nsec, Gt(0));
257+}
258+
259+TEST_F(TestLauncher, WiggleUrgentIconAfterLauncherIsRevealed)
260+{
261+ MockMockLauncherIcon::Ptr icon(new MockMockLauncherIcon);
262+ timespec current;
263+
264+ model_->AddIcon(icon);
265+ launcher_->SetHidden(true);
266+ icon->SetQuirk(AbstractLauncherIcon::Quirk::URGENT, true);
267+
268+ // Wait a bit to simulate the icon's initial wiggle
269+ Utils::WaitForTimeout(1);
270+
271+ clock_gettime(CLOCK_MONOTONIC, &current);
272+ launcher_->HandleUrgentIcon(icon, current);
273+
274+ ASSERT_EQ(launcher_->_urgent_finished_time.tv_sec, 0);
275+ ASSERT_EQ(launcher_->_urgent_finished_time.tv_nsec, 0);
276+
277+ launcher_->SetHidden(false);
278+
279+ clock_gettime(CLOCK_MONOTONIC, &current);
280+ launcher_->HandleUrgentIcon(icon, current);
281+
282+ EXPECT_THAT(launcher_->_urgent_finished_time.tv_sec, Gt(0));
283+ EXPECT_THAT(launcher_->_urgent_finished_time.tv_nsec, Gt(0));
284+}
285+
286 }
287 }
288