Introduce word count plugin #2

msamsel · 2019-06-12T12:15:53Z

Suggested merge commit message (convention)

Feature: Introduce word count feature. Closes ckeditor/ckeditor5#2571. Closes ckeditor/ckeditor5#1301.

Additional information

Reinmar · 2019-06-25T13:55:31Z

docs/_snippets/features/word-count-update.html

+<div class="customized-counter">
+	<div class="customized-counter__words">
+		<label>Words:
+			<progress value="48" max='100'></progress>


msamsel · 2019-06-25T14:18:06Z

I add PR with modification of mgit.json.

ma2ciek · 2019-06-25T21:59:17Z

src/wordcount.js

+	init() {
+		const editor = this.editor;
+
+		editor.model.document.on( 'change', throttle( this._calcWordsAndCharacters.bind( this ), 250 ) );


Maybe we could listen to change:data events only?

This must be a change:data - there's no need to updated word count on selection changes.

jodator

The plugin works nicely, I had some comments regarding wording and some code improvements.

I've checked how fast is the word counting and it looks pretty OK:

However, I found some issues regarding specs:

There's update event instead of onUpdate callback (for me it's fine). Also, we have observable properties.
I'm not sure if the myContainer is implemented the way it was intended. Should the WordCount plugin accept the existing container to be passed to the plugin and to be used instead of creating it internally? Of course only if the wordCount.container is defined.
The Words: NNN. Characters: NNN requirement is done as to blocks - not as inline elements so it displays:
```
Words: NNN.
Characters: NNN
```

@Reinmar could you check those two requirements above?

jodator · 2019-06-26T09:57:43Z

src/utils.js

+ *
+ * **Note:** Function walks through the entire model. There should be considered throttling during usage.
+ *
+ * @param {module:engine/model/node~Node} node


wrong param: should be Element - the Node doesn't have `getChildren()

It actually needs to be Item because it's also called with Text and TextProxy.

But, at the same time, you're completely right that neither of them have getChildren(). Which means that if we'd use a statically typed language we'd need to typecast here or solve it differently. That's why I wrote in the ticket that you reported that we should wait with such polishing until we use TS.

It looks so - I wrote that ticket after this review actually. I worry that we can have more such mistype declaration in the docs for Node/Element/Item trio.

jodator · 2019-06-26T10:14:36Z

src/utils.js

