Skip to content

Commit

Permalink
fix(carousel): replaced $timeout with $interval when it was wrong
Browse files Browse the repository at this point in the history
I had a to tweak a bit two tests since $interval never throws an exception after flushing, hope I did it OK.

Ammend: addresses @chrisirhc critics

Fixes angular-ui#1308
Fixes angular-ui#2454
Closes angular-ui#2776
  • Loading branch information
Gahen authored and Oron Nadiv committed Nov 18, 2014
1 parent 312a3be commit aa9fdee
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 30 deletions.
18 changes: 9 additions & 9 deletions src/carousel/carousel.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
*
*/
angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
.controller('CarouselController', ['$scope', '$timeout', '$transition', function ($scope, $timeout, $transition) {
.controller('CarouselController', ['$scope', '$timeout', '$interval', '$transition', function ($scope, $timeout, $interval, $transition) {
var self = this,
slides = self.slides = $scope.slides = [],
currentIndex = -1,
currentTimeout, isPlaying;
currentInterval, isPlaying;
self.currentSlide = null;

var destroyed = false;
Expand Down Expand Up @@ -106,22 +106,22 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
function restartTimer() {
resetTimer();
var interval = +$scope.interval;
if (!isNaN(interval) && interval>=0) {
currentTimeout = $timeout(timerFn, interval);
if (!isNaN(interval) && interval > 0) {
currentInterval = $interval(timerFn, interval);
}
}

function resetTimer() {
if (currentTimeout) {
$timeout.cancel(currentTimeout);
currentTimeout = null;
if (currentInterval) {
$interval.cancel(currentInterval);
currentInterval = null;
}
}

function timerFn() {
if (isPlaying) {
var interval = +$scope.interval;
if (isPlaying && !isNaN(interval) && interval > 0) {
$scope.next();
restartTimer();
} else {
$scope.pause();
}
Expand Down
46 changes: 25 additions & 21 deletions src/carousel/test/carousel.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ describe('carousel', function() {
}));
beforeEach(module('template/carousel/carousel.html', 'template/carousel/slide.html'));

var $rootScope, $compile, $controller, $timeout;
beforeEach(inject(function(_$rootScope_, _$compile_, _$controller_, _$timeout_) {
var $rootScope, $compile, $controller, $interval;
beforeEach(inject(function(_$rootScope_, _$compile_, _$controller_, _$interval_) {
$rootScope = _$rootScope_;
$compile = _$compile_;
$controller = _$controller_;
$timeout = _$timeout_;
$interval = _$interval_;
}));

describe('basics', function() {
Expand Down Expand Up @@ -148,16 +148,18 @@ describe('carousel', function() {

it('shouldnt go forward if interval is NaN or negative', function() {
testSlideActive(0);
var previousInterval = scope.interval;
scope.$apply('interval = -1');
//no timeout to flush, interval watch doesn't make a new one when interval is invalid
$interval.flush(previousInterval);
testSlideActive(0);
scope.$apply('interval = 1000');
$timeout.flush();
$interval.flush(1000);
testSlideActive(1);
scope.$apply('interval = false');
$interval.flush(1000);
testSlideActive(1);
scope.$apply('interval = 1000');
$timeout.flush();
$interval.flush(1000);
testSlideActive(2);
});

Expand All @@ -182,34 +184,35 @@ describe('carousel', function() {

it('should be playing by default and cycle through slides', function() {
testSlideActive(0);
$timeout.flush();
$interval.flush(scope.interval);
testSlideActive(1);
$timeout.flush();
$interval.flush(scope.interval);
testSlideActive(2);
$timeout.flush();
$interval.flush(scope.interval);
testSlideActive(0);
});

it('should pause and play on mouseover', function() {
testSlideActive(0);
$timeout.flush();
$interval.flush(scope.interval);
testSlideActive(1);
elm.trigger('mouseenter');
expect($timeout.flush).toThrow();//pause should cancel current timeout
testSlideActive(1);
$interval.flush(scope.interval);
testSlideActive(1);
elm.trigger('mouseleave');
$timeout.flush();
$interval.flush(scope.interval);
testSlideActive(2);
});

it('should not pause on mouseover if noPause', function() {
scope.$apply('nopause = true');
testSlideActive(0);
elm.trigger('mouseenter');
$timeout.flush();
$interval.flush(scope.interval);
testSlideActive(1);
elm.trigger('mouseleave');
$timeout.flush();
$interval.flush(scope.interval);
testSlideActive(2);
});

Expand All @@ -219,7 +222,7 @@ describe('carousel', function() {
scope.$apply('slides.splice(0,1)');
expect(elm.find('div.item').length).toBe(2);
testSlideActive(1);
$timeout.flush();
$interval.flush(scope.interval);
testSlideActive(0);
scope.$apply('slides.splice(1,1)');
expect(elm.find('div.item').length).toBe(1);
Expand Down Expand Up @@ -253,14 +256,15 @@ describe('carousel', function() {

it('issue 1414 - should not continue running timers after scope is destroyed', function() {
testSlideActive(0);
$timeout.flush();
$interval.flush(scope.interval);
testSlideActive(1);
$timeout.flush();
$interval.flush(scope.interval);
testSlideActive(2);
$timeout.flush();
$interval.flush(scope.interval);
testSlideActive(0);
spyOn($interval, 'cancel').andCallThrough();
scope.$destroy();
expect($timeout.flush).toThrow('No deferred tasks to be flushed');
expect($interval.cancel).toHaveBeenCalled();
});

});
Expand Down Expand Up @@ -326,12 +330,12 @@ describe('carousel', function() {
scope.interval = 2000;
scope.$digest();

$timeout.flush();
$interval.flush(scope.interval);
expect(scope.next.calls.length).toBe(1);

scope.$destroy();

$timeout.flush(scope.interval);
$interval.flush(scope.interval);
expect(scope.next.calls.length).toBe(1);
});
});
Expand Down

0 comments on commit aa9fdee

Please sign in to comment.