Skip to content
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

Added throttling function #1996

Merged
merged 10 commits into from
Jun 1, 2018
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ New Features:

* [#1761](https://github.com/ckeditor/ckeditor-dev/issues/1761): [Autolink](https://ckeditor.com/cke4/addon/autolink) plugin supports email links.
* [#1703](https://github.com/ckeditor/ckeditor-dev/issues/1703): Introduced Mentions plugin providing autocompletion feature for usernames.
* [#1993](https://github.com/ckeditor/ckeditor-dev/issues/1993): Added [`CKEDITOR.tools.throttle`](http://docs.ckeditor.com/ckeditor4/docs/#!/api/CKEDITOR_tools.html#method-throttle) function.
Copy link
Contributor

Choose a reason for hiding this comment

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

It should land in the API section.


Fixed Issues:

Expand Down
84 changes: 84 additions & 0 deletions core/tools.js
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,90 @@
}, milliseconds || 0 );
},

/**
* Throttles `input` events (or any `input` calls)
Copy link
Contributor

Choose a reason for hiding this comment

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

It has nothing to do with events, we're talking about input being a function here.

* and triggers `output` not more often than once per `minInterval`.
*
* After each `input` scheduled `output` will be destroyed and replaced with
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is clear, but it could be only me.

If multiple calls occur within as single minInterval time, the closest (soonest?) interval call will use the last passed arguments and discard arguments that were given in preceeding calls.

Aaaand looking at above ☝️ it doesn't look any better 😄 I think we'll need to ask other devs to help us out here. But it's crucial this bit is understandable, as it's a very important information.

Copy link
Member Author

@jacekbogdanski jacekbogdanski May 30, 2018

Choose a reason for hiding this comment

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

TBO I'm not sure if we need information about discarding arguments in preceding calls since preceding call (there is only one previously scheduled call) is discarded. However, I think that the sentence:

If multiple calls occur within a single minInterval time, the most recent input call with its arguments will be used to schedule next output call, and the previous output call will be discarded.

best corresponds to how this function works. Although I'm not sure if it's enough understandable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it works well 👍

* new one. It gives you the opportunity to modify passed parameters into `input` function and get
* different results.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it make sense to put a note that the first call is always guaranteed to be executed synchronously.

* var buffer = CKEDITOR.tools.throttle( 200, function( message ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Listings should be wrapped with triple backticks + language hit 🙀

* console.log( message );
* } );
*
* buffer.input( 'foo!' );
* // 'foo!' logged immediately.
* buffer.input( 'buz!' );
Copy link
Contributor

Choose a reason for hiding this comment

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

The third word in foo bar used most commonly is baz so let's stick to foo bar baz. 🙂 And you should reorder it here.

* // Nothing logged.
* buffer.input( 'bar!' );
* // Nothing logged.
* // ... after 200ms a single 'bar!' will be logged.
Copy link
Contributor

Choose a reason for hiding this comment

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

Change ... (thre dots) into a legit … (ellipsis) character 🙂

*
* Can be easily used with events:
*
* var buffer = CKEDITOR.tools.throttle( 200, function( evt ) {
* console.log( evt.data.text );
* } );
*
* editor.on( 'key', buffer.input );
* // Note: There is no need to bind buffer as a context.
*
* @since 4.10.0
* @param {Number} minInterval Minimum interval between `output` calls in milliseconds.
* @param {Function} output Function that will be executed as `output`.
* @param {Object} [scopeObj] The object used to scope the listener call (the `this` object).
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically it's not a scope… it's a context. There is a difference between the two (please have a read on it and don't repeat the mistakes that the Ancient Ones did 🙂). So context or contextObj and a proper docs updates.

* @returns {Object}
* @returns {Function} return.input Buffer's input method.
* Accepts parameters which will be directly passed into `output` function.
* @returns {Function} return.reset Resets buffered events — `output` will not be executed
Copy link
Contributor

Choose a reason for hiding this comment

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

Buffered events calls.

* until next `input` is triggered.
*/
throttle: function( minInterval, output, scopeObj ) {
var scheduled,
lastOutput = 0;

scopeObj = scopeObj || {};

return {
input: input,
reset: reset
};

function input() {
var args = Array.prototype.slice.call( arguments );

if ( scheduled ) {
clearTimeout( scheduled );
scheduled = 0;
}

var diff = ( new Date() ).getTime() - lastOutput;

// If less than minInterval passed after last check,
// schedule next for minInterval after previous one.
if ( diff < minInterval ) {
scheduled = setTimeout( triggerOutput, minInterval - diff );
} else {
triggerOutput();
}

function triggerOutput() {
lastOutput = ( new Date() ).getTime();
scheduled = false;

output.apply( scopeObj, args );
}
}

function reset() {
if ( scheduled ) {
clearTimeout( scheduled );
scheduled = lastOutput = 0;
}
}
},