+
+	if ( node.is( 'text' ) || node.is( 'textProxy' ) ) {
+		text += node.data;
+	} else {


I'd remove the indentation here: return on if and remove the else block. Also you could move let text = '' after if then.

The other question is do you consider TreeWalker here? I don't know if its much overhead (currently parsing a 1M characters (few paragraphs) takes below 30 ms so it is performant enough).

I've made simple code with treewalker and performance dropped significantly.
In my case it was slow down ~50 times, I'm not sure if it's case of my algorithm with tree walker or tree walker itself, however, it seems that walking through children recursively is far more efficient.

current code:

simple tree walking:

const txt = ( editor => { const position = new Position( editor.model.document.getRoot(), [ 0 ] ); const walker = new TreeWalker( { startPosition: position } ); let endTagIndicator = false; let outputText = ''; for ( const element of walker ) { if ( element.typy === 'elementStart' && endTagIndicator ) { outputText += '\n'; endTagIndicator = false; } else if ( element.type === 'elementEnd' ) { endTagIndicator = true; } else if ( element.type === 'text' ) { outputText += element.item.data; } } return outputText; } )( this.editor );

jodator · 2019-06-26T10:32:23Z

src/wordcount.js

+	init() {
+		const editor = this.editor;
+
+		editor.model.document.on( 'change', throttle( this._calcWordsAndCharacters.bind( this ), 250 ) );


This must be a change:data - there's no need to updated word count on selection changes.

jodator · 2019-06-26T10:40:18Z

src/wordcount.js

+			const children = [];
+
+			if ( displayWords || displayWords === undefined ) {
+				const wordsLabel = t( 'Words' ) + ':';


Translate full message: t( 'Words: %0', [ words ] ).

Example: https://github.com/ckeditor/ckeditor5-ui/blob/deb6b06aee480b659ce017a7c2940be67b79262a/src/panel/balloon/contextualballoon.js#L403-L411

jodator · 2019-06-26T10:40:43Z

src/wordcount.js

+			}
+
+			if ( displayCharacters || displayCharacters === undefined ) {
+				const charactersLabel = t( 'Characters' ) + ':';


same as above ☝️ full message to translate.

jodator · 2019-06-26T12:16:55Z

src/wordcount.js

+}
+
+/**
+ * Event is fired after {@link #words} and {@link #characters} are updated.


Suggested change

* Event is fired after {@link #words} and {@link #characters} are updated.

* Event fired after {@link #words} and {@link #characters} are updated.

jodator · 2019-06-26T12:17:50Z

src/wordcount.js

+ */
+
+/**
+ * This option allows on hiding the word counter. The element obtained through


Suggested change

* This option allows on hiding the word counter. The element obtained through

* This option allows for hiding the word counter. The element obtained through

jodator · 2019-06-26T12:18:23Z

src/wordcount.js

+ * The mentioned configuration will result with the followed container:
+ *
+ *		<div class="ck ck-word-count">
+ *			<div>Characters: 28</div>


Remember to update the examples code when adding classes to it.

jodator · 2019-06-26T12:18:40Z

src/wordcount.js

+ */
+
+/**
+ * This option allows on hiding the character counter. The element obtained through


Suggested change

* This option allows on hiding the character counter. The element obtained through

* This option allows for hiding the character counter. The element obtained through

jodator · 2019-06-26T12:19:58Z

src/wordcount.js

+ *			displayCharacters = false
+ *		}
+ *
+ * The mentioned configuration will result with the followed container:


Suggested change

* The mentioned configuration will result with the followed container:

* The mentioned configuration will result in the following container

Reinmar · 2019-06-26T12:35:07Z

There's update event instead of onUpdate callback (for me it's fine). Also, we have observable properties.

The point of config.wordCount.onUpdate is that it's easier to define such callback together with the rest of the configuration, rather than somewhere in the then() block after the editor is initialised. Similar to https://ckeditor.com/docs/ckeditor5/latest/api/module_autosave_autosave-AutosaveConfig.html. That doesn't mean that there must be no event, though. The config property can be a shorthand for that event.

jodator · 2019-06-26T12:37:58Z

For the samples in the docs: I'd change the update event sample so the progress bar has also NNN of 1000 words (or whatever is the number and the character box also has meaningful information. Maybe transition blue > yellow > red would be sufficient?

Reinmar · 2019-06-26T12:38:24Z

I'm not sure if the myContainer is implemented the way it was intended. Should the WordCount plugin accept the existing container to be passed to the plugin and to be used instead of creating it internally? Of course only if the wordCount.container is defined.

As shown in the code sample in https://github.com/ckeditor/ckeditor5-word-count/issues/1 both should work. Either you pass the container via config.wordCount.container and the plugin will use it. Or you don't pass it and the plugin creates it on its own and you can access it via editor.plugins.get( 'WordCount' ).wordCountContainer.

Reinmar · 2019-06-26T12:43:38Z

3. The Words: NNN. Characters: NNN requirement is done as to blocks - not as inline elements so it displays:
Words: NNN. Characters: NNN

No strong opinion here. The styles can be changed, but it'd be good to default to a nice clean markup by default (I guess a bunch of spans with nice BEM classes). CC @oleq @dkonopka

jodator · 2019-06-26T12:54:21Z

Thanks! So @msamsel so the points 1-2 should be targeted. For the 3 I've already proposed adding BEM classes to those containers and the only remaining thing is how to structure them (div or span?).

scofalik · 2019-06-27T09:21:11Z

Did you check how efficient it is for a document with 20+ A4 pages? 50+?

dkonopka · 2019-06-27T09:33:04Z

WDYT to provide some default styles for the word counter and introduce them in ckeditor5-theme-lark. Some proposal:

Also, it would be nice to add a specific class to div inside .ck-word-count and for elements inside it (I mean something like .ck-word-count__label & .ck-word-count__value );

docs/features/word-count.md

jodator · 2019-06-27T09:44:47Z

Also, it would be nice to add a specific class to div inside .ck-word-count and for elements inside it (I mean something like .ck-word-count__label & .ck-word-count__value );

I've already proposed to have the whole label translatable so it would land in one element. The idea behind that was to translate: Words: %0 instead of only Words. Similarly, as we do it in the panel rotator/carousel where we translate %0 of %1 and not only of.

jodator

@msamsel Some polishing to do.

src/wordcount.js

jodator · 2019-06-28T13:30:53Z

tests/wordcount.js

 		it( 'has "WordCount" plugin name', () => {
 			expect( WordCount.pluginName ).to.equal( 'WordCount' );
 		} );
+
+		it( 'has define "_config" object', () => {


I don't think we need to check for private properties.

I removed this test

tests/wordcount.js

tests/utils.js

Co-Authored-By: Maciej <[email protected]>

jodator · 2019-06-28T13:55:27Z

@Reinmar the issues were addressed :) I've created one follow-up: https://github.com/ckeditor/ckeditor5-word-count/issues/5 - but it might be done later together with #4.

Also the idea of breaking up the words: %n in the view is kinda open but I would leave it as is or do it as a follow-up.

lang/contexts.json

Reinmar · 2019-06-30T20:45:20Z

src/wordcount.js

+	 *
+	 * @returns {HTMLElement}
+	 */
+	getWordCountContainer() {


Couldn't this be a getter? It was proposed as a property in the spec: https://github.com/ckeditor/ckeditor5-word-count/issues/1.

Co-Authored-By: Piotrek Koszuliński <[email protected]>

docs/features/word-count.md

lang/contexts.json

src/utils.js

Reinmar · 2019-07-01T08:03:36Z

src/utils.js

+ * @returns {String} Plain text representing model's data
+ */
+export function modelElementToPlainText( element ) {
+	if ( element.is( 'text' ) || element.is( 'textProxy' ) ) {


According to the docs this isn't allowed. The element is an element. Please report a followup ticket to clean this up.

My other problem with this is that we already have viewItemToPlainText in ckeditor5-clipboard. Ideally, those functions should be kept together, but I don't know where would it be. Engine?

I'd go with Engine. Ps. this method would be suitable also for other features (AFAIR: Mention or TextWatcher in particular and maybe the Autoformat - the latter should use TextWatcher API anyway).

However this method might soon be changed to follow requirements from #5.

src/utils.js

Reinmar · 2019-07-01T08:43:04Z

You missed some old names. I'll fix this.

Mateusz Samsel added 11 commits June 3, 2019 09:30

Add required dependencies.

9f4fd8d

Add plugins base.

ad4684e

Add some simple unit test.

76d7485

Fix docs typo.

8e59ff9

Small typo and white characters improvements.

1f0a05b

Add unit tests for word count plugin.

faff505

Update manual test description ad functions.

a92751d

Extend documentation for word count plugin.

fe53c68

Add docs article describing the feature.

a7b485f

Docs improvements.

5b5ab05

Improve snippet's style.

88858fd

Reinmar requested a review from jodator June 25, 2019 13:55

Reinmar reviewed Jun 25, 2019

View reviewed changes

msamsel mentioned this pull request Jun 25, 2019

Add word count plugin to mgit.json ckeditor/ckeditor5#1835

Closed

ma2ciek reviewed Jun 25, 2019

View reviewed changes

jodator suggested changes Jun 26, 2019

View reviewed changes

Mateusz Samsel added 2 commits June 26, 2019 15:23

Fix typos in snippets.

244f4ad

Minor tweaks to feature and tests with review.

5470728

Simplify model to plain text transformation.

0a9fd82

Reinmar reviewed Jun 27, 2019

View reviewed changes

docs/features/word-count.md Outdated Show resolved Hide resolved

Reinmar reviewed Jun 27, 2019

View reviewed changes

docs/features/word-count.md Outdated Show resolved Hide resolved

jodator suggested changes Jun 28, 2019

View reviewed changes

msamsel and others added 2 commits June 28, 2019 15:49

Apply suggestions from code review

2677157

Co-Authored-By: Maciej <[email protected]>

Remove unit test, which checks existence of private property.

c42606d

msamsel requested a review from jodator June 28, 2019 14:08

Reinmar reviewed Jun 30, 2019

View reviewed changes

lang/contexts.json Outdated Show resolved Hide resolved

Reinmar reviewed Jun 30, 2019

View reviewed changes

lang/contexts.json Outdated Show resolved Hide resolved

Reinmar reviewed Jun 30, 2019

View reviewed changes

Apply suggestions from code review

dae254f

Co-Authored-By: Piotrek Koszuliński <[email protected]>

Reinmar self-assigned this Jul 1, 2019

Transform method into getter. Update docs and tests.

57bfe06