Added autocomplete throttle feature #2001

jacekbogdanski · 2018-05-22T13:39:09Z

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?

Implemented throttle feature inside textwatcher / autocomplete plugins. Refactored autocomplete constructor to use CKEDITOR.plugins.autocomplete.configDefinition abstract class.

Based on #1993 (PR #1996).

Closes #1997

mlewand · 2018-06-01T13:04:25Z

Please rebase the branch onto latest upstream branch.

jacekbogdanski · 2018-06-01T13:56:56Z

Rebased into t/1751. I think we should refactor check for the supported environment into tests/plugins/autocomplete/manual/_helpers/utils.js but I would like to do it on t/1751 branch because this one already contains too much refactoring.

mlewand

Please, rebase the branch onto latest autocomplete branch. After dummy rebase, it yields errors in unit tests.

jacekbogdanski · 2018-06-04T13:12:59Z

Rebased onto the latest t/1751.

mlewand

Not all occurences of consturctor calls were updated:

https://github.com/ckeditor/ckeditor-dev/blob/5a130a9/plugins/autocomplete/plugin.js#L94

mlewand · 2018-06-07T08:33:16Z

plugins/autocomplete/plugin.js

+
+		/**
+		 * The editor instance to which autocomplete is attached to.
+		 *


Looks like a merge artifact 🙂

mlewand · 2018-06-07T08:37:33Z

plugins/autocomplete/plugin.js

+	 */
+
+	/**
+	 * Indicates throttle threshold mitigating text checks.


First, docs for this property should clearly express that these are milliseconds given here.

I think that the second part:

Higher levels of throttle (…)

Is not really necessarry, as one should understand this consequence based on information that this is throttling expressed in milliseconds.

mlewand · 2018-06-07T08:39:33Z

plugins/autocomplete/plugin.js

+	 * Higher levels of throttle threshold will create visible delay for autocomplete view
+	 * but also save the number of {@link #dataCallback} calls.
+	 *
+	 * @property {Number} [throttle=0]


The default value for throttling should be greater than zero. Let's make it to a small value that is barely noticable. Let's make it 20ms so that gives us 50fps.

mlewand · 2018-06-07T08:48:21Z

plugins/textwatcher/plugin.js

@@ -261,6 +271,21 @@
 		}
 	};

+	function check( selectionRange ) {


Now that you moved it to a higher scope, this function name collides with other existing function in one scenario:

https://github.com/ckeditor/ckeditor-dev/blob/5a130a9/plugins/textwatcher/plugin.js#L202

Not a good thing, we want to avoid colliding functions. Move it instead to the closest scope where it is needed, so to the constructor where it is used and that will solve this problem.

Also this name could be more meaningful. Since it mostly relates to calling a callback, I think that should be somehow reflected in the name.

mlewand · 2018-06-07T08:54:10Z

tests/plugins/autocomplete/manual/throttle.md

+
+## Expected
+
+1. After `@` character dropdown should contain multiple items and appear immediately.


It won't appear immediately if you focus the editor with a tab key instead of mouse. That's because tab keyup event will trigger the initial check (that is run synchronously) and your typed @ key will get throttled.

I suggest that the first step tells explicitly to focus the editor using mouse.

I'm aslo wondering whether we should put an information at the beginning asking to read expected section before starting the test, as the timing is crucial. What I mean here is that I can see someone spending 2.5 sec on performing first step, reading expected and performing 3-4. In this case result for typing "a" would appear (almost) instnatly.

mlewand · 2018-06-07T09:05:22Z

tests/plugins/textwatcher/manual/throttle.md

+
+Typed text should be logged:
+1. Immediately after first typed character.
+1. After 2000ms intervals.


Technically it should be something like not more often than once every 2000ms. Or something.

I think logging the time (including millisecs) would be very helpful for this test.

mlewand · 2018-06-07T09:15:31Z

plugins/autocomplete/plugin.js

+	 * Simple usage:
+	 *
+	 * ```javascript
+	 * var definition = { dataCallback: dataCallback, textTestCallback: textTestCallback, throttle: 200 };


Avoid putting objects in a single line. One property per line.

mlewand · 2018-06-07T09:17:07Z

plugins/autocomplete/plugin.js

-	 * @param {String} [itemTemplate] Template for list item in dropdown. See {@link CKEDITOR.plugins.autocomplete.view#itemTemplate} for more information.
-	 * @param {String} [outputTemplate] Template for match rendering. See {@link #outputTemplate}.
+	 * @param {CKEDITOR.plugins.autocomplete.configDefinition} config Configuration object keeping information
+	 * how to instantiate autocomplete plugin.


instantiate autocomplete plugin

It's not instantiating the plugin, it's more of a component. I'd just simpify it to "Configuration object for this autocomplete instance." or something alike.

mlewand · 2018-06-07T09:18:04Z

plugins/autocomplete/plugin.js

+		/**
+		 * See {@link CKEDITOR.plugins.autocomplete.configDefinition#throttle}.
+		 *
+		 * @property {Number} [throttle=0]


I'd avaoid duplicating the default value here. Keep it only within configDefinition#throttle instead.

I mean code:

config.throttle || 0;

Is a good defensive way of coding, and that's fine. My concern is only related to API docs.

@property {Number} [throttle=0]

mlewand

Pushed some minor adjustments.

I just saw couple of minor issues in the recent code, and we're close to finish this issue.

mlewand · 2018-06-08T12:23:05Z

plugins/autocomplete/plugin.js

@@ -91,7 +91,10 @@
 	 *	}
 	 *
 	 *	// Finally, instantiate the autocomplete class.
-	 *	new CKEDITOR.plugins.autocomplete( editor, textTestCallback, dataCallback );
+	 *	new CKEDITOR.plugins.autocomplete( editor, {
+	 *  	textTestCallback: textTestCallback,


Mixed tab and spaces, this and following lines.

mlewand · 2018-06-08T12:44:34Z

tests/plugins/textwatcher/manual/throttle.html

+				var counter = 0,
+					counterInterval = 100,
+
+					textWatcher = new CKEDITOR.plugins.textWatcher( editor, function( selectionRange ) {


Formatting is pretty bad here. First, this empty line between counterInterval and textWatcher is a little misleading, but a bigger problem is missing indentation after opening the function, and wrong braces indentation (closing the function).

mlewand · 2018-06-08T12:54:29Z

tests/plugins/textwatcher/manual/throttle.html

+					logger.appendChild( matchEl );
+
+					if ( counter == 0 ) {
+						setInterval( function() {


That's a bit overcomplicated solution… use simply performance.now() and display a difference between last and current call.

However currently performance.now() has an issue that it might be off by +/- 2 ms, see mdn for more details:

The timestamp is not actually high-resolution. To mitigate security threats such as Spectre, browsers currently round the result to varying degrees. (Firefox started rounding to 2 milliseconds in Firefox 59.) Some browsers may also slightly randomize the timestamp. The precision may improve again in future releases; browser developers are still investigating these timing attacks and how best to mitigate them.

In that case I propose to set throttling in the sample to 2002 ms, and issue description should still say that the calls should be no more often than 2000 ms.

Finally make sure to put a short comment (even simply to this comment) in the code, right above the 2002 threshold value.

After doing that you'll be able to remove this fragment:

Other details Measured time by logger can have slight error +/- 100ms.

mlewand · 2018-06-08T12:55:58Z

tests/plugins/textwatcher/manual/throttle.md

+
+Typed text is logged immediately or in invalid interval times.
+
+***Other details*** Measured time by logger can have slight error +/- 100ms.


For the record, use more common way to bold, which is simply double ** character.

mlewand

I have fixed mentioned review issues, and rebased onto latest upstream branch (resolved one conflict during the rebase).

jacekbogdanski mentioned this pull request May 23, 2018

Added mentions output template #1992

Merged

2 tasks

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

jacekbogdanski force-pushed the t/1997 branch from e17eb70 to 800ad86 Compare May 25, 2018 10:52

mlewand self-requested a review June 1, 2018 12:46

jacekbogdanski force-pushed the t/1997 branch from 800ad86 to 86b6800 Compare June 1, 2018 13:52

mlewand suggested changes Jun 4, 2018

View reviewed changes

jacekbogdanski force-pushed the t/1997 branch from 86b6800 to 5a130a9 Compare June 4, 2018 13:11

jacekbogdanski requested a review from mlewand June 4, 2018 13:13

mlewand suggested changes Jun 7, 2018

View reviewed changes

jacekbogdanski requested a review from mlewand June 7, 2018 11:12

mlewand suggested changes Jun 8, 2018

View reviewed changes

jacekbogdanski and others added 16 commits June 8, 2018 15:29

Added textwatcher throttle.

47852c3

Added textwatcher unit test.

69a9007

Added textwatcher manual test.

e038752

Added autocomplete throttle, refactored constructor, docs.

0dde81b

Fixed unit tests.

c119549

Docs refactor.

6354042

Added autocomplete throttle tests.

7a5e06c

Rebase fixes.

06ade88

Fix after rebase.

8766d35

Updated autocomplete docs and added throttle threshold.

31b477e

Refactored tests.

168aebc

Refactored check function.

35c49b8

Removed empty line.

5aaee75

Docs: API docs rewording.

b2c1f49

Refactoring: renamed textwatcher text matching function.

ff99f27

Refactoring: post review corrections.

8f6a4b3

mlewand force-pushed the t/1997 branch from 913bef2 to 8f6a4b3 Compare June 8, 2018 13:32

mlewand approved these changes Jun 8, 2018

View reviewed changes

mlewand merged commit 9e711ca into t/1751 Jun 8, 2018

mlewand deleted the t/1997 branch June 8, 2018 13:34

mlewand mentioned this pull request Jun 8, 2018

Autocomplete plugin requests should be throttled #1997

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added autocomplete throttle feature #2001

Added autocomplete throttle feature #2001

jacekbogdanski commented May 22, 2018

mlewand commented Jun 1, 2018

jacekbogdanski commented Jun 1, 2018 •

edited

Loading

mlewand left a comment

jacekbogdanski commented Jun 4, 2018

mlewand left a comment

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand Jun 7, 2018

mlewand left a comment

mlewand Jun 8, 2018

mlewand Jun 8, 2018

mlewand Jun 8, 2018

mlewand Jun 8, 2018

mlewand Jun 8, 2018

mlewand left a comment


		## Expected

		1. After `@` character dropdown should contain multiple items and appear immediately.


		Typed text is logged immediately or in invalid interval times.

		*Other details* Measured time by logger can have slight error +/- 100ms.

Added autocomplete throttle feature #2001

Added autocomplete throttle feature #2001

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 Jun 1, 2018

jacekbogdanski commented Jun 1, 2018 • edited Loading

mlewand left a comment

Choose a reason for hiding this comment

jacekbogdanski commented Jun 4, 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

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

Choose a reason for hiding this comment

mlewand left a comment

Choose a reason for hiding this comment

jacekbogdanski commented Jun 1, 2018 •

edited

Loading