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

fix(carousel): cancel timer on scope destroyed #1451

Closed
wants to merge 3 commits into from

Conversation

chrisirhc
Copy link
Contributor

My take on this fix based on @joshkurz 's findings.

At first I thought that the go function should check if the scope is destroyed, but I realized that it doesn't have to as long as only one timeout can be set at one time. If only one timeout can be set at one time, the scope $destroyed event just has to cancel it.

I'm also thinking of moving the go function out of the restartTimer as the function is always the same doesn't need to be recreated on each timer run.

TODO:

  • Move go function out of restartTimer

Fixes #1414

@@ -106,6 +106,7 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
function restartTimer() {
if (currentTimeout) {
$timeout.cancel(currentTimeout);
currentTimeout = null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@chrisirhc You should also extract this code:

if (currentTimeout) {
      $timeout.cancel(currentTimeout);
      currentTimeout = null;
}

into its own function as it is called multiple times.

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 do that when I wake heh. Just about to sleep.
Update: Just did it.

Move go function out of restartTimer as it doesn't need to be re-created
during each interval.
@pkozlowski-opensource
Copy link
Member

Simply awesome. It is in master already!

@joshkurz
Copy link
Contributor

so it seems that the issue is still not fixed. from this punker. http://plnkr.co/edit/kMrUtWy3fQqgHwNdHSLO?p=preview. The issue persists with the master code. Im not 100% but I believe if you ran the $destroy test I wrote in #1423, it would fail as the $timeouts are still created after the scope is destroyed here.

@chrisirhc
Copy link
Contributor Author

@joshkurz , you're right. This is caused by

if ($scope.$currentTransition) {
$scope.$currentTransition.cancel();
//Timeout so ng-class in template has time to fix classes for finished slide
$timeout(goNext);
} else {
and I'll fix that in a follow-up PR.

chrisirhc added a commit to chrisirhc/angular-ui-bootstrap that referenced this pull request Dec 23, 2013
pkozlowski-opensource pushed a commit that referenced this pull request Dec 24, 2013
@joshkurz
Copy link
Contributor

nice job @chrisirhc

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Carousel causes a memory leak.
4 participants