/**
* Removes spaces from the start and the end of a string. The following
* characters are removed: space, tab, line break, line feed.
Expand Down
93 changes: 93 additions & 0 deletions tests/core/tools.js
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,99 @@
assert.areSame( 'ABcDeF', c( 'aBcDeF', true ) );
},

'test throttle': function() {
var output = 0,
foo = 'foo',
buz = 'buz',
message,
buffer = CKEDITOR.tools.throttle( 200, function( arg ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Once again, use sinon.stub / spy for function mocking, it will make the API much, much clearer. 🙂

If you'd use sinon, you had assertion like that:

sinon.assert.calledOnce( inputSpy );
sinon.assert.calledWithExactly( inputSpy, 'foo' );

And then:

assert.areSame( 2, inputSpy.callCount );
sinon.assert.calledWithExactly( foo.getCall( 1 ), 'buz' )

Just to make few examples. It's a really convenient API.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem with sinon.assert is that it doesn't allow for meaningful assert messages. because of that issue, I will use assert.isTrue( inputSpy.calledOnce, msg ) assert.

output++;
message = arg;
} );

assert.areSame( 0, output );

buffer.input( foo );

assert.areSame( 1, output );
assert.areSame( foo, message );
Copy link
Contributor

Choose a reason for hiding this comment

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

Good luck with figuring out as any of the assertion fails. In other words, please, put meaningful assert messages when possible and applicable.

For sure this test case is one of them.


buffer.input( buz );
buffer.input( buz );
buffer.input( buz );

assert.areSame( 1, output );
assert.areSame( foo, message );

wait( function() {
assert.areSame( 1, output );
assert.areSame( foo, message );

wait( function() {
assert.areSame( 2, output );
assert.areSame( buz, message );

buffer.input( foo );

assert.areSame( 2, output );
assert.areSame( buz, message );
Copy link
Contributor

Choose a reason for hiding this comment

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

No reason to check the message here, as it couldn't be changed if the call count is the same.


wait( function() {
assert.areSame( 3, output );
assert.areSame( foo, message );

// Check that input triggered after 70ms from previous
// buffer.input will trigger output after next 140ms (200-70).
wait( function() {
buffer.input( buz );

assert.areSame( 3, output );
assert.areSame( foo, message );

wait( function() {
assert.areSame( 4, output );
assert.areSame( buz, message );
}, 140 );
}, 70 );
}, 210 );
}, 110 );
}, 100 );
},

'test throttle.reset': function() {
var output = 0,
buffer = CKEDITOR.tools.throttle( 100, function() {
output++;
} );

assert.areSame( 0, output );

buffer.input();

assert.areSame( 1, output );

buffer.input();
buffer.reset();

wait( function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you could even call it synchronously after reset and check the counter.

assert.areSame( 1, output );

buffer.input();

assert.areSame( 2, output );
}, 110 );
},

'test throttle contex': function() {
var spy = sinon.spy(),
ctxObj = {},
buffer = CKEDITOR.tools.throttle( 100, spy, ctxObj );

buffer.input();

assert.areSame( ctxObj, spy.getCall( 0 ).thisValue, 'callback was executed with the right context' );
},

'test checkIfAnyObjectPropertyMatches': function() {
var c = CKEDITOR.tools.checkIfAnyObjectPropertyMatches,
r1 = /foo/,
Expand Down
21 changes: 21 additions & 0 deletions tests/core/tools/manual/throttle.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<style>
.red {
background-color: red;
}
.blue {
background-color: blue;
}
</style>

<button id="btn" class="red" >Click me!</button>

<script>
var button = document.getElementById( 'btn' ),
buffer = CKEDITOR.tools.throttle( 2000, function ( css ) {
button.className = css === 'red' ? 'blue' : 'red';
} );

button.addEventListener( 'click', function() {
buffer.input( button.className );
} );
</script>
15 changes: 15 additions & 0 deletions tests/core/tools/manual/throttle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
@bender-tags: feature, 4.10.0, 1993
@bender-ui: collapsed
@bender-ckeditor-plugins: toolbar, wysiwygarea

1. Click button `Click me` five times as fast as possible.

## Expected

Button changes its color:
1. After first click to blue.
1. After last click to red.

## Unexpected

Button changes its color in invalid order.