From 5a8eb075d19195ec27b675e710a10a5ea99c0ae0 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 17 May 2018 15:29:50 +0200 Subject: [PATCH 01/32] WIP - initial implementation of autosave feature. --- package.json | 3 ++ src/autosave.js | 119 ++++++++++++++++++++++++++++++++++++++++++++++ tests/autosave.js | 46 ++++++++++++++++++ 3 files changed, 168 insertions(+) create mode 100644 src/autosave.js create mode 100644 tests/autosave.js diff --git a/package.json b/package.json index 5a14841..c929bd3 100644 --- a/package.json +++ b/package.json @@ -9,8 +9,11 @@ "ckeditor5-feature" ], "dependencies": { + "@ckeditor/ckeditor5-utils": "^10.0.0", + "@ckeditor/ckeditor5-core": "^10.0.0" }, "devDependencies": { + "@ckeditor/ckeditor5-engine": "^10.0.0", "eslint": "^4.15.0", "eslint-config-ckeditor5": "^1.0.7", "husky": "^0.14.3", diff --git a/src/autosave.js b/src/autosave.js new file mode 100644 index 0000000..4f68fe0 --- /dev/null +++ b/src/autosave.js @@ -0,0 +1,119 @@ +/** + * Copyright (c) 2016 - 2017, CKSource - Frederico Knabben. All rights reserved. + */ + +import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; +import PendingActions from '@ckeditor/ckeditor5-core/src/pendingactions'; +import throttle from '@ckeditor/ckeditor5-utils/src/lib/lodash/throttle'; +import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; + +/* globals window */ + +/** + * TODO: ... + * Autosave plugin provides an easy-to-use API for getting sync with the editor's content. + * It watches `Document:change`, and `Window:beforeunload` events. + * At these moments, editor.getBody() and editor.getTitle() should work. + */ +export default class Autosave extends Plugin { + static get pluginName() { + return 'Autosave'; + } + + static get requires() { + return [ PendingActions ]; + } + + constructor( editor ) { + super( editor ); + + /** + * @public + * @member {Provider} + */ + this.provider = undefined; + + /** + * @private + * @type {Function} + */ + this._throttledSave = throttle( this._save.bind( this ), 500 ); + + /** + * @private + * @type {DomEmitterMixin} + */ + this._domEmitter = Object.create( DomEmitterMixin ); + + /** + * @protected + * @type {Number} + */ + this._lastDocumentVersion = editor.model.document.version; + } + + init() { + const editor = this.editor; + const doc = editor.model.document; + const pendingActions = editor.plugins.get( PendingActions ); + + this.listenTo( doc, 'change', this._throttledSave ); + + // TODO: Docs + // Flush on the editor's destroy listener with the highest priority to ensure that + // `editor.getBody` will be called before plugins are destroyed. + this.listenTo( editor, 'destroy', () => this._save(), { priority: 'highest' } ); + + // It's not possible to easy test it because karma uses `beforeunload` event + // to warn before full page reload and this event cannot be dispatched manually. + /* istanbul ignore next */ + this._domEmitter.listenTo( window, 'beforeunload', ( evtInfo, domEvt ) => { + if ( pendingActions.isPending ) { + domEvt.returnValue = pendingActions.first.message; + } + } ); + } + + destroy() { + this._throttledSave.cancel(); + this._domEmitter.stopListening(); + super.destroy(); + } + + /** + * TODO: Docs. + */ + save() { + this._throttledSave.cancel(); + + const version = this.editor.model.document.version; + + if ( version <= this._lastDocumentVersion ) { + return Promise.resolve(); + } + + this._lastDocumentVersion = version; + + if ( !this.provider ) { + return Promise.resolve(); + } + + // TODO: add pending action. + + return Promise.resolve() + .then( () => this.provider.save() ) + .then( () => { + // TODO: remove pending action. + } ); + } +} + +/** + * @typedef Provider + */ + +/** + * @function + * @name Provider#save + * @type {Function} + */ diff --git a/tests/autosave.js b/tests/autosave.js new file mode 100644 index 0000000..2ccf03f --- /dev/null +++ b/tests/autosave.js @@ -0,0 +1,46 @@ +/** + * Copyright (c) 2016 - 2017, CKSource - Frederico Knabben. All rights reserved. + */ + +/* globals document */ + +// import ModelText from '@ckeditor/ckeditor5-engine/src/model/text'; +// import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range'; +import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; +import Autosave from '../src/autosave'; + +describe( 'Autosave', () => { + const sandbox = sinon.sandbox.create(); + let editor, element, autosave; + + beforeEach( () => { + element = document.createElement( 'div' ); + document.body.appendChild( element ); + + return ClassicTestEditor + .create( element, { + plugins: [] + } ) + .then( _editor => { + editor = _editor; + autosave = editor.plugins.get( Autosave ); + } ); + } ); + + afterEach( () => { + document.body.removeChild( element ); + sandbox.restore(); + + return editor.destroy(); + } ); + + it( 'should have static pluginName property', () => { + expect( Autosave.pluginName ).to.equal( 'Autosave' ); + } ); + + describe( 'initialization', () => { + it( 'should initialize provider with an undefined value', () => { + expect( autosave.provider ).to.be.undefined; + } ); + } ); +} ); From 036cc64337d08d84b8631b047b2fa761cb3fe826 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 17 May 2018 18:08:06 +0200 Subject: [PATCH 02/32] Added basic implementation. --- package.json | 1 + src/autosave.js | 31 ++++++----- tests/autosave.js | 134 ++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 149 insertions(+), 17 deletions(-) diff --git a/package.json b/package.json index c929bd3..3c0047b 100644 --- a/package.json +++ b/package.json @@ -14,6 +14,7 @@ }, "devDependencies": { "@ckeditor/ckeditor5-engine": "^10.0.0", + "@ckeditor/ckeditor5-paragraph": "^10.0.0", "eslint": "^4.15.0", "eslint-config-ckeditor5": "^1.0.7", "husky": "^0.14.3", diff --git a/src/autosave.js b/src/autosave.js index 4f68fe0..ff83203 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -28,13 +28,12 @@ export default class Autosave extends Plugin { super( editor ); /** - * @public * @member {Provider} */ this.provider = undefined; /** - * @private + * @protected * @type {Function} */ this._throttledSave = throttle( this._save.bind( this ), 500 ); @@ -81,29 +80,35 @@ export default class Autosave extends Plugin { } /** - * TODO: Docs. + * @protected */ - save() { - this._throttledSave.cancel(); + _flush() { + this._throttledSave.flush(); + } + + /** + * @protected + */ + _save() { + if ( !this.provider ) { + return; + } const version = this.editor.model.document.version; if ( version <= this._lastDocumentVersion ) { - return Promise.resolve(); + return; } this._lastDocumentVersion = version; - if ( !this.provider ) { - return Promise.resolve(); - } - - // TODO: add pending action. + const pendingActions = this.editor.plugins.get( PendingActions ); + const action = pendingActions.add( 'Saving in progress.' ); - return Promise.resolve() + Promise.resolve() .then( () => this.provider.save() ) .then( () => { - // TODO: remove pending action. + pendingActions.remove( action ); } ); } } diff --git a/tests/autosave.js b/tests/autosave.js index 2ccf03f..f59f99b 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -2,12 +2,14 @@ * Copyright (c) 2016 - 2017, CKSource - Frederico Knabben. All rights reserved. */ -/* globals document */ +/* globals document, window */ -// import ModelText from '@ckeditor/ckeditor5-engine/src/model/text'; -// import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range'; +import ModelText from '@ckeditor/ckeditor5-engine/src/model/text'; +import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range'; import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; import Autosave from '../src/autosave'; +import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; +import PendingActions from '@ckeditor/ckeditor5-core/src/pendingactions'; describe( 'Autosave', () => { const sandbox = sinon.sandbox.create(); @@ -19,11 +21,17 @@ describe( 'Autosave', () => { return ClassicTestEditor .create( element, { - plugins: [] + plugins: [ Autosave, Paragraph ] } ) .then( _editor => { + const data = '

paragraph1

paragraph2

'; + editor = _editor; + editor.setData( data ); autosave = editor.plugins.get( Autosave ); + + // Clean autosave's state after setting data. + autosave._throttledSave.cancel(); } ); } ); @@ -42,5 +50,123 @@ describe( 'Autosave', () => { it( 'should initialize provider with an undefined value', () => { expect( autosave.provider ).to.be.undefined; } ); + + it( 'should allow plugin to work without any defined provider', () => { + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + // Go to the next cycle to because synchronization of CS documentVersion is async. + return Promise.resolve().then( () => { + autosave._flush(); + } ); + } ); + } ); + + describe( 'autosaving', () => { + it( 'should run provider\'s save method when the editor\'s change event is fired', () => { + autosave.provider = { + save: sinon.spy() + }; + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + // Go to the next cycle to because synchronization of CS documentVersion is async. + return Promise.resolve() + .then( () => autosave._flush() ) + .then( () => { + sinon.assert.calledOnce( autosave.provider.save ); + } ); + } ); + + it( 'should throttle editor\'s change event', () => { + const spy = sinon.spy(); + const savedStates = []; + + autosave.provider = { + save() { + spy(); + + savedStates.push( editor.getData() ); + } + }; + + // Leading (will fire change). + const change1 = () => { + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + }; + + const change2 = () => { + // Throttled (won't fire change). + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); + editor.model.insertContent( new ModelText( 'bar' ), editor.model.document.selection ); + } ); + }; + + const change3 = () => { + // Flushed (will fire change). + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); + editor.model.insertContent( new ModelText( 'biz' ), editor.model.document.selection ); + } ); + }; + + // Provider.save is done asynchronously, so all changes are run in different cycles. + return Promise.resolve() + .then( () => change1() ) + .then( () => change2() ) + .then( () => change3() ) + .then( () => autosave._flush() ) + .then( () => { + expect( spy.callCount ).to.equal( 2 ); + expect( savedStates ).to.deep.equal( [ + '

paragraph1

foo

', + '

paragraph1

biz

' + ] ); + } ); + } ); + + it( 'should add a pending action during the saving.', () => { + const wait5msSpy = sinon.spy(); + const pendingActions = editor.plugins.get( PendingActions ); + + autosave.provider = { + save() { + return wait5ms().then( wait5ms ); + } + }; + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + // Go to the next cycle to because synchronization of CS documentVersion is async. + return Promise.resolve() + .then( () => autosave._flush() ) + .then( () => { + sinon.assert.notCalled( wait5msSpy ); + expect( pendingActions.isPending ).to.be.true; + expect( pendingActions.first.message ).to.equal( 'Saving in progress.' ); + } ) + .then( wait5ms ) + .then( () => { + + } ); + } ); } ); + + function wait5ms() { + return new Promise( res => { + window.setTimeout( res, 5 ); + } ); + } } ); From 450e2ba042f47f4fb7aab8d61d7bbd68d489771e Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 17 May 2018 18:11:29 +0200 Subject: [PATCH 03/32] Improved pending action test. --- tests/autosave.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/autosave.js b/tests/autosave.js index f59f99b..aa4ecb4 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -140,7 +140,8 @@ describe( 'Autosave', () => { autosave.provider = { save() { - return wait5ms().then( wait5ms ); + return wait5ms() + .then( wait5msSpy ); } }; @@ -159,7 +160,8 @@ describe( 'Autosave', () => { } ) .then( wait5ms ) .then( () => { - + sinon.assert.calledOnce( wait5msSpy ); + expect( pendingActions.isPending ).to.be.false; } ); } ); } ); From c7c722bf2f8f2a16bdd57919a9b8ef887b8ecf2e Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 18 May 2018 10:53:28 +0200 Subject: [PATCH 04/32] Simplified code and tests. --- src/autosave.js | 3 +- tests/autosave.js | 94 +++++++++++++++++++---------------------------- 2 files changed, 38 insertions(+), 59 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index ff83203..4cd5ee3 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -105,8 +105,7 @@ export default class Autosave extends Plugin { const pendingActions = this.editor.plugins.get( PendingActions ); const action = pendingActions.add( 'Saving in progress.' ); - Promise.resolve() - .then( () => this.provider.save() ) + Promise.resolve( this.provider.save() ) .then( () => { pendingActions.remove( action ); } ); diff --git a/tests/autosave.js b/tests/autosave.js index aa4ecb4..0258eae 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -57,10 +57,7 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - // Go to the next cycle to because synchronization of CS documentVersion is async. - return Promise.resolve().then( () => { - autosave._flush(); - } ); + autosave._flush(); } ); } ); @@ -76,11 +73,9 @@ describe( 'Autosave', () => { } ); // Go to the next cycle to because synchronization of CS documentVersion is async. - return Promise.resolve() - .then( () => autosave._flush() ) - .then( () => { - sinon.assert.calledOnce( autosave.provider.save ); - } ); + autosave._flush(); + + sinon.assert.calledOnce( autosave.provider.save ); } ); it( 'should throttle editor\'s change event', () => { @@ -96,52 +91,40 @@ describe( 'Autosave', () => { }; // Leading (will fire change). - const change1 = () => { - editor.model.change( writer => { - writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); - editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); - } ); - }; + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); - const change2 = () => { - // Throttled (won't fire change). - editor.model.change( writer => { - writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); - editor.model.insertContent( new ModelText( 'bar' ), editor.model.document.selection ); - } ); - }; + // Throttled (won't fire change). + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); + editor.model.insertContent( new ModelText( 'bar' ), editor.model.document.selection ); + } ); - const change3 = () => { - // Flushed (will fire change). - editor.model.change( writer => { - writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); - editor.model.insertContent( new ModelText( 'biz' ), editor.model.document.selection ); - } ); - }; + // Flushed (will fire change). + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); + editor.model.insertContent( new ModelText( 'biz' ), editor.model.document.selection ); + } ); + + autosave._flush(); - // Provider.save is done asynchronously, so all changes are run in different cycles. - return Promise.resolve() - .then( () => change1() ) - .then( () => change2() ) - .then( () => change3() ) - .then( () => autosave._flush() ) - .then( () => { - expect( spy.callCount ).to.equal( 2 ); - expect( savedStates ).to.deep.equal( [ - '

paragraph1

foo

', - '

paragraph1

biz

' - ] ); - } ); + expect( spy.callCount ).to.equal( 2 ); + expect( savedStates ).to.deep.equal( [ + '

paragraph1

foo

', + '

paragraph1

biz

' + ] ); } ); it( 'should add a pending action during the saving.', () => { - const wait5msSpy = sinon.spy(); + const serverActionSpy = sinon.spy(); const pendingActions = editor.plugins.get( PendingActions ); autosave.provider = { save() { return wait5ms() - .then( wait5msSpy ); + .then( serverActionSpy ); } }; @@ -150,19 +133,16 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - // Go to the next cycle to because synchronization of CS documentVersion is async. - return Promise.resolve() - .then( () => autosave._flush() ) - .then( () => { - sinon.assert.notCalled( wait5msSpy ); - expect( pendingActions.isPending ).to.be.true; - expect( pendingActions.first.message ).to.equal( 'Saving in progress.' ); - } ) - .then( wait5ms ) - .then( () => { - sinon.assert.calledOnce( wait5msSpy ); - expect( pendingActions.isPending ).to.be.false; - } ); + autosave._flush(); + + sinon.assert.notCalled( serverActionSpy ); + expect( pendingActions.isPending ).to.be.true; + expect( pendingActions.first.message ).to.equal( 'Saving in progress.' ); + + return wait5ms().then( () => { + sinon.assert.calledOnce( serverActionSpy ); + expect( pendingActions.isPending ).to.be.false; + } ); } ); } ); From 12f4186cd871ba3607ee4d76704e30a1ee8c5d58 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 18 May 2018 11:28:35 +0200 Subject: [PATCH 05/32] Added tests. --- src/autosave.js | 4 ++++ tests/autosave.js | 57 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/src/autosave.js b/src/autosave.js index 4cd5ee3..ea0a9f3 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -87,6 +87,10 @@ export default class Autosave extends Plugin { } /** + * If the provider is set and new document version exists, + * `_save()` method creates a pending action and calls `provider.save()` method. + * It waits for the result and then removes the created pending action. + * * @protected */ _save() { diff --git a/tests/autosave.js b/tests/autosave.js index 0258eae..c5d0548 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -144,6 +144,63 @@ describe( 'Autosave', () => { expect( pendingActions.isPending ).to.be.false; } ); } ); + + it( 'should flush remaining calls after editor\'s destroy', () => { + const spy = sandbox.spy(); + const savedStates = []; + + autosave.provider = { + save() { + spy(); + + savedStates.push( editor.getData() ); + } + }; + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); + editor.model.insertContent( new ModelText( 'bar' ), editor.model.document.selection ); + } ); + + return editor.destroy().then( () => { + expect( spy.callCount ).to.equal( 2 ); + expect( savedStates ).to.deep.equal( [ + '

foo

paragraph2

', + '

foo

bar

', + ] ); + } ); + } ); + + it( 'should work after editor\'s destroy with long server action time', () => { + const serverActionSpy = sinon.spy(); + const pendingActions = editor.plugins.get( PendingActions ); + + autosave.provider = { + save() { + return wait5ms() + .then( serverActionSpy ); + } + }; + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + return editor.destroy() + .then( () => { + expect( pendingActions.isPending ).to.be.true; + } ) + .then( wait5ms ) + .then( () => { + expect( pendingActions.isPending ).to.be.false; + } ); + } ); } ); function wait5ms() { From 9e031be74e46fb3603259564ea48bc8ff675e34d Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 18 May 2018 17:31:42 +0200 Subject: [PATCH 06/32] Filtered out ununseful batches from autosave. --- src/autosave.js | 32 +++++++++++++++++++++++++++++++- tests/autosave.js | 13 +++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/autosave.js b/src/autosave.js index ea0a9f3..d83b993 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -56,7 +56,7 @@ export default class Autosave extends Plugin { const doc = editor.model.document; const pendingActions = editor.plugins.get( PendingActions ); - this.listenTo( doc, 'change', this._throttledSave ); + this.listenTo( doc, 'change', this._saveIfDocumentChanged.bind( this ) ); // TODO: Docs // Flush on the editor's destroy listener with the highest priority to ensure that @@ -86,6 +86,26 @@ export default class Autosave extends Plugin { this._throttledSave.flush(); } + /** + * Filter out changes in document, that don't impact on the data (e.g. selection changes). + * Ends once it finds the operation that potentially changes document's data. + * + * @private + */ + _saveIfDocumentChanged( evt, batch ) { + for ( const delta of batch.deltas ) { + for ( const operation of delta.operations ) { + const name = operation.name; + + if ( !name || !isUserSelectionOperation( operation ) ) { + this._throttledSave(); + + return; + } + } + } + } + /** * If the provider is set and new document version exists, * `_save()` method creates a pending action and calls `provider.save()` method. @@ -125,3 +145,13 @@ export default class Autosave extends Plugin { * @name Provider#save * @type {Function} */ + +/** + * @private + */ +function isUserSelectionOperation( operation ) { + return ( + operation.name.startsWith( 'user:position' ) || + operation.name.startsWith( 'user:range' ) + ); +} diff --git a/tests/autosave.js b/tests/autosave.js index c5d0548..8d22b96 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -145,6 +145,19 @@ describe( 'Autosave', () => { } ); } ); + it( 'should filter out batches that don\'t change content', () => { + autosave.provider = { + save: sandbox.spy() + }; + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + } ); + + autosave._flush(); + sinon.assert.notCalled( autosave.provider.save ); + } ); + it( 'should flush remaining calls after editor\'s destroy', () => { const spy = sandbox.spy(); const savedStates = []; From 0630a10f841e8c0bed7779457e3403908945aea2 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 18 May 2018 18:25:11 +0200 Subject: [PATCH 07/32] Filtered out ununseful batches from autosave #2. --- tests/autosave.js | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/autosave.js b/tests/autosave.js index 8d22b96..c4f76cb 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -145,7 +145,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should filter out batches that don\'t change content', () => { + it( 'should filter out change batches that don\'t change content', () => { autosave.provider = { save: sandbox.spy() }; @@ -158,6 +158,34 @@ describe( 'Autosave', () => { sinon.assert.notCalled( autosave.provider.save ); } ); + it( 'should filter out change batches that don\'t change content #2', () => { + autosave.provider = { + save: sandbox.spy() + }; + + const operation = { name: 'user:position' }; + editor.model.document.fire( 'change', { + deltas: [ { operations: [ operation ] } ] + } ); + + autosave._flush(); + sinon.assert.notCalled( autosave.provider.save ); + } ); + + it( 'should filter out change batches that don\'t change content #3', () => { + autosave.provider = { + save: sandbox.spy() + }; + + const operation = { name: 'user:range' }; + editor.model.document.fire( 'change', { + deltas: [ { operations: [ operation ] } ] + } ); + + autosave._flush(); + sinon.assert.notCalled( autosave.provider.save ); + } ); + it( 'should flush remaining calls after editor\'s destroy', () => { const spy = sandbox.spy(); const savedStates = []; From 0bee399e36e225ee22ffa1b8ffae61ccc6b97afc Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 18 May 2018 18:40:24 +0200 Subject: [PATCH 08/32] Improved docs. --- src/autosave.js | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index d83b993..f3ff111 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -10,10 +10,30 @@ import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; /* globals window */ /** - * TODO: ... * Autosave plugin provides an easy-to-use API for getting sync with the editor's content. - * It watches `Document:change`, and `Window:beforeunload` events. - * At these moments, editor.getBody() and editor.getTitle() should work. + * It watches `Document:change`, and `Window:beforeunload` events and keep the save provider + * in sync with the editor's data. + * + * ClassicEditor + * .create( document.querySelector( '#editor' ), { + * plugins: [ ArticlePluginSet, Autosave ], + * toolbar: [ 'heading', '|', 'bold', 'italic', 'link', 'bulletedList', 'numberedList', 'blockQuote', 'undo', 'redo' ], + * image: { + * toolbar: [ 'imageStyle:full', 'imageStyle:side', '|', 'imageTextAlternative' ], + * } + * } ) + * .then( editor => { + * const autosave = editor.plugins.get( Autosave ); + * autosave.provider = { + * save() { + * const data = editor.getData(); + * + * // Note: saveEditorsContentToDatabase function should return a promise + * // to pending action or be sync. + * return saveEditorsContentToDatabase( data ); + * } + * }; + * } ); */ export default class Autosave extends Plugin { static get pluginName() { @@ -58,9 +78,8 @@ export default class Autosave extends Plugin { this.listenTo( doc, 'change', this._saveIfDocumentChanged.bind( this ) ); - // TODO: Docs // Flush on the editor's destroy listener with the highest priority to ensure that - // `editor.getBody` will be called before plugins are destroyed. + // `editor.getData()` will be called before plugins are destroyed. this.listenTo( editor, 'destroy', () => this._save(), { priority: 'highest' } ); // It's not possible to easy test it because karma uses `beforeunload` event @@ -87,8 +106,8 @@ export default class Autosave extends Plugin { } /** - * Filter out changes in document, that don't impact on the data (e.g. selection changes). - * Ends once it finds the operation that potentially changes document's data. + * Filters out changes in document, that don't impact on the data (e.g. selection changes). + * Quickly returns when finds an operation that potentially changes document's data. * * @private */ @@ -111,7 +130,7 @@ export default class Autosave extends Plugin { * `_save()` method creates a pending action and calls `provider.save()` method. * It waits for the result and then removes the created pending action. * - * @protected + * @private */ _save() { if ( !this.provider ) { From 9233ea738520f685208dbbca644061d9e3f50025 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 18 May 2018 18:58:02 +0200 Subject: [PATCH 09/32] Added 3 simple manual tests. --- tests/manual/autosave.html | 6 +++++ tests/manual/autosave.js | 45 ++++++++++++++++++++++++++++++++++++++ tests/manual/autosave.md | 5 +++++ 3 files changed, 56 insertions(+) create mode 100644 tests/manual/autosave.html create mode 100644 tests/manual/autosave.js create mode 100644 tests/manual/autosave.md diff --git a/tests/manual/autosave.html b/tests/manual/autosave.html new file mode 100644 index 0000000..d9f6cb4 --- /dev/null +++ b/tests/manual/autosave.html @@ -0,0 +1,6 @@ + + +
+

Heading 1

+

Paragraph

+
diff --git a/tests/manual/autosave.js b/tests/manual/autosave.js new file mode 100644 index 0000000..8fae361 --- /dev/null +++ b/tests/manual/autosave.js @@ -0,0 +1,45 @@ +/** + * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +/* globals document, window, console */ + +import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor'; + +import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset'; +import Autosave from '../../src/autosave'; + +ClassicEditor + .create( document.querySelector( '#editor' ), { + plugins: [ ArticlePluginSet, Autosave ], + toolbar: [ 'heading', '|', 'bold', 'italic', 'link', 'bulletedList', 'numberedList', 'blockQuote', 'undo', 'redo' ], + image: { + toolbar: [ 'imageStyle:full', 'imageStyle:side', '|', 'imageTextAlternative' ], + } + } ) + .then( editor => { + window.editor = editor; + + const destroyButton = document.getElementById( 'destroy-editor-button' ); + destroyButton.addEventListener( 'click', () => editor.destroy() ); + + const autosave = editor.plugins.get( Autosave ); + autosave.provider = { + save() { + const data = editor.getData(); + + return saveEditorContentToDatabase( data ); + } + }; + } ); + +function saveEditorContentToDatabase( data ) { + return new Promise( res => { + window.setTimeout( () => { + console.log( data ); + + res(); + }, 1000 ); + } ); +} diff --git a/tests/manual/autosave.md b/tests/manual/autosave.md new file mode 100644 index 0000000..3a4cdf3 --- /dev/null +++ b/tests/manual/autosave.md @@ -0,0 +1,5 @@ +1. Play with the editor. You should logs of the editor's data in console with 1s timeout (it simulates the back-end). You should not see logs when you changes the selection. + +1. Type something and quickly try to reload the page. You should see something like this: `Reload site? Changes that you made may not be saved.`. + +1. Type something and quickly and click the `destroy editor` button. Most recent changes should be logged to the console. From 6cdece715642c780b839077de32a8b58c573abc1 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 18 May 2018 19:07:20 +0200 Subject: [PATCH 10/32] Added missing dependency to package.json. --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index 3c0047b..9f1a72b 100644 --- a/package.json +++ b/package.json @@ -15,6 +15,7 @@ "devDependencies": { "@ckeditor/ckeditor5-engine": "^10.0.0", "@ckeditor/ckeditor5-paragraph": "^10.0.0", + "@ckeditor/ckeditor5-editor-classic": "^10.0.0", "eslint": "^4.15.0", "eslint-config-ckeditor5": "^1.0.7", "husky": "^0.14.3", From 9312fde8bdebcb2c1ae904294dbc2a6cc7cba4db Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 21 May 2018 13:45:14 +0200 Subject: [PATCH 11/32] Improved docs. --- src/autosave.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index f3ff111..eed1b6c 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -10,9 +10,8 @@ import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; /* globals window */ /** - * Autosave plugin provides an easy-to-use API for getting sync with the editor's content. - * It watches `Document:change`, and `Window:beforeunload` events and keep the save provider - * in sync with the editor's data. + * Autosave plugin provides an easy-to-use API to save the editor's content. + * It watches `Document:change`, and `Window:beforeunload` events and calls the provider's save method. * * ClassicEditor * .create( document.querySelector( '#editor' ), { From 2b565e9f8f310ba07476c95f228891ffcaed303d Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 1 Jun 2018 20:41:09 +0200 Subject: [PATCH 12/32] Added custom implementation of throttle to support advanced support for pending actions. --- src/autosave.js | 173 ++++++++++++++++++++++++++++++++++------------ tests/autosave.js | 35 +++++++--- 2 files changed, 151 insertions(+), 57 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index eed1b6c..83c79ad 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -4,7 +4,6 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import PendingActions from '@ckeditor/ckeditor5-core/src/pendingactions'; -import throttle from '@ckeditor/ckeditor5-utils/src/lib/lodash/throttle'; import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; /* globals window */ @@ -47,7 +46,7 @@ export default class Autosave extends Plugin { super( editor ); /** - * @member {Provider} + * @member {module:autosave/autosave~SaveProvider} */ this.provider = undefined; @@ -57,6 +56,12 @@ export default class Autosave extends Plugin { */ this._throttledSave = throttle( this._save.bind( this ), 500 ); + /** + * @protected + * @type {Number} + */ + this._lastDocumentVersion = editor.model.document.version; + /** * @private * @type {DomEmitterMixin} @@ -64,10 +69,16 @@ export default class Autosave extends Plugin { this._domEmitter = Object.create( DomEmitterMixin ); /** - * @protected + * @private * @type {Number} */ - this._lastDocumentVersion = editor.model.document.version; + this._saveActionCounter = 0; + + /** + * @private + * @type {Object|null} + */ + this._action = null; } init() { @@ -75,11 +86,19 @@ export default class Autosave extends Plugin { const doc = editor.model.document; const pendingActions = editor.plugins.get( PendingActions ); - this.listenTo( doc, 'change', this._saveIfDocumentChanged.bind( this ) ); + this.listenTo( doc, 'change:data', () => { + this._addAction(); + + const isCancelled = this._throttledSave(); + + if ( isCancelled ) { + this._removeAction(); + } + } ); // Flush on the editor's destroy listener with the highest priority to ensure that // `editor.getData()` will be called before plugins are destroyed. - this.listenTo( editor, 'destroy', () => this._save(), { priority: 'highest' } ); + this.listenTo( editor, 'destroy', () => this._flush(), { priority: 'highest' } ); // It's not possible to easy test it because karma uses `beforeunload` event // to warn before full page reload and this event cannot be dispatched manually. @@ -92,7 +111,11 @@ export default class Autosave extends Plugin { } destroy() { - this._throttledSave.cancel(); + const isCanceled = this._throttledSave.cancel(); + if ( isCanceled ) { + this._removeAction(); + } + this._domEmitter.stopListening(); super.destroy(); } @@ -104,26 +127,6 @@ export default class Autosave extends Plugin { this._throttledSave.flush(); } - /** - * Filters out changes in document, that don't impact on the data (e.g. selection changes). - * Quickly returns when finds an operation that potentially changes document's data. - * - * @private - */ - _saveIfDocumentChanged( evt, batch ) { - for ( const delta of batch.deltas ) { - for ( const operation of delta.operations ) { - const name = operation.name; - - if ( !name || !isUserSelectionOperation( operation ) ) { - this._throttledSave(); - - return; - } - } - } - } - /** * If the provider is set and new document version exists, * `_save()` method creates a pending action and calls `provider.save()` method. @@ -132,44 +135,122 @@ export default class Autosave extends Plugin { * @private */ _save() { - if ( !this.provider ) { - return; - } - const version = this.editor.model.document.version; - if ( version <= this._lastDocumentVersion ) { + if ( !this.provider || version <= this._lastDocumentVersion ) { + this._removeAction(); + return; } this._lastDocumentVersion = version; - const pendingActions = this.editor.plugins.get( PendingActions ); - const action = pendingActions.add( 'Saving in progress.' ); - Promise.resolve( this.provider.save() ) .then( () => { - pendingActions.remove( action ); + this._removeAction(); } ); } + + /** + * @private + */ + _addAction() { + this._saveActionCounter++; + + if ( !this.action ) { + const pendingActions = this.editor.plugins.get( PendingActions ); + this.action = pendingActions.add( 'Saving in progress.' ); + } + } + + /** + * @private + */ + _removeAction() { + this._saveActionCounter--; + + if ( this._saveActionCounter === 0 ) { + const pendingActions = this.editor.plugins.get( PendingActions ); + pendingActions.remove( this.action ); + this.action = null; + } + } } /** - * @typedef Provider + * @interface module:autosave/autosave~SaveProvider */ /** - * @function - * @name Provider#save - * @type {Function} + * Method that will be called when the data model changes. + * + * @method #save + * @returns {Promise.<*>|undefined} */ /** - * @private + * @param {Number} time */ -function isUserSelectionOperation( operation ) { - return ( - operation.name.startsWith( 'user:position' ) || - operation.name.startsWith( 'user:range' ) - ); +function throttle( fn, time ) { + let lastCall = 0; + let scheduledCall = false; + let callId = 0; + + // @returns {Boolean} `true` if the call is canceled. + function throttledFn() { + const now = Date.now(); + + // Call instantly, as the fn wasn't called within the `time` period. + if ( now > lastCall + time ) { + call( callId ); + return false; + } + + // Cancel call, as the next call is scheduled. + if ( scheduledCall ) { + return true; + } + + // Set timeout, so the fn will be called `time` ms after the last call. + scheduledCall = true; + window.setTimeout( call, lastCall + time - now, callId ); + + return false; + } + + throttledFn.cancel = cancel; + throttledFn.flush = flush; + + // @returns {Boolean} `true` if the call is canceled. + function cancel() { + const wasScheduledCall = scheduledCall; + scheduledCall = false; + lastCall = 0; + callId++; + + return wasScheduledCall; + } + + function flush() { + if ( scheduledCall ) { + call( callId ); + } + + scheduledCall = false; + lastCall = 0; + callId++; + } + + function call( id ) { + if ( id !== callId ) { + return; + } + + lastCall = Date.now(); + scheduledCall = false; + + fn(); + } + + return throttledFn; } diff --git a/tests/autosave.js b/tests/autosave.js index c4f76cb..d0c9bcc 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -31,7 +31,7 @@ describe( 'Autosave', () => { autosave = editor.plugins.get( Autosave ); // Clean autosave's state after setting data. - autosave._throttledSave.cancel(); + autosave._flush(); } ); } ); @@ -145,39 +145,52 @@ describe( 'Autosave', () => { } ); } ); - it( 'should filter out change batches that don\'t change content', () => { + it( 'should add a pending action during the saving #2.', () => { + const serverActionSpy = sinon.spy(); + const pendingActions = editor.plugins.get( PendingActions ); + autosave.provider = { - save: sandbox.spy() + save() { + serverActionSpy(); + } }; + expect( pendingActions.isPending ).to.be.false; + editor.model.change( writer => { writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); + expect( pendingActions.isPending ).to.be.true; + autosave._flush(); - sinon.assert.notCalled( autosave.provider.save ); + + // Server action needs to wait at least a cycle. + return Promise.resolve().then( () => { + expect( pendingActions.isPending ).to.be.false; + } ); } ); - it( 'should filter out change batches that don\'t change content #2', () => { + it( 'should filter out change batches that don\'t change content', () => { autosave.provider = { save: sandbox.spy() }; - const operation = { name: 'user:position' }; - editor.model.document.fire( 'change', { - deltas: [ { operations: [ operation ] } ] + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); } ); autosave._flush(); sinon.assert.notCalled( autosave.provider.save ); } ); - it( 'should filter out change batches that don\'t change content #3', () => { + it.skip( 'should filter out change batches that don\'t change content #2', () => { autosave.provider = { save: sandbox.spy() }; - const operation = { name: 'user:range' }; + const operation = { name: 'user:position' }; editor.model.document.fire( 'change', { deltas: [ { operations: [ operation ] } ] } ); @@ -217,7 +230,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should work after editor\'s destroy with long server action time', () => { + it( 'should work after editor\'s destroy with long server\'s action time', () => { const serverActionSpy = sinon.spy(); const pendingActions = editor.plugins.get( PendingActions ); From f7f2fef58b1960efc202c0b229390416bee71eee Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 6 Jun 2018 13:22:48 +0200 Subject: [PATCH 13/32] Improving API docs and details in the implementation. --- src/autosave.js | 53 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 83c79ad..bd56d90 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -2,6 +2,10 @@ * Copyright (c) 2016 - 2017, CKSource - Frederico Knabben. All rights reserved. */ +/** + * @module autosave/autosave + */ + import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import PendingActions from '@ckeditor/ckeditor5-core/src/pendingactions'; import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; @@ -34,10 +38,16 @@ import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; * } ); */ export default class Autosave extends Plugin { + /** + * @inheritDoc + */ static get pluginName() { return 'Autosave'; } + /** + * @inheritDoc + */ static get requires() { return [ PendingActions ]; } @@ -46,7 +56,7 @@ export default class Autosave extends Plugin { super( editor ); /** - * @member {module:autosave/autosave~SaveProvider} + * @type {module:autosave/autosave~SaveProvider} */ this.provider = undefined; @@ -89,9 +99,9 @@ export default class Autosave extends Plugin { this.listenTo( doc, 'change:data', () => { this._addAction(); - const isCancelled = this._throttledSave(); + const willOriginalFunctionBeCalled = this._throttledSave(); - if ( isCancelled ) { + if ( !willOriginalFunctionBeCalled ) { this._removeAction(); } } ); @@ -111,8 +121,8 @@ export default class Autosave extends Plugin { } destroy() { - const isCanceled = this._throttledSave.cancel(); - if ( isCanceled ) { + const wasPendingCallCanceled = this._throttledSave.cancel(); + if ( wasPendingCallCanceled ) { this._removeAction(); } @@ -189,20 +199,26 @@ export default class Autosave extends Plugin { */ /** - * @param {Number} time + * Throttle function helper that provides ability to specify minimum time gap between calling original function. + * Comparing to the lodash implementation, it supports getting an information if calling the throttled function will result in + * calling the original function and whether canceling throttling will cancel some pending call. + * + * @private + * @param {Function} fn Original function that will be called. + * @param {Number} time Amount of time between calling the original function. */ function throttle( fn, time ) { - let lastCall = 0; + let lastCallTime = 0; let scheduledCall = false; let callId = 0; - // @returns {Boolean} `true` if the call is canceled. + // @returns {Boolean} `true` if the original function was or will be called. function throttledFn() { const now = Date.now(); // Call instantly, as the fn wasn't called within the `time` period. - if ( now > lastCall + time ) { - call( callId ); + if ( now > lastCallTime + time ) { + call(); return false; } @@ -213,7 +229,7 @@ function throttle( fn, time ) { // Set timeout, so the fn will be called `time` ms after the last call. scheduledCall = true; - window.setTimeout( call, lastCall + time - now, callId ); + window.setTimeout( call, lastCallTime + time - now, callId ); return false; } @@ -221,11 +237,11 @@ function throttle( fn, time ) { throttledFn.cancel = cancel; throttledFn.flush = flush; - // @returns {Boolean} `true` if the call is canceled. + // @returns {Boolean} `true` if some pending call was canceled. function cancel() { const wasScheduledCall = scheduledCall; scheduledCall = false; - lastCall = 0; + lastCallTime = 0; callId++; return wasScheduledCall; @@ -233,20 +249,23 @@ function throttle( fn, time ) { function flush() { if ( scheduledCall ) { - call( callId ); + call(); } scheduledCall = false; - lastCall = 0; + lastCallTime = 0; callId++; } - function call( id ) { + // Call the function if the callId variables wasn't increased in meantime. + // Increasing the `callId` means canceling the original call. + // @param {Number} [id] + function call( id = callId ) { if ( id !== callId ) { return; } - lastCall = Date.now(); + lastCallTime = Date.now(); scheduledCall = false; fn(); From 28c4ef8a0fdc80e296428df8e223436c98cc99a8 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 6 Jun 2018 13:30:31 +0200 Subject: [PATCH 14/32] API docs improvements. --- src/autosave.js | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index bd56d90..a9fa9b3 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -52,6 +52,9 @@ export default class Autosave extends Plugin { return [ PendingActions ]; } + /** + * @inheritDoc + */ constructor( editor ) { super( editor ); @@ -91,6 +94,9 @@ export default class Autosave extends Plugin { this._action = null; } + /** + * @inheritDoc + */ init() { const editor = this.editor; const doc = editor.model.document; @@ -120,6 +126,9 @@ export default class Autosave extends Plugin { } ); } + /** + * @inheritDoc + */ destroy() { const wasPendingCallCanceled = this._throttledSave.cancel(); if ( wasPendingCallCanceled ) { @@ -131,6 +140,8 @@ export default class Autosave extends Plugin { } /** + * Invokes remaining call (if exists) in the throttled save function. + * * @protected */ _flush() { @@ -199,9 +210,9 @@ export default class Autosave extends Plugin { */ /** - * Throttle function helper that provides ability to specify minimum time gap between calling original function. - * Comparing to the lodash implementation, it supports getting an information if calling the throttled function will result in - * calling the original function and whether canceling throttling will cancel some pending call. + * Throttle function - a helper that provides ability to specify minimum time gap between calling an original function. + * Comparing to the lodash implementation, this provides an information if calling the throttled function will result in + * calling the original function and whether canceling throttling will actually cancel some pending call. * * @private * @param {Function} fn Original function that will be called. @@ -219,19 +230,19 @@ function throttle( fn, time ) { // Call instantly, as the fn wasn't called within the `time` period. if ( now > lastCallTime + time ) { call(); - return false; + return true; } // Cancel call, as the next call is scheduled. if ( scheduledCall ) { - return true; + return false; } // Set timeout, so the fn will be called `time` ms after the last call. scheduledCall = true; window.setTimeout( call, lastCallTime + time - now, callId ); - return false; + return true; } throttledFn.cancel = cancel; From 08bfbfa87b51b8915476c283b697b9c26912633d Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 6 Jun 2018 14:44:56 +0200 Subject: [PATCH 15/32] Added more tests. fixed small bug. --- src/autosave.js | 4 +- tests/autosave.js | 105 +++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 103 insertions(+), 6 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index a9fa9b3..50c7354 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -158,7 +158,9 @@ export default class Autosave extends Plugin { _save() { const version = this.editor.model.document.version; - if ( !this.provider || version <= this._lastDocumentVersion ) { + // Marker's change may not produce an operation, so the document's version + // can be the same after that change. + if ( !this.provider || version < this._lastDocumentVersion ) { this._removeAction(); return; diff --git a/tests/autosave.js b/tests/autosave.js index d0c9bcc..c6638d8 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -172,7 +172,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should filter out change batches that don\'t change content', () => { + it( 'should filter out changes in the selection', () => { autosave.provider = { save: sandbox.spy() }; @@ -185,20 +185,115 @@ describe( 'Autosave', () => { sinon.assert.notCalled( autosave.provider.save ); } ); - it.skip( 'should filter out change batches that don\'t change content #2', () => { + it( 'should filter out markers that does not affect the data model', () => { autosave.provider = { save: sandbox.spy() }; - const operation = { name: 'user:position' }; - editor.model.document.fire( 'change', { - deltas: [ { operations: [ operation ] } ] + const range = ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ); + const range2 = ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ); + + editor.model.change( writer => { + writer.addMarker( 'name', { usingOperation: true, affectsData: false, range } ); + } ); + + autosave._flush(); + + editor.model.change( writer => { + writer.updateMarker( 'name', { range: range2 } ); } ); autosave._flush(); + sinon.assert.notCalled( autosave.provider.save ); } ); + it( 'should filter out markers that does not affect the data model #2', () => { + autosave.provider = { + save: sandbox.spy() + }; + + const range = ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ); + const range2 = ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ); + + editor.model.change( writer => { + writer.addMarker( 'name', { usingOperation: false, affectsData: false, range } ); + } ); + + autosave._flush(); + + editor.model.change( writer => { + writer.updateMarker( 'name', { range: range2 } ); + } ); + + autosave._flush(); + + sinon.assert.notCalled( autosave.provider.save ); + } ); + + it( 'should call the save method when some marker affects the data model', () => { + autosave.provider = { + save: sandbox.spy() + }; + + const range = ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ); + const range2 = ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ); + + editor.model.change( writer => { + writer.addMarker( 'name', { usingOperation: true, affectsData: true, range } ); + } ); + + autosave._flush(); + + editor.model.change( writer => { + writer.updateMarker( 'name', { range: range2 } ); + } ); + + autosave._flush(); + + sinon.assert.calledTwice( autosave.provider.save ); + } ); + + it( 'should call the save method when some marker affects the data model #2', () => { + autosave.provider = { + save: sandbox.spy() + }; + + const range = ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ); + const range2 = ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ); + + editor.model.change( writer => { + writer.addMarker( 'name', { usingOperation: false, affectsData: true, range } ); + } ); + + autosave._flush(); + sinon.assert.calledOnce( autosave.provider.save ); + + editor.model.change( writer => { + writer.updateMarker( 'name', { range: range2 } ); + } ); + + autosave._flush(); + + sinon.assert.calledTwice( autosave.provider.save ); + } ); + + it( 'should call the save method when some marker affects the data model #3', () => { + autosave.provider = { + save: sandbox.spy() + }; + + const range = ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ); + + editor.model.change( writer => { + writer.addMarker( 'marker-not-affecting-data', { usingOperation: false, affectsData: true, range } ); + writer.addMarker( 'marker-affecting-data', { usingOperation: false, affectsData: false, range } ); + } ); + + autosave._flush(); + sinon.assert.calledOnce( autosave.provider.save ); + } ); + it( 'should flush remaining calls after editor\'s destroy', () => { const spy = sandbox.spy(); const savedStates = []; From c75355ac680947ea6634cf64fe08650fe9722613 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 6 Jun 2018 16:36:18 +0200 Subject: [PATCH 16/32] API docs improvements. --- src/autosave.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 50c7354..3b88745 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -14,7 +14,8 @@ import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; /** * Autosave plugin provides an easy-to-use API to save the editor's content. - * It watches `Document:change`, and `Window:beforeunload` events and calls the provider's save method. + * It watches {module:engine/model/document~Document#event:change:data change:data} + * and `Window:beforeunload` events and calls the provider's save method. * * ClassicEditor * .create( document.querySelector( '#editor' ), { @@ -30,8 +31,7 @@ import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; * save() { * const data = editor.getData(); * - * // Note: saveEditorsContentToDatabase function should return a promise - * // to pending action or be sync. + * // Note: saveEditorsContentToDatabase function might be async and return a promise to the saving action. * return saveEditorsContentToDatabase( data ); * } * }; From d7c175a7c3a897a5ad737b66e63818e811ed55c1 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 6 Jun 2018 18:41:23 +0200 Subject: [PATCH 17/32] Removed `throttle.cancel`, fixed CC. --- src/autosave.js | 45 +++++++++++++-------------------------------- 1 file changed, 13 insertions(+), 32 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 3b88745..309fdf4 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -130,17 +130,14 @@ export default class Autosave extends Plugin { * @inheritDoc */ destroy() { - const wasPendingCallCanceled = this._throttledSave.cancel(); - if ( wasPendingCallCanceled ) { - this._removeAction(); - } + // There's no need for canceling or flushing the throttled save, as it's done on editor's destroy event with the highest priority. this._domEmitter.stopListening(); super.destroy(); } /** - * Invokes remaining call (if exists) in the throttled save function. + * Invokes remaining `_save` method call. * * @protected */ @@ -212,25 +209,27 @@ export default class Autosave extends Plugin { */ /** - * Throttle function - a helper that provides ability to specify minimum time gap between calling an original function. + * Throttle function - a helper that provides ability to specify minimum time gap between calling the original function. * Comparing to the lodash implementation, this provides an information if calling the throttled function will result in - * calling the original function and whether canceling throttling will actually cancel some pending call. + * calling the original function. * * @private * @param {Function} fn Original function that will be called. - * @param {Number} time Amount of time between calling the original function. + * @param {Number} wait Minimum amount of time between original function calls. */ -function throttle( fn, time ) { +function throttle( fn, wait ) { + // Time in ms of the last call. let lastCallTime = 0; + + // Specifies whether there is a pending call. let scheduledCall = false; - let callId = 0; // @returns {Boolean} `true` if the original function was or will be called. function throttledFn() { const now = Date.now(); // Call instantly, as the fn wasn't called within the `time` period. - if ( now > lastCallTime + time ) { + if ( now > lastCallTime + wait ) { call(); return true; } @@ -242,24 +241,13 @@ function throttle( fn, time ) { // Set timeout, so the fn will be called `time` ms after the last call. scheduledCall = true; - window.setTimeout( call, lastCallTime + time - now, callId ); + window.setTimeout( call, lastCallTime + wait - now ); return true; } - throttledFn.cancel = cancel; throttledFn.flush = flush; - // @returns {Boolean} `true` if some pending call was canceled. - function cancel() { - const wasScheduledCall = scheduledCall; - scheduledCall = false; - lastCallTime = 0; - callId++; - - return wasScheduledCall; - } - function flush() { if ( scheduledCall ) { call(); @@ -267,17 +255,10 @@ function throttle( fn, time ) { scheduledCall = false; lastCallTime = 0; - callId++; } - // Call the function if the callId variables wasn't increased in meantime. - // Increasing the `callId` means canceling the original call. - // @param {Number} [id] - function call( id = callId ) { - if ( id !== callId ) { - return; - } - + // Calls the original function and updates internals. + function call() { lastCallTime = Date.now(); scheduledCall = false; From 5d346a59968184e2130e6f7a7666f55abc43f68b Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 7 Jun 2018 11:12:04 +0200 Subject: [PATCH 18/32] Aligned code to changes in the engine. --- tests/autosave.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/autosave.js b/tests/autosave.js index c6638d8..84efaae 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -194,7 +194,7 @@ describe( 'Autosave', () => { const range2 = ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ); editor.model.change( writer => { - writer.addMarker( 'name', { usingOperation: true, affectsData: false, range } ); + writer.addMarker( 'name', { usingOperation: true, range } ); } ); autosave._flush(); @@ -217,7 +217,7 @@ describe( 'Autosave', () => { const range2 = ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ); editor.model.change( writer => { - writer.addMarker( 'name', { usingOperation: false, affectsData: false, range } ); + writer.addMarker( 'name', { usingOperation: false, range } ); } ); autosave._flush(); From e8a4cc997a01d66c67c7729c7c5272d8efdd54af Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 8 Jun 2018 12:12:05 +0200 Subject: [PATCH 19/32] Improved API docs, fixed variable name typo. --- src/autosave.js | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 309fdf4..4448cfc 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -59,17 +59,25 @@ export default class Autosave extends Plugin { super( editor ); /** + * Provider is an object with the `save()` method. That method will be called whenever + * the model's data changes. It might be called some time after the change, + * since the event is throttled for performance reasons. + * * @type {module:autosave/autosave~SaveProvider} */ this.provider = undefined; /** + * Throttled save method. + * * @protected * @type {Function} */ this._throttledSave = throttle( this._save.bind( this ), 500 ); /** + * Last document version. + * * @protected * @type {Number} */ @@ -82,12 +90,16 @@ export default class Autosave extends Plugin { this._domEmitter = Object.create( DomEmitterMixin ); /** + * Save action counter monitors number of actions. + * * @private * @type {Number} */ this._saveActionCounter = 0; /** + * An action that will be added to pending action manager for actions happening in that plugin. + * * @private * @type {Object|null} */ @@ -130,7 +142,8 @@ export default class Autosave extends Plugin { * @inheritDoc */ destroy() { - // There's no need for canceling or flushing the throttled save, as it's done on editor's destroy event with the highest priority. + // There's no need for canceling or flushing the throttled save, as + // it's done on the editor's destroy event with the highest priority. this._domEmitter.stopListening(); super.destroy(); @@ -177,9 +190,9 @@ export default class Autosave extends Plugin { _addAction() { this._saveActionCounter++; - if ( !this.action ) { + if ( !this._action ) { const pendingActions = this.editor.plugins.get( PendingActions ); - this.action = pendingActions.add( 'Saving in progress.' ); + this._action = pendingActions.add( 'Saving in progress.' ); } } @@ -191,8 +204,8 @@ export default class Autosave extends Plugin { if ( this._saveActionCounter === 0 ) { const pendingActions = this.editor.plugins.get( PendingActions ); - pendingActions.remove( this.action ); - this.action = null; + pendingActions.remove( this._action ); + this._action = null; } } } @@ -202,7 +215,8 @@ export default class Autosave extends Plugin { */ /** - * Method that will be called when the data model changes. + * Method that will be called when the data model changes. It might return a promise (e.g. in case of saving content to the database), + * so the `Autosave` plugin will wait for that action before removing it from the pending actions. * * @method #save * @returns {Promise.<*>|undefined} From f5c9f42ba63156a311fac138b90f79190112c7b0 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 8 Jun 2018 12:21:55 +0200 Subject: [PATCH 20/32] API docs fixes. --- src/autosave.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 4448cfc..cb108e8 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -14,8 +14,8 @@ import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; /** * Autosave plugin provides an easy-to-use API to save the editor's content. - * It watches {module:engine/model/document~Document#event:change:data change:data} - * and `Window:beforeunload` events and calls the provider's save method. + * It watches {@link module:engine/model/document~Document#event:change:data change:data} + * and `Window:beforeunload` events and calls the {@link module:autosave/autosave~SaveProvider#save} method. * * ClassicEditor * .create( document.querySelector( '#editor' ), { From ae38970c7657fec1456db1e054b81cd86bbbf391 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 13 Jun 2018 10:24:45 +0200 Subject: [PATCH 21/32] Changed method names. --- src/autosave.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index cb108e8..fc02b60 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -115,12 +115,12 @@ export default class Autosave extends Plugin { const pendingActions = editor.plugins.get( PendingActions ); this.listenTo( doc, 'change:data', () => { - this._addAction(); + this._incrementCounter(); const willOriginalFunctionBeCalled = this._throttledSave(); if ( !willOriginalFunctionBeCalled ) { - this._removeAction(); + this._decrementCounter(); } } ); @@ -171,7 +171,7 @@ export default class Autosave extends Plugin { // Marker's change may not produce an operation, so the document's version // can be the same after that change. if ( !this.provider || version < this._lastDocumentVersion ) { - this._removeAction(); + this._decrementCounter(); return; } @@ -180,14 +180,14 @@ export default class Autosave extends Plugin { Promise.resolve( this.provider.save() ) .then( () => { - this._removeAction(); + this._decrementCounter(); } ); } /** * @private */ - _addAction() { + _incrementCounter() { this._saveActionCounter++; if ( !this._action ) { @@ -199,7 +199,7 @@ export default class Autosave extends Plugin { /** * @private */ - _removeAction() { + _decrementCounter() { this._saveActionCounter--; if ( this._saveActionCounter === 0 ) { From c3dcd74943d8d900a4293d3cd5702c26a15c6243 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 13 Jun 2018 10:39:09 +0200 Subject: [PATCH 22/32] Added docs for private methods. --- src/autosave.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/autosave.js b/src/autosave.js index fc02b60..2204f02 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -185,6 +185,8 @@ export default class Autosave extends Plugin { } /** + * Increments counter and adds pending action if it not exists. + * * @private */ _incrementCounter() { @@ -197,6 +199,9 @@ export default class Autosave extends Plugin { } /** + * Decrements counter and removes pending action if counter is empty, + * which means, that no new save action occurred. + * * @private */ _decrementCounter() { From 5ad99086ef2ac675e08da3f253d7799c9a1325a4 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 13 Jun 2018 15:33:02 +0200 Subject: [PATCH 23/32] Added two more complex tests, added fake timers. --- tests/autosave.js | 124 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 103 insertions(+), 21 deletions(-) diff --git a/tests/autosave.js b/tests/autosave.js index 84efaae..05b147c 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -12,7 +12,7 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import PendingActions from '@ckeditor/ckeditor5-core/src/pendingactions'; describe( 'Autosave', () => { - const sandbox = sinon.sandbox.create(); + const sandbox = sinon.sandbox.create( { useFakeTimers: true } ); let editor, element, autosave; beforeEach( () => { @@ -118,14 +118,14 @@ describe( 'Autosave', () => { } ); it( 'should add a pending action during the saving.', () => { - const serverActionSpy = sinon.spy(); + sandbox.useFakeTimers(); const pendingActions = editor.plugins.get( PendingActions ); + const serverActionSpy = sinon.spy(); + const serverActionStub = sinon.stub(); + serverActionStub.onCall( 0 ).resolves( wait( 500 ).then( serverActionSpy ) ); autosave.provider = { - save() { - return wait5ms() - .then( serverActionSpy ); - } + save: serverActionStub }; editor.model.change( writer => { @@ -133,13 +133,12 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - autosave._flush(); - sinon.assert.notCalled( serverActionSpy ); expect( pendingActions.isPending ).to.be.true; expect( pendingActions.first.message ).to.equal( 'Saving in progress.' ); - return wait5ms().then( () => { + sandbox.clock.tick( 500 ); + return Promise.resolve().then( () => Promise.resolve() ).then( () => { sinon.assert.calledOnce( serverActionSpy ); expect( pendingActions.isPending ).to.be.false; } ); @@ -150,9 +149,37 @@ describe( 'Autosave', () => { const pendingActions = editor.plugins.get( PendingActions ); autosave.provider = { - save() { - serverActionSpy(); - } + save: serverActionSpy + }; + + expect( pendingActions.isPending ).to.be.false; + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + expect( pendingActions.isPending ).to.be.true; + + // Server action needs to wait at least a cycle. + return Promise.resolve().then( () => { + sinon.assert.calledOnce( serverActionSpy ); + expect( pendingActions.isPending ).to.be.false; + } ); + } ); + + it( 'should handle correctly throttled save action and preserve pending action until both save actions finish', () => { + sandbox.useFakeTimers(); + const serverActionSpy = sinon.spy(); + const pendingActions = editor.plugins.get( PendingActions ); + + // Create a fake server that responses after 500ms for the first call and after 1000ms for the second call. + const serverActionStub = sinon.stub(); + serverActionStub.onCall( 0 ).resolves( wait( 500 ).then( serverActionSpy ) ); + serverActionStub.onCall( 1 ).resolves( wait( 1000 ).then( serverActionSpy ) ); + + autosave.provider = { + save: serverActionStub }; expect( pendingActions.isPending ).to.be.false; @@ -164,11 +191,65 @@ describe( 'Autosave', () => { expect( pendingActions.isPending ).to.be.true; + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'bar' ), editor.model.document.selection ); + } ); + autosave._flush(); + expect( pendingActions.isPending ).to.be.true; + + sandbox.clock.tick( 500 ); + return Promise.resolve().then( () => { + expect( pendingActions.isPending ).to.be.true; + sinon.assert.calledOnce( serverActionSpy ); + + // Wait another 500ms for the second server action. + sandbox.clock.tick( 500 ); + return Promise.resolve().then( () => { + expect( pendingActions.isPending ).to.be.false; + sinon.assert.calledTwice( serverActionSpy ); + } ); + } ); + } ); + + it( 'should handle correctly throttled save action and preserve pending action until both save actions finish #2', () => { + const serverActionSpy = sinon.spy(); + const pendingActions = editor.plugins.get( PendingActions ); + + autosave.provider = { + save: serverActionSpy + }; + + expect( pendingActions.isPending ).to.be.false; + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + expect( pendingActions.isPending ).to.be.true; + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'bar' ), editor.model.document.selection ); + } ); + + expect( pendingActions.isPending ).to.be.true; + // Server action needs to wait at least a cycle. return Promise.resolve().then( () => { - expect( pendingActions.isPending ).to.be.false; + sinon.assert.calledOnce( serverActionSpy ); + expect( pendingActions.isPending ).to.be.true; + + autosave._flush(); + + // Wait another promise cycle. + return Promise.resolve().then( () => { + sinon.assert.calledTwice( serverActionSpy ); + expect( pendingActions.isPending ).to.be.false; + } ); } ); } ); @@ -326,14 +407,14 @@ describe( 'Autosave', () => { } ); it( 'should work after editor\'s destroy with long server\'s action time', () => { - const serverActionSpy = sinon.spy(); + sandbox.useFakeTimers(); const pendingActions = editor.plugins.get( PendingActions ); + const serverActionSpy = sinon.spy(); + const serverActionStub = sinon.stub(); + serverActionStub.onCall( 0 ).resolves( wait( 500 ).then( serverActionSpy ) ); autosave.provider = { - save() { - return wait5ms() - .then( serverActionSpy ); - } + save: serverActionStub }; editor.model.change( writer => { @@ -344,17 +425,18 @@ describe( 'Autosave', () => { return editor.destroy() .then( () => { expect( pendingActions.isPending ).to.be.true; + sandbox.clock.tick( 500 ); } ) - .then( wait5ms ) .then( () => { expect( pendingActions.isPending ).to.be.false; + sinon.assert.calledOnce( serverActionSpy ); } ); } ); } ); - function wait5ms() { + function wait( time ) { return new Promise( res => { - window.setTimeout( res, 5 ); + window.setTimeout( res, time ); } ); } } ); From 51a72b9a84294a9b4fff2b3a4b25db16e89313a2 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 13 Jun 2018 15:36:06 +0200 Subject: [PATCH 24/32] Improved tests. --- tests/autosave.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/autosave.js b/tests/autosave.js index 05b147c..8715878 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -205,12 +205,11 @@ describe( 'Autosave', () => { expect( pendingActions.isPending ).to.be.true; sinon.assert.calledOnce( serverActionSpy ); - // Wait another 500ms for the second server action. + // Wait another 500ms and a promise cycle for the second server action. sandbox.clock.tick( 500 ); - return Promise.resolve().then( () => { - expect( pendingActions.isPending ).to.be.false; - sinon.assert.calledTwice( serverActionSpy ); - } ); + } ).then( () => { + expect( pendingActions.isPending ).to.be.false; + sinon.assert.calledTwice( serverActionSpy ); } ); } ); From b1520da8fb353136ff0b336d9b38a7e872908372 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 14 Jun 2018 15:25:59 +0200 Subject: [PATCH 25/32] Fixed file headers, added pending actions caching. --- src/autosave.js | 23 +++++++++++++++-------- tests/autosave.js | 3 ++- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 2204f02..56c3f0b 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -1,5 +1,6 @@ /** - * Copyright (c) 2016 - 2017, CKSource - Frederico Knabben. All rights reserved. + * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. */ /** @@ -104,6 +105,13 @@ export default class Autosave extends Plugin { * @type {Object|null} */ this._action = null; + + /** + * Editor's pending actions manager. + * + * @private + * @member {@module:core/pendingactions~PendingActions} #_pendingActions + */ } /** @@ -112,7 +120,8 @@ export default class Autosave extends Plugin { init() { const editor = this.editor; const doc = editor.model.document; - const pendingActions = editor.plugins.get( PendingActions ); + + this._pendingActions = editor.plugins.get( PendingActions ); this.listenTo( doc, 'change:data', () => { this._incrementCounter(); @@ -132,8 +141,8 @@ export default class Autosave extends Plugin { // to warn before full page reload and this event cannot be dispatched manually. /* istanbul ignore next */ this._domEmitter.listenTo( window, 'beforeunload', ( evtInfo, domEvt ) => { - if ( pendingActions.isPending ) { - domEvt.returnValue = pendingActions.first.message; + if ( this._pendingActions.isPending ) { + domEvt.returnValue = this._pendingActions.first.message; } } ); } @@ -193,8 +202,7 @@ export default class Autosave extends Plugin { this._saveActionCounter++; if ( !this._action ) { - const pendingActions = this.editor.plugins.get( PendingActions ); - this._action = pendingActions.add( 'Saving in progress.' ); + this._action = this._pendingActions.add( 'Saving in progress.' ); } } @@ -208,8 +216,7 @@ export default class Autosave extends Plugin { this._saveActionCounter--; if ( this._saveActionCounter === 0 ) { - const pendingActions = this.editor.plugins.get( PendingActions ); - pendingActions.remove( this._action ); + this._pendingActions.remove( this._action ); this._action = null; } } diff --git a/tests/autosave.js b/tests/autosave.js index 8715878..2c92b62 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -1,5 +1,6 @@ /** - * Copyright (c) 2016 - 2017, CKSource - Frederico Knabben. All rights reserved. + * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. */ /* globals document, window */ From 07a3e19e19dfc1b32eec39aa2a58a722a3414f86 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 14 Jun 2018 15:37:33 +0200 Subject: [PATCH 26/32] Improved implementation of the throttle util. --- src/autosave.js | 16 ++++++++-------- tests/autosave.js | 4 +++- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 56c3f0b..cb92ee3 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -247,8 +247,8 @@ function throttle( fn, wait ) { // Time in ms of the last call. let lastCallTime = 0; - // Specifies whether there is a pending call. - let scheduledCall = false; + // Timeout id that enables stopping scheduled call. + let timeoutId = null; // @returns {Boolean} `true` if the original function was or will be called. function throttledFn() { @@ -261,13 +261,12 @@ function throttle( fn, wait ) { } // Cancel call, as the next call is scheduled. - if ( scheduledCall ) { + if ( timeoutId ) { return false; } // Set timeout, so the fn will be called `time` ms after the last call. - scheduledCall = true; - window.setTimeout( call, lastCallTime + wait - now ); + timeoutId = window.setTimeout( call, lastCallTime + wait - now ); return true; } @@ -275,18 +274,19 @@ function throttle( fn, wait ) { throttledFn.flush = flush; function flush() { - if ( scheduledCall ) { + if ( timeoutId ) { + window.clearTimeout( timeoutId ); + timeoutId = null; + call(); } - scheduledCall = false; lastCallTime = 0; } // Calls the original function and updates internals. function call() { lastCallTime = Date.now(); - scheduledCall = false; fn(); } diff --git a/tests/autosave.js b/tests/autosave.js index 2c92b62..032dd7b 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -406,7 +406,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should work after editor\'s destroy with long server\'s action time', () => { + it( 'should work after editor\'s destroy with long server\'s response time', () => { sandbox.useFakeTimers(); const pendingActions = editor.plugins.get( PendingActions ); const serverActionSpy = sinon.spy(); @@ -427,9 +427,11 @@ describe( 'Autosave', () => { expect( pendingActions.isPending ).to.be.true; sandbox.clock.tick( 500 ); } ) + .then( () => Promise.resolve() ) .then( () => { expect( pendingActions.isPending ).to.be.false; sinon.assert.calledOnce( serverActionSpy ); + sinon.assert.calledOnce( serverActionStub ); } ); } ); } ); From 935a675a066c821d5ba1e5a87ab152e1b799632b Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 14 Jun 2018 16:30:38 +0200 Subject: [PATCH 27/32] Added tests for the throttle, moved throttle to separate file. --- src/autosave.js | 61 +---------------- src/throttle.js | 68 +++++++++++++++++++ tests/autosave.js | 12 ++-- tests/throttle.js | 165 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 240 insertions(+), 66 deletions(-) create mode 100644 src/throttle.js create mode 100644 tests/throttle.js diff --git a/src/autosave.js b/src/autosave.js index cb92ee3..1647ac0 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -10,6 +10,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import PendingActions from '@ckeditor/ckeditor5-core/src/pendingactions'; import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; +import throttle from './throttle'; /* globals window */ @@ -233,63 +234,3 @@ export default class Autosave extends Plugin { * @method #save * @returns {Promise.<*>|undefined} */ - -/** - * Throttle function - a helper that provides ability to specify minimum time gap between calling the original function. - * Comparing to the lodash implementation, this provides an information if calling the throttled function will result in - * calling the original function. - * - * @private - * @param {Function} fn Original function that will be called. - * @param {Number} wait Minimum amount of time between original function calls. - */ -function throttle( fn, wait ) { - // Time in ms of the last call. - let lastCallTime = 0; - - // Timeout id that enables stopping scheduled call. - let timeoutId = null; - - // @returns {Boolean} `true` if the original function was or will be called. - function throttledFn() { - const now = Date.now(); - - // Call instantly, as the fn wasn't called within the `time` period. - if ( now > lastCallTime + wait ) { - call(); - return true; - } - - // Cancel call, as the next call is scheduled. - if ( timeoutId ) { - return false; - } - - // Set timeout, so the fn will be called `time` ms after the last call. - timeoutId = window.setTimeout( call, lastCallTime + wait - now ); - - return true; - } - - throttledFn.flush = flush; - - function flush() { - if ( timeoutId ) { - window.clearTimeout( timeoutId ); - timeoutId = null; - - call(); - } - - lastCallTime = 0; - } - - // Calls the original function and updates internals. - function call() { - lastCallTime = Date.now(); - - fn(); - } - - return throttledFn; -} diff --git a/src/throttle.js b/src/throttle.js new file mode 100644 index 0000000..36aaf6c --- /dev/null +++ b/src/throttle.js @@ -0,0 +1,68 @@ +/** + * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +/** + * @module autosave/throttle + */ + +/* globals window */ + +/** + * Throttle function - a helper that provides ability to specify minimum time gap between calling the original function. + * Comparing to the lodash implementation, this provides an information if calling the throttled function will result in + * calling the original function. + * + * @param {Function} fn Original function that will be called. + * @param {Number} wait Minimum amount of time between original function calls. + */ +export default function throttle( fn, wait ) { + // Time in ms of the last call. + let lastCallTime = 0; + + // Timeout id that enables stopping scheduled call. + let timeoutId = null; + + // @returns {Boolean} `true` if the original function was or will be called. + function throttledFn() { + const now = Date.now(); + + // Cancel call, as the next call is scheduled. + if ( timeoutId ) { + return false; + } + + // Call instantly, as the fn wasn't called within the `time` period. + if ( now > lastCallTime + wait ) { + call(); + return true; + } + + // Set timeout, so the fn will be called `time` ms after the last call. + timeoutId = window.setTimeout( call, lastCallTime + wait - now ); + + return true; + } + + throttledFn.flush = flush; + + function flush() { + if ( timeoutId ) { + window.clearTimeout( timeoutId ); + call(); + } + + lastCallTime = 0; + } + + // Calls the original function and updates internals. + function call() { + lastCallTime = Date.now(); + timeoutId = null; + + fn(); + } + + return throttledFn; +} diff --git a/tests/autosave.js b/tests/autosave.js index 032dd7b..4dfe1fd 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -435,10 +435,10 @@ describe( 'Autosave', () => { } ); } ); } ); - - function wait( time ) { - return new Promise( res => { - window.setTimeout( res, time ); - } ); - } } ); + +function wait( time ) { + return new Promise( res => { + window.setTimeout( res, time ); + } ); +} diff --git a/tests/throttle.js b/tests/throttle.js new file mode 100644 index 0000000..c0f5df6 --- /dev/null +++ b/tests/throttle.js @@ -0,0 +1,165 @@ +/** + * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +import throttle from '../src/throttle'; + +describe( 'throttle', () => { + const sandbox = sinon.sandbox.create( { + useFakeTimers: true + } ); + + beforeEach( () => { + sandbox.useFakeTimers( { now: 1000 } ); + } ); + + afterEach( () => { + sandbox.restore(); + } ); + + it( 'should run first call synchronously', () => { + const spy = sinon.spy(); + const throttledFn = throttle( spy, 100 ); + + throttledFn(); + + sinon.assert.calledOnce( spy ); + + sandbox.clock.runAll(); + sinon.assert.calledOnce( spy ); + } ); + + it( 'should run next calls at the specified maximum rate', () => { + const spy = sinon.spy(); + const throttledFn = throttle( spy, 100 ); + + throttledFn(); + throttledFn(); + + sinon.assert.calledOnce( spy ); + + sandbox.clock.tick( 99 ); + + sinon.assert.calledOnce( spy ); + + sandbox.clock.tick( 1 ); + + sinon.assert.calledTwice( spy ); + sandbox.clock.runAll(); + } ); + + it( 'should run next calls at the specified maximum rate', () => { + const spy = sinon.spy(); + const throttledFn = throttle( spy, 100 ); + + throttledFn(); + throttledFn(); + + sinon.assert.calledOnce( spy ); + + sandbox.clock.tick( 99 ); + + sinon.assert.calledOnce( spy ); + + sandbox.clock.tick( 1 ); + + sinon.assert.calledTwice( spy ); + + sandbox.clock.runAll(); + sinon.assert.calledTwice( spy ); + } ); + + it( 'should skip the call if another call is scheduled', () => { + const spy = sinon.spy(); + const throttledFn = throttle( spy, 100 ); + + const isFirstInvoked = throttledFn(); + const willSecondInvoke = throttledFn(); + const willThirdInvoke = throttledFn(); + + expect( isFirstInvoked ).to.be.true; + expect( willSecondInvoke ).to.be.true; + expect( willThirdInvoke ).to.be.false; + + sandbox.clock.runAll(); + sinon.assert.calledTwice( spy ); + } ); + + it( 'should call the next call after the specified amount of time', () => { + const spy = sinon.spy(); + const throttledFn = throttle( spy, 100 ); + + throttledFn(); + throttledFn(); + + sandbox.clock.tick( 50 ); + + sinon.assert.calledOnce( spy ); + + sandbox.clock.tick( 50 ); + + sinon.assert.calledTwice( spy ); + + throttledFn(); + + sandbox.clock.tick( 100 ); + + sinon.assert.calledThrice( spy ); + } ); + + describe( 'flush', () => { + it( 'should be provide as a method on the throttled function', () => { + const spy = sinon.spy(); + const throttledFn = throttle( spy, 100 ); + + expect( throttledFn.flush ).to.be.a( 'function' ); + } ); + + it( 'should enable calling the throttled call immediately', () => { + const spy = sinon.spy(); + const throttledFn = throttle( spy, 100 ); + + throttledFn(); + throttledFn(); + + sinon.assert.calledOnce( spy ); + + throttledFn.flush(); + sinon.assert.calledTwice( spy ); + + sandbox.clock.runAll(); + sinon.assert.calledTwice( spy ); + } ); + + it( 'should do nothing if there is no scheduled call', () => { + const spy = sinon.spy(); + const throttledFn = throttle( spy, 100 ); + + throttledFn(); + + sinon.assert.calledOnce( spy ); + + throttledFn.flush(); + sinon.assert.calledOnce( spy ); + + sandbox.clock.runAll(); + sinon.assert.calledOnce( spy ); + } ); + + it( 'should enable calling after the flushed call', () => { + const spy = sinon.spy(); + const throttledFn = throttle( spy, 100 ); + + throttledFn(); + throttledFn(); + throttledFn.flush(); + throttledFn(); + + sinon.assert.calledThrice( spy ); + + sandbox.clock.runAll(); + sinon.assert.calledThrice( spy ); + } ); + } ); +} ); From 835e7062e6486aabd2cfaf7804078ec6fc633741 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 14 Jun 2018 16:53:46 +0200 Subject: [PATCH 28/32] Improved docs. --- src/autosave.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 1647ac0..8f27db5 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -33,7 +33,7 @@ import throttle from './throttle'; * save() { * const data = editor.getData(); * - * // Note: saveEditorsContentToDatabase function might be async and return a promise to the saving action. + * // Note: saveEditorsContentToDatabase function should return a promise to the saving action. * return saveEditorsContentToDatabase( data ); * } * }; @@ -228,7 +228,7 @@ export default class Autosave extends Plugin { */ /** - * Method that will be called when the data model changes. It might return a promise (e.g. in case of saving content to the database), + * Method that will be called when the data model changes. It should return a promise (e.g. in case of saving content to the database), * so the `Autosave` plugin will wait for that action before removing it from the pending actions. * * @method #save From f35614556ecc8acc4c7db23fdd58c9a14523f1e9 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 14 Jun 2018 17:19:55 +0200 Subject: [PATCH 29/32] Changed property name: provider to adapter. --- src/autosave.js | 28 +++++++++++++--------- tests/autosave.js | 52 ++++++++++++++++++++-------------------- tests/manual/autosave.js | 2 +- 3 files changed, 44 insertions(+), 38 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 8f27db5..ec10420 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -17,7 +17,7 @@ import throttle from './throttle'; /** * Autosave plugin provides an easy-to-use API to save the editor's content. * It watches {@link module:engine/model/document~Document#event:change:data change:data} - * and `Window:beforeunload` events and calls the {@link module:autosave/autosave~SaveProvider#save} method. + * and `Window:beforeunload` events and calls the {@link module:autosave/autosave~Adapter#save} method. * * ClassicEditor * .create( document.querySelector( '#editor' ), { @@ -29,7 +29,7 @@ import throttle from './throttle'; * } ) * .then( editor => { * const autosave = editor.plugins.get( Autosave ); - * autosave.provider = { + * autosave.adapter = { * save() { * const data = editor.getData(); * @@ -61,13 +61,13 @@ export default class Autosave extends Plugin { super( editor ); /** - * Provider is an object with the `save()` method. That method will be called whenever + * The adapter is an object with the `save()` method. That method will be called whenever * the model's data changes. It might be called some time after the change, * since the event is throttled for performance reasons. * - * @type {module:autosave/autosave~SaveProvider} + * @type {module:autosave/autosave~Adapter} */ - this.provider = undefined; + this.adapter = undefined; /** * Throttled save method. @@ -86,6 +86,8 @@ export default class Autosave extends Plugin { this._lastDocumentVersion = editor.model.document.version; /** + * DOM emitter. + * * @private * @type {DomEmitterMixin} */ @@ -169,8 +171,8 @@ export default class Autosave extends Plugin { } /** - * If the provider is set and new document version exists, - * `_save()` method creates a pending action and calls `provider.save()` method. + * If the adapter is set and new document version exists, + * `_save()` method creates a pending action and calls `adapter.save()` method. * It waits for the result and then removes the created pending action. * * @private @@ -180,7 +182,7 @@ export default class Autosave extends Plugin { // Marker's change may not produce an operation, so the document's version // can be the same after that change. - if ( !this.provider || version < this._lastDocumentVersion ) { + if ( !this.adapter || version < this._lastDocumentVersion ) { this._decrementCounter(); return; @@ -188,7 +190,7 @@ export default class Autosave extends Plugin { this._lastDocumentVersion = version; - Promise.resolve( this.provider.save() ) + Promise.resolve( this.adapter.save() ) .then( () => { this._decrementCounter(); } ); @@ -224,12 +226,16 @@ export default class Autosave extends Plugin { } /** - * @interface module:autosave/autosave~SaveProvider + * An interface that requires the `save()` method. + * + * Used by {module:autosave/autosave~Autosave#adapter} + * + * @interface module:autosave/autosave~Adapter */ /** * Method that will be called when the data model changes. It should return a promise (e.g. in case of saving content to the database), - * so the `Autosave` plugin will wait for that action before removing it from the pending actions. + * so the `Autosave` plugin will wait for that action before removing it from pending actions. * * @method #save * @returns {Promise.<*>|undefined} diff --git a/tests/autosave.js b/tests/autosave.js index 4dfe1fd..b7d28f4 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -48,11 +48,11 @@ describe( 'Autosave', () => { } ); describe( 'initialization', () => { - it( 'should initialize provider with an undefined value', () => { - expect( autosave.provider ).to.be.undefined; + it( 'should initialize adapter with an undefined value', () => { + expect( autosave.adapter ).to.be.undefined; } ); - it( 'should allow plugin to work without any defined provider', () => { + it( 'should allow plugin to work without any defined adapter', () => { editor.model.change( writer => { writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); @@ -63,8 +63,8 @@ describe( 'Autosave', () => { } ); describe( 'autosaving', () => { - it( 'should run provider\'s save method when the editor\'s change event is fired', () => { - autosave.provider = { + it( 'should run adapter\'s save method when the editor\'s change event is fired', () => { + autosave.adapter = { save: sinon.spy() }; @@ -76,14 +76,14 @@ describe( 'Autosave', () => { // Go to the next cycle to because synchronization of CS documentVersion is async. autosave._flush(); - sinon.assert.calledOnce( autosave.provider.save ); + sinon.assert.calledOnce( autosave.adapter.save ); } ); it( 'should throttle editor\'s change event', () => { const spy = sinon.spy(); const savedStates = []; - autosave.provider = { + autosave.adapter = { save() { spy(); @@ -125,7 +125,7 @@ describe( 'Autosave', () => { const serverActionStub = sinon.stub(); serverActionStub.onCall( 0 ).resolves( wait( 500 ).then( serverActionSpy ) ); - autosave.provider = { + autosave.adapter = { save: serverActionStub }; @@ -149,7 +149,7 @@ describe( 'Autosave', () => { const serverActionSpy = sinon.spy(); const pendingActions = editor.plugins.get( PendingActions ); - autosave.provider = { + autosave.adapter = { save: serverActionSpy }; @@ -179,7 +179,7 @@ describe( 'Autosave', () => { serverActionStub.onCall( 0 ).resolves( wait( 500 ).then( serverActionSpy ) ); serverActionStub.onCall( 1 ).resolves( wait( 1000 ).then( serverActionSpy ) ); - autosave.provider = { + autosave.adapter = { save: serverActionStub }; @@ -218,7 +218,7 @@ describe( 'Autosave', () => { const serverActionSpy = sinon.spy(); const pendingActions = editor.plugins.get( PendingActions ); - autosave.provider = { + autosave.adapter = { save: serverActionSpy }; @@ -254,7 +254,7 @@ describe( 'Autosave', () => { } ); it( 'should filter out changes in the selection', () => { - autosave.provider = { + autosave.adapter = { save: sandbox.spy() }; @@ -263,11 +263,11 @@ describe( 'Autosave', () => { } ); autosave._flush(); - sinon.assert.notCalled( autosave.provider.save ); + sinon.assert.notCalled( autosave.adapter.save ); } ); it( 'should filter out markers that does not affect the data model', () => { - autosave.provider = { + autosave.adapter = { save: sandbox.spy() }; @@ -286,11 +286,11 @@ describe( 'Autosave', () => { autosave._flush(); - sinon.assert.notCalled( autosave.provider.save ); + sinon.assert.notCalled( autosave.adapter.save ); } ); it( 'should filter out markers that does not affect the data model #2', () => { - autosave.provider = { + autosave.adapter = { save: sandbox.spy() }; @@ -309,11 +309,11 @@ describe( 'Autosave', () => { autosave._flush(); - sinon.assert.notCalled( autosave.provider.save ); + sinon.assert.notCalled( autosave.adapter.save ); } ); it( 'should call the save method when some marker affects the data model', () => { - autosave.provider = { + autosave.adapter = { save: sandbox.spy() }; @@ -332,11 +332,11 @@ describe( 'Autosave', () => { autosave._flush(); - sinon.assert.calledTwice( autosave.provider.save ); + sinon.assert.calledTwice( autosave.adapter.save ); } ); it( 'should call the save method when some marker affects the data model #2', () => { - autosave.provider = { + autosave.adapter = { save: sandbox.spy() }; @@ -348,7 +348,7 @@ describe( 'Autosave', () => { } ); autosave._flush(); - sinon.assert.calledOnce( autosave.provider.save ); + sinon.assert.calledOnce( autosave.adapter.save ); editor.model.change( writer => { writer.updateMarker( 'name', { range: range2 } ); @@ -356,11 +356,11 @@ describe( 'Autosave', () => { autosave._flush(); - sinon.assert.calledTwice( autosave.provider.save ); + sinon.assert.calledTwice( autosave.adapter.save ); } ); it( 'should call the save method when some marker affects the data model #3', () => { - autosave.provider = { + autosave.adapter = { save: sandbox.spy() }; @@ -372,14 +372,14 @@ describe( 'Autosave', () => { } ); autosave._flush(); - sinon.assert.calledOnce( autosave.provider.save ); + sinon.assert.calledOnce( autosave.adapter.save ); } ); it( 'should flush remaining calls after editor\'s destroy', () => { const spy = sandbox.spy(); const savedStates = []; - autosave.provider = { + autosave.adapter = { save() { spy(); @@ -413,7 +413,7 @@ describe( 'Autosave', () => { const serverActionStub = sinon.stub(); serverActionStub.onCall( 0 ).resolves( wait( 500 ).then( serverActionSpy ) ); - autosave.provider = { + autosave.adapter = { save: serverActionStub }; diff --git a/tests/manual/autosave.js b/tests/manual/autosave.js index 8fae361..1b29ca6 100644 --- a/tests/manual/autosave.js +++ b/tests/manual/autosave.js @@ -25,7 +25,7 @@ ClassicEditor destroyButton.addEventListener( 'click', () => editor.destroy() ); const autosave = editor.plugins.get( Autosave ); - autosave.provider = { + autosave.adapter = { save() { const data = editor.getData(); From 120dc0038ed533851e6a949c6c8d9f69f1240b2f Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 14 Jun 2018 21:40:16 +0200 Subject: [PATCH 30/32] Removed duplicated test. --- tests/throttle.js | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/tests/throttle.js b/tests/throttle.js index c0f5df6..7c718d3 100644 --- a/tests/throttle.js +++ b/tests/throttle.js @@ -30,7 +30,7 @@ describe( 'throttle', () => { sinon.assert.calledOnce( spy ); } ); - it( 'should run next calls at the specified maximum rate', () => { + it( 'should run next calls after specified amount of time', () => { const spy = sinon.spy(); const throttledFn = throttle( spy, 100 ); @@ -46,28 +46,6 @@ describe( 'throttle', () => { sandbox.clock.tick( 1 ); sinon.assert.calledTwice( spy ); - sandbox.clock.runAll(); - } ); - - it( 'should run next calls at the specified maximum rate', () => { - const spy = sinon.spy(); - const throttledFn = throttle( spy, 100 ); - - throttledFn(); - throttledFn(); - - sinon.assert.calledOnce( spy ); - - sandbox.clock.tick( 99 ); - - sinon.assert.calledOnce( spy ); - - sandbox.clock.tick( 1 ); - - sinon.assert.calledTwice( spy ); - - sandbox.clock.runAll(); - sinon.assert.calledTwice( spy ); } ); it( 'should skip the call if another call is scheduled', () => { From 439f39660ab22b550f17af24d2879e5bae7f56ac Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 15 Jun 2018 13:29:40 +0200 Subject: [PATCH 31/32] Added support for the autosave config. --- src/autosave.js | 16 ++++- tests/autosave.js | 153 +++++++++++++++++++++++++++++++++++----------- 2 files changed, 132 insertions(+), 37 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index ec10420..a921962 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -109,6 +109,14 @@ export default class Autosave extends Plugin { */ this._action = null; + /** + * Plugins' config. + * + * @private + * @type {Object} + */ + this._config = editor.config.get( 'autosave' ) || {}; + /** * Editor's pending actions manager. * @@ -180,9 +188,13 @@ export default class Autosave extends Plugin { _save() { const version = this.editor.model.document.version; + // Get defined callbacks. + const saveCallbacks = [ this.adapter && this.adapter.save, this._config.save ] + .filter( cb => !!cb ); + // Marker's change may not produce an operation, so the document's version // can be the same after that change. - if ( !this.adapter || version < this._lastDocumentVersion ) { + if ( version < this._lastDocumentVersion || !saveCallbacks.length ) { this._decrementCounter(); return; @@ -190,7 +202,7 @@ export default class Autosave extends Plugin { this._lastDocumentVersion = version; - Promise.resolve( this.adapter.save() ) + Promise.all( saveCallbacks.map( cb => cb() ) ) .then( () => { this._decrementCounter(); } ); diff --git a/tests/autosave.js b/tests/autosave.js index b7d28f4..f642ce1 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -16,31 +16,8 @@ describe( 'Autosave', () => { const sandbox = sinon.sandbox.create( { useFakeTimers: true } ); let editor, element, autosave; - beforeEach( () => { - element = document.createElement( 'div' ); - document.body.appendChild( element ); - - return ClassicTestEditor - .create( element, { - plugins: [ Autosave, Paragraph ] - } ) - .then( _editor => { - const data = '

paragraph1

paragraph2

'; - - editor = _editor; - editor.setData( data ); - autosave = editor.plugins.get( Autosave ); - - // Clean autosave's state after setting data. - autosave._flush(); - } ); - } ); - afterEach( () => { - document.body.removeChild( element ); sandbox.restore(); - - return editor.destroy(); } ); it( 'should have static pluginName property', () => { @@ -48,11 +25,37 @@ describe( 'Autosave', () => { } ); describe( 'initialization', () => { + beforeEach( () => { + element = document.createElement( 'div' ); + document.body.appendChild( element ); + + return ClassicTestEditor + .create( element, { + plugins: [ Autosave, Paragraph ] + } ) + .then( _editor => { + const data = '

paragraph1

paragraph2

'; + + editor = _editor; + editor.setData( data ); + autosave = editor.plugins.get( Autosave ); + + // Clean autosave's state after setting data. + autosave._flush(); + } ); + } ); + + afterEach( () => { + document.body.removeChild( element ); + + return editor.destroy(); + } ); + it( 'should initialize adapter with an undefined value', () => { expect( autosave.adapter ).to.be.undefined; } ); - it( 'should allow plugin to work without any defined adapter', () => { + it( 'should allow plugin to work without defined adapter and without its config', () => { editor.model.change( writer => { writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); @@ -62,7 +65,88 @@ describe( 'Autosave', () => { } ); } ); + describe( 'config', () => { + beforeEach( () => { + element = document.createElement( 'div' ); + document.body.appendChild( element ); + + return ClassicTestEditor + .create( element, { + plugins: [ Autosave, Paragraph ], + autosave: { + save: sinon.spy() + } + } ) + .then( _editor => { + const data = '

paragraph1

paragraph2

'; + + editor = _editor; + editor.setData( data ); + autosave = editor.plugins.get( Autosave ); + + // Clean autosave's state after setting data. + autosave._flush(); + editor.config.get( 'autosave' ).save.resetHistory(); + } ); + } ); + + afterEach( () => { + document.body.removeChild( element ); + + return editor.destroy(); + } ); + + it( 'should enable providing callback via the config', () => { + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + sinon.assert.calledOnce( editor.config.get( 'autosave' ).save ); + } ); + + it( 'its callback and adapter callback should be called if both are provided', () => { + autosave.adapter = { + save: sinon.spy() + }; + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + sinon.assert.calledOnce( editor.config.get( 'autosave' ).save ); + sinon.assert.calledOnce( autosave.adapter.save ); + } ); + } ); + describe( 'autosaving', () => { + beforeEach( () => { + element = document.createElement( 'div' ); + document.body.appendChild( element ); + + return ClassicTestEditor + .create( element, { + plugins: [ Autosave, Paragraph ] + } ) + .then( _editor => { + const data = '

paragraph1

paragraph2

'; + + editor = _editor; + editor.setData( data ); + autosave = editor.plugins.get( Autosave ); + + // Clean autosave's state after setting data. + autosave._flush(); + } ); + } ); + + afterEach( () => { + document.body.removeChild( element ); + + return editor.destroy(); + } ); + it( 'should run adapter\'s save method when the editor\'s change event is fired', () => { autosave.adapter = { save: sinon.spy() @@ -73,9 +157,6 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - // Go to the next cycle to because synchronization of CS documentVersion is async. - autosave._flush(); - sinon.assert.calledOnce( autosave.adapter.save ); } ); @@ -163,7 +244,7 @@ describe( 'Autosave', () => { expect( pendingActions.isPending ).to.be.true; // Server action needs to wait at least a cycle. - return Promise.resolve().then( () => { + return wait().then( () => { sinon.assert.calledOnce( serverActionSpy ); expect( pendingActions.isPending ).to.be.false; } ); @@ -202,16 +283,19 @@ describe( 'Autosave', () => { expect( pendingActions.isPending ).to.be.true; sandbox.clock.tick( 500 ); + return Promise.resolve().then( () => { expect( pendingActions.isPending ).to.be.true; sinon.assert.calledOnce( serverActionSpy ); // Wait another 500ms and a promise cycle for the second server action. sandbox.clock.tick( 500 ); - } ).then( () => { - expect( pendingActions.isPending ).to.be.false; - sinon.assert.calledTwice( serverActionSpy ); - } ); + } ) + .then( () => Promise.resolve() ) + .then( () => { + expect( pendingActions.isPending ).to.be.false; + sinon.assert.calledTwice( serverActionSpy ); + } ); } ); it( 'should handle correctly throttled save action and preserve pending action until both save actions finish #2', () => { @@ -239,14 +323,14 @@ describe( 'Autosave', () => { expect( pendingActions.isPending ).to.be.true; // Server action needs to wait at least a cycle. - return Promise.resolve().then( () => { + return wait().then( () => { sinon.assert.calledOnce( serverActionSpy ); expect( pendingActions.isPending ).to.be.true; autosave._flush(); // Wait another promise cycle. - return Promise.resolve().then( () => { + return wait().then( () => { sinon.assert.calledTwice( serverActionSpy ); expect( pendingActions.isPending ).to.be.false; } ); @@ -371,7 +455,6 @@ describe( 'Autosave', () => { writer.addMarker( 'marker-affecting-data', { usingOperation: false, affectsData: false, range } ); } ); - autosave._flush(); sinon.assert.calledOnce( autosave.adapter.save ); } ); From eeab88a1954211be13221d57e4e6fbbf7184abc3 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 15 Jun 2018 15:55:46 +0200 Subject: [PATCH 32/32] Improved code style and API docs. --- src/autosave.js | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index a921962..91a6c96 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -17,7 +17,7 @@ import throttle from './throttle'; /** * Autosave plugin provides an easy-to-use API to save the editor's content. * It watches {@link module:engine/model/document~Document#event:change:data change:data} - * and `Window:beforeunload` events and calls the {@link module:autosave/autosave~Adapter#save} method. + * and `window#beforeunload` events and calls the {@link module:autosave/autosave~Adapter#save} method. * * ClassicEditor * .create( document.querySelector( '#editor' ), { @@ -25,18 +25,14 @@ import throttle from './throttle'; * toolbar: [ 'heading', '|', 'bold', 'italic', 'link', 'bulletedList', 'numberedList', 'blockQuote', 'undo', 'redo' ], * image: { * toolbar: [ 'imageStyle:full', 'imageStyle:side', '|', 'imageTextAlternative' ], - * } - * } ) - * .then( editor => { - * const autosave = editor.plugins.get( Autosave ); - * autosave.adapter = { + * }, + * autosave: { * save() { - * const data = editor.getData(); - * - * // Note: saveEditorsContentToDatabase function should return a promise to the saving action. + * // Note: saveEditorsContentToDatabase function should return a promise + * // which should be resolved when the saving action is complete. * return saveEditorsContentToDatabase( data ); * } - * }; + * } * } ); */ export default class Autosave extends Plugin { @@ -188,9 +184,15 @@ export default class Autosave extends Plugin { _save() { const version = this.editor.model.document.version; - // Get defined callbacks. - const saveCallbacks = [ this.adapter && this.adapter.save, this._config.save ] - .filter( cb => !!cb ); + const saveCallbacks = []; + + if ( this.adapter && this.adapter.save ) { + saveCallbacks.push( this.adapter.save ); + } + + if ( this._config.save ) { + saveCallbacks.push( this._config.save ); + } // Marker's change may not produce an operation, so the document's version // can be the same after that change.