Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(carousel): replaced $timeout with $interval when it was wrong #2776

Closed
wants to merge 1 commit into from
Closed

fix(carousel): replaced $timeout with $interval when it was wrong #2776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions src/carousel/carousel.js
View file
Open in desktop
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
View file
Open in desktop
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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test now needs $interval.flush(5000); here to actually test these values.
I found that this actually causes the test to fail on $interval function because it's calling $interval with false as the timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be $interval.flush(1000)?

Looking at it more closely I don't get why here we use $apply here to set scope.interval value and it's directly modified on the last test as scope.interval = 2000;. Any clue?

$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