From e90e60b766813aa5bcf5e5186a96bef1c11be668 Mon Sep 17 00:00:00 2001 From: Josh Kurz Date: Tue, 17 Dec 2013 00:58:03 -0500 Subject: [PATCH 1/2] fix(carousel): using setInterval to get rid of dangling $timeouts Signed-off-by: Josh Kurz --- src/carousel/carousel.js | 25 ++++++++----- src/carousel/test/carousel.spec.js | 57 ++++++++++++++++++++++-------- 2 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/carousel/carousel.js b/src/carousel/carousel.js index cb7d585173..b273c5a9ff 100644 --- a/src/carousel/carousel.js +++ b/src/carousel/carousel.js @@ -11,7 +11,7 @@ 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" */ @@ -57,8 +57,6 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition']) } self.currentSlide = nextSlide; currentIndex = nextIndex; - //every time you change slides, reset the timer - restartTimer(); } function transitionDone(next, current) { angular.extend(next, {direction: '', active: true, leaving: false, entering: false}); @@ -103,34 +101,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); } 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); } } }; @@ -163,6 +164,12 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition']) currentIndex--; } }; + + $scope.$on('$destroy', function(){ + clearInterval(currentInterval); + }); + + }]) /** diff --git a/src/carousel/test/carousel.spec.js b/src/carousel/test/carousel.spec.js index 0bea1d3daa..ecc510bee8 100644 --- a/src/carousel/test/carousel.spec.js +++ b/src/carousel/test/carousel.spec.js @@ -8,6 +8,7 @@ describe('carousel', function() { $compile = _$compile_; $controller = _$controller_; $timeout = _$timeout_; + jasmine.Clock.useMock(); })); describe('basics', function() { @@ -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); }); @@ -160,23 +163,29 @@ 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); }); @@ -184,10 +193,12 @@ describe('carousel', 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); }); @@ -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); @@ -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]); @@ -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); + }); + + }); }); }); From 5eb8985c194160f7dc39ca11e9f98ed8222a6dc3 Mon Sep 17 00:00:00 2001 From: Josh Kurz Date: Wed, 18 Dec 2013 23:43:09 -0500 Subject: [PATCH 2/2] fix(carousel): restart the timer when clicked Signed-off-by: Josh Kurz --- src/carousel/carousel.js | 14 +++++++++----- template/carousel/carousel.html | 4 ++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/carousel/carousel.js b/src/carousel/carousel.js index b273c5a9ff..567250da5b 100644 --- a/src/carousel/carousel.js +++ b/src/carousel/carousel.js @@ -15,7 +15,7 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition']) 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) { @@ -57,6 +57,10 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition']) } self.currentSlide = nextSlide; currentIndex = nextIndex; + //only reset the timer when this method is called from a click + if(clicked === true){ + restartTimer(); + } } function transitionDone(next, current) { angular.extend(next, {direction: '', active: true, leaving: false, entering: false}); @@ -70,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); } }; diff --git a/template/carousel/carousel.html b/template/carousel/carousel.html index 97fad9f3f1..64e5296c60 100644 --- a/template/carousel/carousel.html +++ b/template/carousel/carousel.html @@ -3,6 +3,6 @@
  • - ‹ - › + ‹ + ›