-
Notifications
You must be signed in to change notification settings - Fork 20.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Effects: sync .stop/.finish with .tick to maintain proper queue (#3497) #3501
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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