Effects: sync .stop/.finish with .tick to maintain proper queue (#3497)#3501
Effects: sync .stop/.finish with .tick to maintain proper queue (#3497)#3501preethi26 wants to merge 2 commits into
Conversation
|
@preethi26, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jeresig, @gnarf and @timmywil to be potential reviewers. |
…Handled repeated execution of animation in first tick. Fixes jquery#3497. corrected syntax error Fixes jquery#3497 Fixed. jquery#3497
d1bd21f to
ae094b1
Compare
|
|
||
| for ( ; i < timers.length; i++ ) { | ||
| timer = timers[ i ]; | ||
| for ( var j = 0; j < lis.length; j++ ) { |
There was a problem hiding this comment.
I'm hoping to avoid O(N²) operations and extra global variables, and was thinking we'd do this by checking/clearing timer.elem since that's already cleared in an always callback.
There was a problem hiding this comment.
ok ! not a problem.
I would like to know if the second bug about extra processing was handled?
As I wasn't sure about it I did not mark it in the checklist.
There was a problem hiding this comment.
Probably not, but we'll see. This PR will also require unit tests that don't pass without the updated code.
There was a problem hiding this comment.
hey @gibson042 ,
jQuery.timers = [];
jQuery.fx.tick = function() {
var timer,
i = 0,
timers = jQuery.timers;
fxNow = jQuery.now();
for ( ; i < timers.length; i++ ) {
timer = timers[ i ];
var x = timers.indexOf(timer)
if ( !timer() ) {
if( x > timers.indexOf(timer)) {
i = i - ( x - timers.indexOf(timer) )
}
if( timers[ i ] === timer ) {
timers.splice( i--, 1 );
}
}
}
if ( !timers.length ) {
jQuery.fx.stop();
}
fxNow = undefined;
};
This is the edited [ jQuery.fx.tick()] https://github.com/jquery/jquery/blob/master/src/effects.js#L644) function.
I would like to know if this fixes the first part of the bug before proceeding further.
Thank you,
Preethi.
There was a problem hiding this comment.
@gibson042. Sorry, just realized that it wouldn't work. Coz we need to check the value of "timer" 's index in between consecutive iterations ( which is not done here ).
There was a problem hiding this comment.
In every iteration, making the value of the iterator equal to the value of the previous timer that successfully ran + 1.
|
@preethi26 friendly ping |
|
This is really not what I'm looking for; see my comment about O(N²) operations and extra global variables. And note that a functional change will absolutely not be accepted without unit tests. |
|
@gibson042 , |
|
No problem. As I mentioned in the comment, I think we can mostly cover this by checking and deleting the |
|
Closing & re-opening the PR to trigger the EasyCLA check... |
|
#3497
Summary
1.Whenever the .finish or the .stop functions modify the timers queue, we maintain all those indexes that are removed in a list ( i.e, lis[ ] ). Whenever the tick function iterates over the queue, we update the iterator based on the value of the indexes removed ( only when the indexes less than the current iterator value are removed there will be a skipping over the animations ).
All the new animations that are added to the timers queue in the middle of a particular iteration of the tick function are processed in the next iteration.
Checklist