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

PASSED: Continuous integration, rev:3322
http://jenkins.qa.ubuntu.com/job/unity-ci/55/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-raring-amd64-ci/55
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-raring-armhf-ci/55
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-raring-i386-ci/55

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-ci/55/rebuild

Looks nice :)

Yay

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.

PASSED: Continuous integration, rev:3325
http://jenkins.qa.ubuntu.com/job/unity-ci/111/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-raring-amd64-ci/115
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-raring-armhf-ci/111
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-raring-i386-ci/111

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-ci/111/rebuild

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?

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?

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.

> 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.

PASSED: Continuous integration, rev:3328
http://jenkins.qa.ubuntu.com/job/unity-ci/122/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-saucy-amd64-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-saucy-armhf-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-saucy-i386-ci/10

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-ci/122/rebuild

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?

PASSED: Continuous integration, rev:3329
http://jenkins.qa.ubuntu.com/job/unity-ci/126/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-saucy-amd64-ci/14
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-saucy-armhf-ci/14
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-saucy-i386-ci/14

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-ci/126/rebuild

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