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

fix(carousel): using setInterval to get rid of dangling $timeouts #1423

Closed
wants to merge 2 commits into from
Closed

fix(carousel): using setInterval to get rid of dangling $timeouts #1423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e90e60b
fix(carousel): using setInterval to get rid of dangling $timeouts
jkurz Dec 17, 2013
5eb8985
fix(carousel): restart the timer when clicked
jkurz Dec 19, 2013
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
39 changes: 25 additions & 14 deletions src/carousel/carousel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
var self = this,
slides = self.slides = [],
currentIndex = -1,
currentTimeout, isPlaying;
currentInterval, isPlaying;
self.currentSlide = null;

/* direction: "prev" or "next" */
self.select = function(nextSlide, direction) {
self.select = function(nextSlide, direction, clicked) {
var nextIndex = slides.indexOf(nextSlide);
//Decide direction if it's not given
if (direction === undefined) {
Expand Down Expand Up @@ -57,8 +57,10 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
}
self.currentSlide = nextSlide;
currentIndex = nextIndex;
//every time you change slides, reset the timer
restartTimer();
//only reset the timer when this method is called from a click
if(clicked === true){
Copy link
Contributor

Choose a reason for hiding this comment

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

@joshkurz Removing this one I think has a side effect on the time between transitions of the slides. What I mean is that if for example the interval is 5 sec and at 3 sec you move to the next slide, then that one will stay only for 2 sec instead of 5. There are no tests for this, but I think this is the expected behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I'll get that back in there then. Good catch.

Thanks
Josh Kurz (mobile)

restartTimer();
}
}
function transitionDone(next, current) {
angular.extend(next, {direction: '', active: true, leaving: false, entering: false});
Expand All @@ -72,21 +74,21 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
return slides.indexOf(slide);
};

$scope.next = function() {
$scope.next = function(clicked) {
var newIndex = (currentIndex + 1) % slides.length;

//Prevent this user-triggered transition from occurring if there is already one in progress
if (!$scope.$currentTransition) {
return self.select(slides[newIndex], 'next');
return self.select(slides[newIndex], 'next', clicked);
}
};

$scope.prev = function() {
$scope.prev = function(clicked) {
var newIndex = currentIndex - 1 < 0 ? slides.length - 1 : currentIndex - 1;

//Prevent this user-triggered transition from occurring if there is already one in progress
if (!$scope.$currentTransition) {
return self.select(slides[newIndex], 'prev');
return self.select(slides[newIndex], 'prev', clicked);
}
};

Expand All @@ -103,34 +105,37 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
};

$scope.$watch('interval', restartTimer);

function restartTimer() {
if (currentTimeout) {
$timeout.cancel(currentTimeout);
if (currentInterval) {
clearInterval(currentInterval);
}
function go() {
if (isPlaying) {
$scope.next();
restartTimer();
} else {
$scope.pause();
}
$timeout(angular.noop);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be $scope.$apply().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

causes digest errors. This is the safe way to run a digest without checking for the $$phase on the scope.

}
var interval = +$scope.interval;
if (!isNaN(interval) && interval>=0) {
currentTimeout = $timeout(go, interval);
currentInterval = setInterval(go, interval);
}
}

$scope.play = function() {
if (!isPlaying) {
isPlaying = true;
restartTimer();
}
};

$scope.pause = function() {
if (!$scope.noPause) {
isPlaying = false;
if (currentTimeout) {
$timeout.cancel(currentTimeout);
if (currentInterval) {
clearInterval(currentInterval);
}
}
};
Expand Down Expand Up @@ -163,6 +168,12 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
currentIndex--;
}
};

$scope.$on('$destroy', function(){
clearInterval(currentInterval);
});


}])

/**
Expand Down
57 changes: 43 additions & 14 deletions src/carousel/test/carousel.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ describe('carousel', function() {
$compile = _$compile_;
$controller = _$controller_;
$timeout = _$timeout_;
jasmine.Clock.useMock();
}));

describe('basics', function() {
Expand Down Expand Up @@ -130,12 +131,14 @@ describe('carousel', function() {
//no timeout to flush, interval watch doesn't make a new one when interval is invalid
testSlideActive(0);
scope.$apply('interval = 1000');
$timeout.flush();
jasmine.Clock.tick(1000);
scope.$apply();
testSlideActive(1);
scope.$apply('interval = false');
testSlideActive(1);
scope.$apply('interval = 1000');
$timeout.flush();
jasmine.Clock.tick(1000);
scope.$apply();
testSlideActive(2);
});

Expand All @@ -160,34 +163,42 @@ describe('carousel', function() {

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

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

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

Expand All @@ -197,7 +208,8 @@ describe('carousel', function() {
scope.$apply('slides.splice(0,1)');
expect(elm.find('div.item').length).toBe(2);
testSlideActive(1);
$timeout.flush();
jasmine.Clock.tick(5000);
scope.$apply();
testSlideActive(0);
scope.$apply('slides.splice(1,1)');
expect(elm.find('div.item').length).toBe(1);
Expand Down Expand Up @@ -237,12 +249,12 @@ describe('carousel', function() {
});

describe('controller', function() {
var scope, ctrl;
//create an array of slides and add to the scope
var slides = [{'content': 1},{'content': 2},{'content':3},{'content':4}];
var scope, ctrl, slides;

beforeEach(function() {
slides = [{'content': 1},{'content': 2},{'content':3},{'content':4}];
scope = $rootScope.$new();
scope.interval = 5000;
ctrl = $controller('CarouselController', {$scope: scope, $element: null});
for(var i = 0;i < slides.length;i++){
ctrl.addSlide(slides[i]);
Expand Down Expand Up @@ -295,5 +307,22 @@ describe('carousel', function() {
expect(ctrl.currentSlide).toBe(ctrl.slides[0]);
});
});

describe('next', function() {

it('should remove slide and change active slide if needed', function() {
spyOn(scope, 'next');
expect(scope.next.callCount).toBe(0);
expect(ctrl.currentSlide).toBe(slides[0]);
jasmine.Clock.tick(15000);
scope.$apply();
expect(scope.next.callCount).toBe(3);
scope.$destroy();
jasmine.Clock.tick(15000);
scope.$apply();
expect(scope.next.callCount).toBe(3);
});

});
});
});
4 changes: 2 additions & 2 deletions template/carousel/carousel.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@
<li ng-repeat="slide in slides()" ng-class="{active: isActive(slide)}" ng-click="select(slide)"></li>
</ol>
<div class="carousel-inner" ng-transclude></div>
<a ng-click="prev()" class="carousel-control left" ng-show="slides().length > 1">&lsaquo;</a>
<a ng-click="next()" class="carousel-control right" ng-show="slides().length > 1">&rsaquo;</a>
<a ng-click="prev(true)" class="carousel-control left" ng-show="slides().length > 1">&lsaquo;</a>
<a ng-click="next(true)" class="carousel-control right" ng-show="slides().length > 1">&rsaquo;</a>
</div>