-
Notifications
You must be signed in to change notification settings - Fork 6.7k
fix(carousel): cancel timer on scope destroyed #1451
Conversation
@@ -106,6 +106,7 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition']) | |||
function restartTimer() { | |||
if (currentTimeout) { | |||
$timeout.cancel(currentTimeout); | |||
currentTimeout = null; | |||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Simply awesome. It is in master already! |
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. |
@joshkurz , you're right. This is caused by bootstrap/src/carousel/carousel.js Lines 25 to 29 in 0b39994
|
Discovered after angular-ui#1451
Discovered after #1451
nice job @chrisirhc |
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 therestartTimer
as the function is always the same doesn't need to be recreated on each timer run.TODO:
restartTimer
Fixes #1414