feat(carousel): use $interval instead of $timeout

[mvhecke]

Use the $interval service instead of the $timeout service for the carousel.

[mvhecke]

I know the tests fail and that's the only thing a need some help with, because I have no idea how to get the active $interval and pass it on to the $interval.cancel() function.

[joshkurz]

I seriously hate my phone. Anytime I start a draft on it, it will
automatically send even if I just back out and don't send it.

I was going to say... you need to use interval.flush, although i'm not too
sure how the team how the bootstrap team feels about using interval
anymore.

I believe its the right way to go for the carousel.

On Tue, Jan 21, 2014 at 8:39 AM, jkurz25@gmail.com jkurz25@gmail.comwrote:

Thanks
Josh Kurz (mobile)

----- Reply message -----
From: "Marcellino van Hecke" notifications@github.com
To: "angular-ui/bootstrap" bootstrap@noreply.github.com
Subject: [bootstrap] feat(carousel): use $interval instead of $timeout
(#1630)
Date: Tue, Jan 21, 2014 08:30

I know the tests fail and that's the only thing a need some help with,
because I have no idea how to get the active $interval and pass it on to
the $interval.cancel() function.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1630#issuecomment-32884757
.

Josh Kurz
www.atlantacarlocksmith.com http://www.atlantacarlocksmith.com

[mvhecke]

The thing is that the behavior of $interval.flush differs from what $timeout.flush does. Interval runs the interval tasks scheduled to be run in the next milliseconds and timeout flushes the queue of pending tasks.

PS: @joshkurz Phones can be a pain in the ass sometimes

[joshkurz]

well there is a clock tick function that jasmine offers you can use.

On Tue, Jan 21, 2014 at 10:11 AM, Marcellino van Hecke <
notifications@github.com> wrote:

The thing is that the behavior of $interval.flush differs from what
$timeout.flush does. Interval runs the interval tasks scheduled to be run
in the next milliseconds and timeout flushes the queue of pending tasks.

PS: @joshkurz https://github.com/joshkurz Phones can be a pain in the
ass sometimes

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1630#issuecomment-32893389
.

Josh Kurz
www.atlantacarlocksmith.com http://www.atlantacarlocksmith.com

[mvhecke]

@joshkurz Thanks for the quick reply, but I'm not familiar with Jasmine. @bekos How would I be able to access this? Is creating a scope variable bad practice and does this impact the performance?

[bekos]

@Gamemaniak Check @joshkurz's older PR, and how he implemented the tests #1423, but I am not sure about the $interval here. You can read some of the comments on that PR also.

[Foxandxss]

Angular's $interval tests doesn't help? (from ignorance): https://github.com/angular/angular.js/blob/master/test/ng/intervalSpec.js

[mvhecke]

@bekos I tried the same method as @joshkurz, but all tests fail when I use that method. @Foxandxss It only confirms what I already knew.

Let me try and explain where it goes wrong (English isn't my native language). At the moment my promise, currentInterval, that I need for the $interval.cancel is not accessible because it's a local variable in the CarouselController. The method $interval.flush always works because it's a service so that works just fine in the tests.

[pkozlowski-opensource]

@Gamemaniak what is the added value of this change?

[mvhecke]

@pkozlowski-opensource Primarily for performance reasons because a carousel doesn't need that exact timing. It's setTimeout vs setInterval, interesting (article)[http://ejohn.org/blog/how-javascript-timers-work/] about it from John Regig.

[pkozlowski-opensource]

@Gamemaniak I didn't know that we've got performance issues with carousel :-/
Isn't it a case of micro-optimisation? I mean, why not, but I'm not sure it is critical...

[mvhecke]

@pkozlowski-opensource Not really an issue and yes it's more a micro micro-optimization as you call it. Bootstrap is also used on mobile devices so a high performance is a good thing if it's possible in my opinion.

[joshkurz]

agreed and it would save some LOC.

On Tue, Jan 21, 2014 at 1:40 PM, Marcellino van Hecke <
notifications@github.com> wrote:

@pkozlowski-opensource https://github.com/pkozlowski-opensource Not
really an issue and yes it's more a micro micro-optimization as you call
it. Bootstrap is also used on mobile devices so a high performance is a
good thing if it's possible in my opinion.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1630#issuecomment-32915064
.

Josh Kurz
www.atlantacarlocksmith.com http://www.atlantacarlocksmith.com

[joshkurz]

it just seems like using timeouts to perform interval like functionality is
a hack. The original bootstrap project uses setInterval for the carousel.
Not saying we are trying to mimick them, but I think their carousel works
pretty darn well.

On Tue, Jan 21, 2014 at 1:43 PM, Josh Kurz jkurz25@gmail.com wrote:

agreed and it would save some LOC.

On Tue, Jan 21, 2014 at 1:40 PM, Marcellino van Hecke <
notifications@github.com> wrote:

@pkozlowski-opensource https://github.com/pkozlowski-opensource Not
really an issue and yes it's more a micro micro-optimization as you call
it. Bootstrap is also used on mobile devices so a high performance is a
good thing if it's possible in my opinion.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1630#issuecomment-32915064
.

Josh Kurz
www.atlantacarlocksmith.com http://www.atlantacarlocksmith.com

Josh Kurz
www.atlantacarlocksmith.com http://www.atlantacarlocksmith.com

[Foxandxss]

I agree, if this doesn't break anything, it is always a best way to do it.

[mvhecke]

Any ideas on how I could expose the interval to testing properly? Everything functions as expected and only testing is still an issue.

[rgaskill]

This change would also make it possible to use protractor for testing pages with a carrousel on it. (see #1308). Currently you are unable to use protractor to test any page with a AngularUI carrousel on it.

[juliemr]

Hi folks,

Following up on a kind of stale issue here. Protractor end to end tests will wait before every action until Angular says it's synchronized. This means that all $timeouts are done. If someone is using a carousel, this will never happen, since it uses $timeout indefinitely instead of $interval. Any chance you could take another look at this change?

[pkozlowski-opensource]

@juliemr oh, I see! Yes, in this case we've got a valid reason to switch to $interval. Opened the #2454 for this. Thnx!

[Gahen]

Hey guys,

I made a PR (#2776) about this ticket (I posted it on #2454). It was a simple fix and it's passing all tests, please tell me if something is missing to approve it.

Thanks!