Added throttling function #1996

jacekbogdanski · 2018-05-22T10:00:03Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

Added throttling function based on CKEDITOR.tools.eventsBuffer.

Closes #1993

mlewand · 2018-05-25T10:23:33Z

Seems that this branch got somehow broken, there's a little too much commits in there 🙂

mlewand

Some things to be resolved. On top of this this code is a copy and pasta from CKEDITOR.tools.eventBuffer we should refactor both methods to use a common code base.

mlewand · 2018-05-29T15:48:30Z

tests/core/tools.js

+				foo = 'foo',
+				buz = 'buz',
+				message,
+				buffer = CKEDITOR.tools.throttle( 200, function( arg ) {


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.

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.

mlewand · 2018-05-29T15:51:35Z

tests/core/tools.js

+			buffer.input();
+			buffer.reset();
+
+			wait( function() {


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

mlewand · 2018-05-29T16:09:15Z

core/tools.js

+		 * Throttles `input` events (or any `input` calls)
+		 * and triggers `output` not more often than once per `minInterval`.
+		 *
+		 * After each `input` scheduled `output` will be destroyed and replaced with


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.

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.

I think it works well 👍

mlewand · 2018-05-29T16:09:47Z

core/tools.js

+		 * After each `input` scheduled `output` will be destroyed and replaced with
+		 * new one. It gives you the opportunity to modify passed parameters into `input` function and get
+		 * different results.
+		 *


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

mlewand · 2018-05-29T16:11:35Z

core/tools.js

+		 *
+		 *		buffer.input( 'foo!' );
+		 *		// 'foo!' logged immediately.
+		 *		buffer.input( 'buz!' );


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.

mlewand · 2018-05-29T16:13:07Z

core/tools.js

+		 * new one. It gives you the opportunity to modify passed parameters into `input` function and get
+		 * different results.
+		 *
+		 *		var buffer = CKEDITOR.tools.throttle( 200, function( message ) {


Listings should be wrapped with triple backticks + language hit 🙀

mlewand · 2018-05-29T16:14:46Z

core/tools.js

+		 * @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).


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.

mlewand · 2018-05-29T16:16:49Z

tests/core/tools.js

+			buffer.input( foo );
+
+			assert.areSame( 1, output );
+			assert.areSame( foo, message );


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.

mlewand · 2018-05-29T16:17:42Z

tests/core/tools.js

+					buffer.input( foo );
+
+					assert.areSame( 2, output );
+					assert.areSame( buz, message );


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

mlewand · 2018-05-29T16:22:51Z

CHANGES.md

@@ -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.


It should land in the API section.

…rgument for throttled calls.

mlewand · 2018-05-29T16:28:02Z

I have also pushed a single test case explicitly checking that the last argument is always used for throttled functions.

mlewand · 2018-06-01T09:57:12Z

tests/core/tools.js

+			assert.isTrue( inputSpy.calledWithExactly( foo ), 'Call argument the after second call' );
+
+			wait( function() {
+				assert.isTrue( inputSpy.calledOnce, 'Call count after the second call timeout (1st)' );


This assertion does not display call count as it says. Actually I'd rather see that in the assertion, so:

assert.areSame( 1, inputSpy.callCount, 'Call count after the second call timeout (1st)' );

mlewand · 2018-06-01T10:34:37Z

core/tools.js

+		 * Throttles `input` events (or any `input` calls)
+		 * and triggers `output` not more often than once per `minInterval`.
+		 *
+		 * After each `input` scheduled `output` will be destroyed and replaced with


I think it works well 👍

mlewand

Code looks much better, however you did not address one important point from review summary:

Some things to be resolved. On top of this this code is a copy and pasta from CKEDITOR.tools.eventBuffer we should refactor both methods to use a common code base.

We can extract it to a separate issue though.

The only thing that we need to work on are the docs. I'll adjust it in a following PR and I think we're good.

mlewand · 2018-06-01T10:36:23Z

core/tools.js

@@ -579,6 +579,95 @@
 			}, milliseconds || 0 );
 		},

+		/**
+		 * Throttles `input` events (or any `input` calls)


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

mlewand · 2018-06-01T11:46:31Z

core/tools.js

+		 * @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 &mdash; `output` will not be executed


Buffered ~~events~~ calls.

mlewand · 2018-06-01T12:33:45Z

I have adjusted things that I was missing. Request from #1996 (review) extracted to #2045.

jacekbogdanski mentioned this pull request May 22, 2018

Added autocomplete throttle feature #2001

Merged

2 tasks

mlewand self-requested a review May 24, 2018 12:23

jacekbogdanski force-pushed the t/1751 branch from ab712c3 to ac6bad8 Compare May 24, 2018 16:42

jacekbogdanski added 4 commits May 25, 2018 12:39

Added throttle function.

14bfa1c

Added unit tests.

a359ff1

Added manual test.

affd52e

Changelog entry.

73aa69d

jacekbogdanski force-pushed the t/1993 branch from 1557e4c to 73aa69d Compare May 25, 2018 10:40

mlewand suggested changes May 29, 2018

View reviewed changes

Tests: added a test case ensuring that throttle always use the last a…

9f83ca5

…rgument for throttled calls.

jacekbogdanski added 3 commits May 30, 2018 12:42

Fixed unit tests.

b465c9d

Fixed changelog.

c291d41

Docs refactor.

0307707

jacekbogdanski requested a review from mlewand May 30, 2018 11:32

mlewand reviewed Jun 1, 2018

View reviewed changes

mlewand suggested changes Jun 1, 2018

View reviewed changes

Docs: adjusted CKEDITOR.tools.throttle API docs.

051fd74

mlewand mentioned this pull request Jun 1, 2018

Reuse code in tools.throttle and tools.eventsBuffer #2045

Closed

Tests: adjusted assertions for CKEDITOR.tools.throttle.

2d2bea6

mlewand approved these changes Jun 1, 2018

View reviewed changes

mlewand merged commit 77470cc into t/1751 Jun 1, 2018

mlewand deleted the t/1993 branch June 1, 2018 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added throttling function #1996

Added throttling function #1996

jacekbogdanski commented May 22, 2018

mlewand commented May 25, 2018

mlewand left a comment

mlewand May 29, 2018

jacekbogdanski May 30, 2018

mlewand May 29, 2018

mlewand May 29, 2018

jacekbogdanski May 30, 2018 •

edited

Loading

mlewand Jun 1, 2018

mlewand May 29, 2018

mlewand May 29, 2018

mlewand May 29, 2018

mlewand May 29, 2018

mlewand May 29, 2018

mlewand May 29, 2018

mlewand May 29, 2018

mlewand commented May 29, 2018

mlewand Jun 1, 2018

mlewand Jun 1, 2018

mlewand left a comment

mlewand Jun 1, 2018

mlewand Jun 1, 2018

mlewand commented Jun 1, 2018

Added throttling function #1996

Added throttling function #1996

Conversation

jacekbogdanski commented May 22, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

mlewand commented May 25, 2018

mlewand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacekbogdanski May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand commented Jun 1, 2018

jacekbogdanski May 30, 2018 •

edited

Loading