From d0fe7f0b782b997d2a0b2e5df7daacdaba4490c9 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 27 Jul 2018 20:24:16 +0200 Subject: [PATCH 01/16] Implemented new version of the Autosave mechanism. --- src/autosave.js | 99 +++++++++++++++++++++++++------------------------ 1 file changed, 50 insertions(+), 49 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 346fa28..17e9e5c 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -10,7 +10,9 @@ 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'; +import debounce from '@ckeditor/ckeditor5-utils/src/lib/lodash/debounce'; +import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin'; +import mix from '@ckeditor/ckeditor5-utils/src/mix'; /* globals window */ @@ -76,7 +78,7 @@ export default class Autosave extends Plugin { * @protected * @type {Function} */ - this._throttledSave = throttle( this._save.bind( this ), 1000 ); + this._debouncedSave = debounce( this._save.bind( this ), 2000 ); /** * Last document version. @@ -95,12 +97,12 @@ export default class Autosave extends Plugin { this._domEmitter = Object.create( DomEmitterMixin ); /** - * Save action counter monitors number of actions. + * The state of this plugin. * * @private - * @type {Number} + * @type {'initial'|'debounced-saving'|'external-saving'} */ - this._saveActionCounter = 0; + this.set( '_state', 'initial' ); /** * An action that will be added to pending action manager for actions happening in that plugin. @@ -110,7 +112,7 @@ export default class Autosave extends Plugin { */ /** - * Plugins' config. + * The config of this plugins. * * @private * @type {Object} @@ -134,14 +136,25 @@ export default class Autosave extends Plugin { this._pendingActions = editor.plugins.get( PendingActions ); + // this._state = 'initial'; + this.listenTo( doc, 'change:data', () => { - this._incrementCounter(); + if ( !this._saveCallbacks.length ) { + return; + } + + if ( this._state == 'initial' ) { + this._action = this._pendingActions.add( this.editor.t( 'Saving changes' ) ); + this._state = 'debounced-saving'; - const willOriginalFunctionBeCalled = this._throttledSave(); + this._debouncedSave(); + } - if ( !willOriginalFunctionBeCalled ) { - this._decrementCounter(); + else if ( this._state == 'debounced-saving' ) { + this._debouncedSave(); } + + // Do nothing if the plugin is in `external-saving` state. } ); // Flush on the editor's destroy listener with the highest priority to ensure that @@ -175,7 +188,7 @@ export default class Autosave extends Plugin { * @protected */ _flush() { - this._throttledSave.flush(); + this._debouncedSave.flush(); } /** @@ -188,74 +201,62 @@ export default class Autosave extends Plugin { _save() { const version = this.editor.model.document.version; - const saveCallbacks = []; - - if ( this.adapter && this.adapter.save ) { - saveCallbacks.push( this.adapter.save ); - } - - if ( this._config.save ) { - saveCallbacks.push( this._config.save ); - } - // Change may not produce an operation, so the document's version // can be the same after that change. if ( version < this._lastDocumentVersion || - !saveCallbacks.length || this.editor.state === 'initializing' ) { - this._throttledSave.flush(); - this._decrementCounter(); + this._debouncedSave.cancel(); return; } this._lastDocumentVersion = version; - // Wait one promise cycle to be sure that: - // 1. The save method is always asynchronous. - // 2. Save callbacks are not called inside conversions or while editor's state changes. + this._state = 'external-saving'; + + // Wait one promise cycle to be sure that save callbacks are not called + // inside a conversion or when the editor's state changes. Promise.resolve() .then( () => Promise.all( - saveCallbacks.map( cb => cb( this.editor ) ) + this._saveCallbacks.map( cb => cb( this.editor ) ) ) ) .then( () => { - this._decrementCounter(); + if ( this.editor.model.document.version > this._lastDocumentVersion ) { + this._state = 'debounced-saving'; + this._debouncedSave(); + } else { + this._state = 'initial'; + this._pendingActions.remove( this._action ); + this._action = null; + } } ); } /** - * Increments counter and adds pending action if it not exists. + * Save callbacks. * * @private + * @type {Array.} */ - _incrementCounter() { - const t = this.editor.t; - - this._saveActionCounter++; + get _saveCallbacks() { + const saveCallbacks = []; - if ( !this._action ) { - this._action = this._pendingActions.add( t( 'Saving changes' ) ); + if ( this.adapter && this.adapter.save ) { + saveCallbacks.push( this.adapter.save ); } - } - /** - * Decrements counter and removes pending action if counter is empty, - * which means, that no new save action occurred. - * - * @private - */ - _decrementCounter() { - this._saveActionCounter--; - - if ( this._saveActionCounter === 0 ) { - this._pendingActions.remove( this._action ); - this._action = null; + if ( this._config.save ) { + saveCallbacks.push( this._config.save ); } + + return saveCallbacks; } } +mix( Autosave, ObservableMixin ); + /** * An interface that requires the `save()` method. * From 1bbb661a73c90d88b15eb72a89197ed1c5998ebb Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 27 Jul 2018 20:24:38 +0200 Subject: [PATCH 02/16] Improved manual tests. --- tests/manual/autosave.js | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/tests/manual/autosave.js b/tests/manual/autosave.js index 1b29ca6..92ce8c3 100644 --- a/tests/manual/autosave.js +++ b/tests/manual/autosave.js @@ -29,17 +29,38 @@ ClassicEditor save() { const data = editor.getData(); - return saveEditorContentToDatabase( data ); + return wait( 1000 ) + .then( () => console.log( `${ getTime() } Saved content:`, data ) ); } }; + + autosave.listenTo( autosave, 'change:_state', + ( evt, propName, newValue, oldValue ) => console.log( `${ getTime() } Changed state: ${ oldValue } -> ${ newValue }` ) ); } ); -function saveEditorContentToDatabase( data ) { +function wait( time ) { return new Promise( res => { - window.setTimeout( () => { - console.log( data ); - - res(); - }, 1000 ); + window.setTimeout( res, time ); } ); } + +function getTime() { + const date = new Date(); + + return '[' + + date.getHours() + ':' + + setDigitSize( date.getMinutes(), 2 ) + ':' + + setDigitSize( date.getSeconds(), 2 ) + '.' + + setDigitSize( date.getMilliseconds(), 2 ) + + ']'; +} + +function setDigitSize( number, size ) { + const string = String( number ); + + if ( string.length >= size ) { + return string.slice( 0, size ); + } + + return '0'.repeat( size - string.length ) + string; +} From 1700f71bae433a1dceffa0218f6db8b3b5ec0fc2 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 27 Jul 2018 20:53:55 +0200 Subject: [PATCH 03/16] Introduced public Autosave#state. --- src/autosave.js | 35 +++++++++++++++++++---------------- tests/manual/autosave.js | 4 ++-- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 17e9e5c..31dd16a 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -72,6 +72,19 @@ export default class Autosave extends Plugin { * @member {module:autosave/autosave~AutosaveAdapter} #adapter */ + /** + * The state of this plugin. + * + * The plugin can be in the following states: + * + * * synchronized - when all changes are saved + * * waiting - when the plugin is waiting for other changes before calling `adapter#save()` and `config.autosave.save()` + * * saving - when the provided save method is called and the plugin waits for the response + * + * @member {'synchronized'|'waiting'|'saving'} #state + */ + this.set( 'state', 'synchronized' ); + /** * Throttled save method. * @@ -96,14 +109,6 @@ export default class Autosave extends Plugin { */ this._domEmitter = Object.create( DomEmitterMixin ); - /** - * The state of this plugin. - * - * @private - * @type {'initial'|'debounced-saving'|'external-saving'} - */ - this.set( '_state', 'initial' ); - /** * An action that will be added to pending action manager for actions happening in that plugin. * @@ -136,21 +141,19 @@ export default class Autosave extends Plugin { this._pendingActions = editor.plugins.get( PendingActions ); - // this._state = 'initial'; - this.listenTo( doc, 'change:data', () => { if ( !this._saveCallbacks.length ) { return; } - if ( this._state == 'initial' ) { + if ( this.state == 'synchronized' ) { this._action = this._pendingActions.add( this.editor.t( 'Saving changes' ) ); - this._state = 'debounced-saving'; + this.state = 'waiting'; this._debouncedSave(); } - else if ( this._state == 'debounced-saving' ) { + else if ( this.state == 'waiting' ) { this._debouncedSave(); } @@ -214,7 +217,7 @@ export default class Autosave extends Plugin { this._lastDocumentVersion = version; - this._state = 'external-saving'; + this.state = 'saving'; // Wait one promise cycle to be sure that save callbacks are not called // inside a conversion or when the editor's state changes. @@ -224,10 +227,10 @@ export default class Autosave extends Plugin { ) ) .then( () => { if ( this.editor.model.document.version > this._lastDocumentVersion ) { - this._state = 'debounced-saving'; + this.state = 'waiting'; this._debouncedSave(); } else { - this._state = 'initial'; + this.state = 'synchronized'; this._pendingActions.remove( this._action ); this._action = null; } diff --git a/tests/manual/autosave.js b/tests/manual/autosave.js index 92ce8c3..d500f93 100644 --- a/tests/manual/autosave.js +++ b/tests/manual/autosave.js @@ -30,11 +30,11 @@ ClassicEditor const data = editor.getData(); return wait( 1000 ) - .then( () => console.log( `${ getTime() } Saved content:`, data ) ); + .then( () => console.log( `${ getTime() } Saved content: ${ data }` ) ); } }; - autosave.listenTo( autosave, 'change:_state', + autosave.listenTo( autosave, 'change:state', ( evt, propName, newValue, oldValue ) => console.log( `${ getTime() } Changed state: ${ oldValue } -> ${ newValue }` ) ); } ); From 8634e667831d44a1b3a8355cb8e97faeb1db4ad3 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 27 Jul 2018 20:58:12 +0200 Subject: [PATCH 04/16] Removed custom throttle function. --- src/throttle.js | 68 ---------------------- tests/throttle.js | 143 ---------------------------------------------- 2 files changed, 211 deletions(-) delete mode 100644 src/throttle.js delete mode 100644 tests/throttle.js diff --git a/src/throttle.js b/src/throttle.js deleted file mode 100644 index 36aaf6c..0000000 --- a/src/throttle.js +++ /dev/null @@ -1,68 +0,0 @@ -/** - * @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/throttle.js b/tests/throttle.js deleted file mode 100644 index 7c718d3..0000000 --- a/tests/throttle.js +++ /dev/null @@ -1,143 +0,0 @@ -/** - * @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 after specified amount of time', () => { - 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 ); - } ); - - 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 d268277da3408d4116a5109bf68ff773d4266322 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 6 Aug 2018 14:17:56 +0200 Subject: [PATCH 05/16] Introduced `config.autosave.waitingTime`. --- src/autosave.js | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 31dd16a..d4d3dec 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -64,6 +64,8 @@ export default class Autosave extends Plugin { constructor( editor ) { super( editor ); + const config = editor.config.get( 'autosave' ) || {}; + /** * The adapter is an object with a `save()` method. That method will be called whenever * the data changes. It might be called some time after the change, @@ -85,13 +87,22 @@ export default class Autosave extends Plugin { */ this.set( 'state', 'synchronized' ); + /** + * A minimum amount of time that needs to pass after the last action. + * After that time the provided save callbacks are being called. + * + * @protected + * @type {Number} + */ + this._waitingTime = config.waitingTime || 2000; + /** * Throttled save method. * * @protected * @type {Function} */ - this._debouncedSave = debounce( this._save.bind( this ), 2000 ); + this._debouncedSave = debounce( this._save.bind( this ), this._debounceTime ); /** * Last document version. @@ -122,7 +133,7 @@ export default class Autosave extends Plugin { * @private * @type {Object} */ - this._config = editor.config.get( 'autosave' ) || {}; + this._config = config; /** * Editor's pending actions manager. @@ -328,3 +339,22 @@ mix( Autosave, ObservableMixin ); * @param {module:core/editor/editor~Editor} editor The editor instance. * @returns {Promise.<*>} */ + +/** + * The minimum amount of time that need to pass after last action to call the provided callback. + * + * ClassicEditor + * .create( editorElement, { + * autosave: { + * save( editor ) { + * return saveData( editor.getData() ); + * }, + * waitingTime: 1000 + * } + * } ); + * .then( ... ) + * .catch( ... ); + * + * @property module:autosave/autosave~AutosaveConfig#waitingTime + * @type {Number} + */ From 0b8ff771caadffa316b45ef7fba8758461b26db6 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 8 Aug 2018 14:08:46 +0200 Subject: [PATCH 06/16] Fixed tests with usage of `lodash.debounce`. --- package.json | 3 +- src/autosave.js | 2 +- tests/autosave.js | 282 ++++++++++++++++++---------------------------- 3 files changed, 113 insertions(+), 174 deletions(-) diff --git a/package.json b/package.json index 8a6c361..c01e05d 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,8 @@ ], "dependencies": { "@ckeditor/ckeditor5-utils": "^10.2.0", - "@ckeditor/ckeditor5-core": "^11.0.0" + "@ckeditor/ckeditor5-core": "^11.0.0", + "lodash-es": "^4.17.10" }, "devDependencies": { "@ckeditor/ckeditor5-engine": "^10.2.0", diff --git a/src/autosave.js b/src/autosave.js index d4d3dec..ff7cb8a 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -10,9 +10,9 @@ 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 debounce from '@ckeditor/ckeditor5-utils/src/lib/lodash/debounce'; import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin'; import mix from '@ckeditor/ckeditor5-utils/src/mix'; +import { debounce } from 'lodash-es'; /* globals window */ diff --git a/tests/autosave.js b/tests/autosave.js index e45ddbc..bd1823a 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -13,8 +13,19 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import PendingActions from '@ckeditor/ckeditor5-core/src/pendingactions'; describe( 'Autosave', () => { - const sandbox = sinon.sandbox.create( { useFakeTimers: true } ); - let editor, element, autosave; + /** @type {(typeof import 'sinon')} */ + const sandbox = sinon.sandbox; + + let editor; + + let element; + + /** @type {Autosave} */ + let autosave; + + beforeEach( () => { + sandbox.useFakeTimers( { now: Date.now() } ); + } ); afterEach( () => { sandbox.restore(); @@ -39,9 +50,6 @@ describe( 'Autosave', () => { editor = _editor; editor.setData( data ); autosave = editor.plugins.get( Autosave ); - - // Clean autosave's state after setting data. - autosave._flush(); } ); } ); @@ -56,12 +64,14 @@ describe( 'Autosave', () => { } ); 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 ); - } ); + expect( () => { + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); - autosave._flush(); + sandbox.clock.tick( 2000 ); + } ).to.not.throw(); } ); } ); @@ -78,14 +88,13 @@ describe( 'Autosave', () => { } } ) .then( _editor => { - const data = '

paragraph1

paragraph2

'; - editor = _editor; - editor.setData( data ); + autosave = editor.plugins.get( Autosave ); + // editor.listenTo( autosave, 'change:state', ( e, ...data ) => console.log( data ) ); - // Clean autosave's state after setting data. - autosave._flush(); + const data = '

paragraph1

paragraph2

'; + editor.setData( data ); } ); } ); @@ -103,7 +112,9 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - return wait().then( () => { + sandbox.clock.tick( 2000 ); + + return Promise.resolve().then( () => { sinon.assert.calledOnce( editor.config.get( 'autosave' ).save ); } ); } ); @@ -120,7 +131,9 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - return wait().then( () => { + sandbox.clock.tick( 2000 ); + + return Promise.resolve().then( () => { sinon.assert.calledOnce( autosave.adapter.save ); sinon.assert.calledOnce( editor.config.get( 'autosave' ).save ); } ); @@ -138,7 +151,9 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - return wait().then( () => { + sandbox.clock.tick( 2000 ); + + return Promise.resolve().then( () => { sinon.assert.calledWithExactly( autosave.adapter.save, editor ); sinon.assert.calledWithExactly( editor.config.get( 'autosave' ).save, editor ); } ); @@ -160,9 +175,6 @@ describe( 'Autosave', () => { editor = _editor; editor.setData( data ); autosave = editor.plugins.get( Autosave ); - - // Clean autosave's state after setting data. - autosave._flush(); } ); } ); @@ -182,12 +194,14 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - return wait().then( () => { + sandbox.clock.tick( 2000 ); + + return Promise.resolve().then( () => { sinon.assert.calledOnce( autosave.adapter.save ); } ); } ); - it( 'should throttle editor\'s change event', () => { + it( 'should debounce editor\'s change event', () => { const spy = sinon.spy(); const savedStates = []; @@ -199,139 +213,71 @@ describe( 'Autosave', () => { } }; - // Leading (will fire change). editor.model.change( writer => { writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - return wait().then( () => { - // 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 ); - } ); - - return wait(); - } ).then( () => { - // 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(); - - return wait(); - } ).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.', () => { - sandbox.useFakeTimers(); - const pendingActions = editor.plugins.get( PendingActions ); - const serverActionSpy = sinon.spy(); - const serverActionStub = sinon.stub(); - serverActionStub.onCall( 0 ).resolves( wait( 1000 ).then( serverActionSpy ) ); - - autosave.adapter = { - save: serverActionStub - }; + sandbox.clock.tick( 1000 ); editor.model.change( writer => { - writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); - editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); + editor.model.insertContent( new ModelText( 'bar' ), editor.model.document.selection ); } ); - sinon.assert.notCalled( serverActionSpy ); - expect( pendingActions.hasAny ).to.be.true; - expect( pendingActions.first.message ).to.equal( 'Saving changes' ); - sandbox.clock.tick( 1000 ); - return Promise.resolve().then( () => Promise.resolve() ).then( () => { - sinon.assert.calledOnce( serverActionSpy ); - expect( pendingActions.hasAny ).to.be.false; - } ); - } ); - - it( 'should add a pending action during the saving #2.', () => { - const serverActionSpy = sinon.spy(); - const pendingActions = editor.plugins.get( PendingActions ); - - autosave.adapter = { - save: serverActionSpy - }; - - expect( pendingActions.hasAny ).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 ); + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); + editor.model.insertContent( new ModelText( 'biz' ), editor.model.document.selection ); } ); - expect( pendingActions.hasAny ).to.be.true; + sandbox.clock.tick( 2000 ); - // Server action needs to wait at least a cycle. - return wait().then( () => { - sinon.assert.calledOnce( serverActionSpy ); - expect( pendingActions.hasAny ).to.be.false; + return Promise.resolve().then( () => { + expect( spy.callCount ).to.equal( 1 ); + expect( savedStates ).to.deep.equal( [ + '

paragraph1

biz

' + ] ); } ); } ); - it( 'should handle correctly throttled save action and preserve pending action until both save actions finish', () => { - sandbox.useFakeTimers(); - const serverActionSpy = sinon.spy(); + it( 'should add a pending action during the saving.', () => { const pendingActions = editor.plugins.get( PendingActions ); - - // Create a fake server that responses after 1000ms for the first call and after 1000ms for the second call. + const serverActionSpy = sinon.spy(); const serverActionStub = sinon.stub(); - serverActionStub.onCall( 0 ).resolves( wait( 1000 ).then( serverActionSpy ) ); - serverActionStub.onCall( 1 ).resolves( wait( 2000 ).then( serverActionSpy ) ); + serverActionStub.callsFake( () => wait( 1000 ).then( serverActionSpy ) ); autosave.adapter = { save: serverActionStub }; - expect( pendingActions.hasAny ).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 ); } ); + sinon.assert.notCalled( serverActionSpy ); expect( pendingActions.hasAny ).to.be.true; + expect( pendingActions.first.message ).to.equal( 'Saving changes' ); - 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(); + sandbox.clock.tick( 2000 ); + sinon.assert.notCalled( serverActionSpy ); expect( pendingActions.hasAny ).to.be.true; - - sandbox.clock.tick( 1000 ); + expect( pendingActions.first.message ).to.equal( 'Saving changes' ); return Promise.resolve().then( () => { - expect( pendingActions.hasAny ).to.be.true; - sinon.assert.calledOnce( serverActionSpy ); - - // Wait another 1000ms and a promise cycle for the second server action. sandbox.clock.tick( 1000 ); - } ) - .then( () => Promise.resolve() ) - .then( () => { + + return runPromiseCycles().then( () => { + sinon.assert.calledOnce( serverActionSpy ); expect( pendingActions.hasAny ).to.be.false; - sinon.assert.calledTwice( serverActionSpy ); } ); + } ); } ); - it( 'should handle correctly throttled save action and preserve pending action until both save actions finish #2', () => { + it( 'should add a pending action during the saving #2.', () => { const serverActionSpy = sinon.spy(); const pendingActions = editor.plugins.get( PendingActions ); @@ -348,25 +294,11 @@ describe( 'Autosave', () => { expect( pendingActions.hasAny ).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.hasAny ).to.be.true; + sandbox.clock.tick( 2000 ); - // Server action needs to wait at least a cycle. - return wait().then( () => { + return runPromiseCycles().then( () => { sinon.assert.calledOnce( serverActionSpy ); - expect( pendingActions.hasAny ).to.be.true; - - autosave._flush(); - - // Wait another promise cycle. - return wait().then( () => { - sinon.assert.calledTwice( serverActionSpy ); - expect( pendingActions.hasAny ).to.be.false; - } ); + expect( pendingActions.hasAny ).to.be.false; } ); } ); @@ -379,8 +311,11 @@ describe( 'Autosave', () => { writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); } ); - autosave._flush(); - sinon.assert.notCalled( autosave.adapter.save ); + sandbox.clock.tick( 2000 ); + + return runPromiseCycles().then( () => { + sinon.assert.notCalled( autosave.adapter.save ); + } ); } ); it( 'should filter out markers that does not affect the data model', () => { @@ -395,15 +330,17 @@ describe( 'Autosave', () => { writer.addMarker( 'name', { usingOperation: true, range } ); } ); - autosave._flush(); + sandbox.clock.tick( 2000 ); editor.model.change( writer => { writer.updateMarker( 'name', { range: range2 } ); } ); - autosave._flush(); + sandbox.clock.tick( 2000 ); - sinon.assert.notCalled( autosave.adapter.save ); + return runPromiseCycles().then( () => { + sinon.assert.notCalled( autosave.adapter.save ); + } ); } ); it( 'should filter out markers that does not affect the data model #2', () => { @@ -418,15 +355,17 @@ describe( 'Autosave', () => { writer.addMarker( 'name', { usingOperation: false, range } ); } ); - autosave._flush(); + sandbox.clock.tick( 2000 ); editor.model.change( writer => { writer.updateMarker( 'name', { range: range2 } ); } ); - autosave._flush(); + sandbox.clock.tick( 2000 ); - sinon.assert.notCalled( autosave.adapter.save ); + return runPromiseCycles().then( () => { + sinon.assert.notCalled( autosave.adapter.save ); + } ); } ); it( 'should call the save method when some marker affects the data model', () => { @@ -435,24 +374,15 @@ describe( 'Autosave', () => { }; 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(); - - return wait().then( () => { - editor.model.change( writer => { - writer.updateMarker( 'name', { range: range2 } ); - } ); - - autosave._flush(); + sandbox.clock.tick( 2000 ); - return wait().then( () => { - sinon.assert.calledTwice( autosave.adapter.save ); - } ); + return runPromiseCycles().then( () => { + sinon.assert.calledOnce( autosave.adapter.save ); } ); } ); @@ -468,18 +398,18 @@ describe( 'Autosave', () => { writer.addMarker( 'name', { usingOperation: false, affectsData: true, range } ); } ); - autosave._flush(); + sandbox.clock.tick( 2000 ); - return wait().then( () => { + return runPromiseCycles().then( () => { sinon.assert.calledOnce( autosave.adapter.save ); editor.model.change( writer => { writer.updateMarker( 'name', { range: range2 } ); } ); - autosave._flush(); + sandbox.clock.tick( 2000 ); - return wait().then( () => { + return runPromiseCycles().then( () => { sinon.assert.calledTwice( autosave.adapter.save ); } ); } ); @@ -497,12 +427,14 @@ describe( 'Autosave', () => { writer.addMarker( 'marker-affecting-data', { usingOperation: false, affectsData: false, range } ); } ); - return wait().then( () => { + sandbox.clock.tick( 2000 ); + + return runPromiseCycles().then( () => { sinon.assert.calledOnce( autosave.adapter.save ); } ); } ); - it( 'should flush remaining calls after editor\'s destroy', () => { + it( 'should flush remaining call after editor\'s destroy', () => { const spy = sandbox.spy(); const savedStates = []; @@ -519,24 +451,20 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - return wait().then( () => { - editor.model.change( writer => { - writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 1 ) ) ); - editor.model.insertContent( new ModelText( 'bar' ), 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

', - ] ); - } ); + return editor.destroy().then( () => { + expect( spy.callCount ).to.equal( 1 ); + expect( savedStates ).to.deep.equal( [ + '

foo

bar

', + ] ); } ); } ); 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(); const serverActionStub = sinon.stub(); @@ -565,7 +493,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should wait on editor initialization', () => { + it( 'should wait on the editor initialization', () => { element = document.createElement( 'div' ); document.body.appendChild( element ); editor = null; @@ -583,6 +511,8 @@ describe( 'Autosave', () => { writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); editor.model.insertContent( new ModelText( 'bar' ), editor.model.document.selection ); } ); + + sandbox.clock.tick( 2000 ); } ); return Promise.resolve().then( () => Promise.resolve() ); @@ -619,3 +549,11 @@ function wait( time ) { window.setTimeout( res, time ); } ); } + +function runPromiseCycles() { + return Promise.resolve() + .then( () => Promise.resolve() ) + .then( () => Promise.resolve() ) + .then( () => Promise.resolve() ) + .then( () => Promise.resolve() ); +} From 766f6d5af84d9e55236c433446d158df113743bb Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 9 Aug 2018 13:34:13 +0200 Subject: [PATCH 07/16] Improved unit tests. --- tests/autosave.js | 110 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 106 insertions(+), 4 deletions(-) diff --git a/tests/autosave.js b/tests/autosave.js index bd1823a..e25ddcc 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -75,7 +75,7 @@ describe( 'Autosave', () => { } ); } ); - describe( 'config', () => { + describe( 'config.autosave.save', () => { beforeEach( () => { element = document.createElement( 'div' ); document.body.appendChild( element ); @@ -119,7 +119,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'its callback and adapter callback should be called if both are provided', () => { + it( 'config callback and adapter callback should be called if both are provided', () => { editor.config.get( 'autosave' ).save.resetHistory(); autosave.adapter = { @@ -160,6 +160,45 @@ describe( 'Autosave', () => { } ); } ); + describe( 'config.autosave.waitingTime', () => { + afterEach( () => { + document.body.removeChild( element ); + + return editor.destroy(); + } ); + + it( 'should specify the time of waiting on the next use action before saving', () => { + element = document.createElement( 'div' ); + document.body.appendChild( element ); + + return ClassicTestEditor + .create( element, { + plugins: [ Autosave, Paragraph ], + autosave: { + save: sinon.spy(), + waitingTime: 500 + } + } ) + .then( _editor => { + editor = _editor; + + const data = '

paragraph1

paragraph2

'; + editor.setData( data ); + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + sandbox.clock.tick( 500 ); + + return Promise.resolve().then( () => { + sinon.assert.calledOnce( editor.config.get( 'autosave' ).save ); + } ); + } ); + } ); + } ); + describe( 'autosaving', () => { beforeEach( () => { element = document.createElement( 'div' ); @@ -232,10 +271,12 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'biz' ), editor.model.document.selection ); } ); + sinon.assert.notCalled( spy ); + sandbox.clock.tick( 2000 ); return Promise.resolve().then( () => { - expect( spy.callCount ).to.equal( 1 ); + sinon.assert.calledOnce( spy ); expect( savedStates ).to.deep.equal( [ '

paragraph1

biz

' ] ); @@ -277,6 +318,65 @@ describe( 'Autosave', () => { } ); } ); + // Integration test. + it( 'should add a pending action during the saving #2.', () => { + const pendingActions = editor.plugins.get( PendingActions ); + const serverActionSpy = sinon.spy(); + const serverActionStub = sinon.stub(); + serverActionStub.callsFake( () => wait( 1000 ).then( serverActionSpy ) ); + + autosave.adapter = { + save: serverActionStub + }; + + 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.notCalled( serverActionSpy ); + expect( pendingActions.hasAny ).to.be.true; + expect( pendingActions.first.message ).to.equal( 'Saving changes' ); + + sandbox.clock.tick( 2000 ); + expect( autosave.state ).to.equal( 'saving' ); + + return Promise.resolve().then( () => { + // Add new change before the response from the server. + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + sandbox.clock.tick( 1000 ); + + return runPromiseCycles(); + } ).then( () => { + // Now there should come the first server response. + expect( autosave.state ).to.equal( 'waiting' ); + expect( pendingActions.hasAny ).to.be.true; + sinon.assert.calledOnce( serverActionSpy ); + + sandbox.clock.tick( 2000 ); + + return runPromiseCycles(); + } ).then( () => { + expect( autosave.state ).to.equal( 'saving' ); + expect( pendingActions.hasAny ).to.be.true; + sinon.assert.calledOnce( serverActionSpy ); + + // Wait for the second server response. + sandbox.clock.tick( 1000 ); + + return runPromiseCycles(); + } ).then( () => { + expect( pendingActions.hasAny ).to.be.false; + expect( autosave.state ).to.equal( 'synchronized' ); + sinon.assert.calledTwice( serverActionSpy ); + } ); + } ); + it( 'should add a pending action during the saving #2.', () => { const serverActionSpy = sinon.spy(); const pendingActions = editor.plugins.get( PendingActions ); @@ -456,8 +556,10 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'bar' ), editor.model.document.selection ); } ); + sinon.assert.notCalled( spy ); + return editor.destroy().then( () => { - expect( spy.callCount ).to.equal( 1 ); + sinon.assert.calledOnce( spy ); expect( savedStates ).to.deep.equal( [ '

foo

bar

', ] ); From 29d3e764297b693c2d0aa52a2cb17ba8e00cc6f0 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 9 Aug 2018 13:37:02 +0200 Subject: [PATCH 08/16] Code style. --- tests/autosave.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tests/autosave.js b/tests/autosave.js index e25ddcc..8770129 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -13,15 +13,9 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import PendingActions from '@ckeditor/ckeditor5-core/src/pendingactions'; describe( 'Autosave', () => { - /** @type {(typeof import 'sinon')} */ const sandbox = sinon.sandbox; - let editor; - - let element; - - /** @type {Autosave} */ - let autosave; + let editor, element, autosave; beforeEach( () => { sandbox.useFakeTimers( { now: Date.now() } ); From 5174852545938be42fe40973c1604bb571671991 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 13 Aug 2018 13:47:38 +0200 Subject: [PATCH 09/16] Improved unit tests. --- tests/autosave.js | 66 +++++++++++++++++++++++------------------------ 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/tests/autosave.js b/tests/autosave.js index 8770129..460933d 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -85,7 +85,6 @@ describe( 'Autosave', () => { editor = _editor; autosave = editor.plugins.get( Autosave ); - // editor.listenTo( autosave, 'change:state', ( e, ...data ) => console.log( data ) ); const data = '

paragraph1

paragraph2

'; editor.setData( data ); @@ -277,7 +276,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should add a pending action during the saving.', () => { + it( 'should add a pending action after a change and wait on the server response', () => { const pendingActions = editor.plugins.get( PendingActions ); const serverActionSpy = sinon.spy(); const serverActionStub = sinon.stub(); @@ -312,8 +311,32 @@ describe( 'Autosave', () => { } ); } ); - // Integration test. it( 'should add a pending action during the saving #2.', () => { + const serverActionSpy = sinon.spy(); + const pendingActions = editor.plugins.get( PendingActions ); + + autosave.adapter = { + save: serverActionSpy + }; + + expect( pendingActions.hasAny ).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.hasAny ).to.be.true; + + sandbox.clock.tick( 2000 ); + + return runPromiseCycles().then( () => { + sinon.assert.calledOnce( serverActionSpy ); + expect( pendingActions.hasAny ).to.be.false; + } ); + } ); + + it( 'should be in correct states during the saving', () => { const pendingActions = editor.plugins.get( PendingActions ); const serverActionSpy = sinon.spy(); const serverActionStub = sinon.stub(); @@ -371,32 +394,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should add a pending action during the saving #2.', () => { - const serverActionSpy = sinon.spy(); - const pendingActions = editor.plugins.get( PendingActions ); - - autosave.adapter = { - save: serverActionSpy - }; - - expect( pendingActions.hasAny ).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.hasAny ).to.be.true; - - sandbox.clock.tick( 2000 ); - - return runPromiseCycles().then( () => { - sinon.assert.calledOnce( serverActionSpy ); - expect( pendingActions.hasAny ).to.be.false; - } ); - } ); - - it( 'should filter out changes in the selection', () => { + it( 'should filter out selection changes', () => { autosave.adapter = { save: sandbox.spy() }; @@ -412,7 +410,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should filter out markers that does not affect the data model', () => { + it( 'should filter out markers that does not affect the data', () => { autosave.adapter = { save: sandbox.spy() }; @@ -437,7 +435,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should filter out markers that does not affect the data model #2', () => { + it( 'should filter out markers that does not affect the data #2', () => { autosave.adapter = { save: sandbox.spy() }; @@ -462,7 +460,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should call the save method when some marker affects the data model', () => { + it( 'should call the save method when some marker affects the data', () => { autosave.adapter = { save: sandbox.spy() }; @@ -480,7 +478,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should call the save method when some marker affects the data model #2', () => { + it( 'should call the save method when some marker affects the data #2', () => { autosave.adapter = { save: sandbox.spy() }; @@ -509,7 +507,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should call the save method when some marker affects the data model #3', () => { + it( 'should call the save method when some marker affects the data #3', () => { autosave.adapter = { save: sandbox.spy() }; From b07f3e02da394a860a74816b003a6badedcbdbb0 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 13 Aug 2018 14:31:21 +0200 Subject: [PATCH 10/16] Fixed bug and added test for it. --- src/autosave.js | 2 +- tests/autosave.js | 46 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index ff7cb8a..da6107a 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -102,7 +102,7 @@ export default class Autosave extends Plugin { * @protected * @type {Function} */ - this._debouncedSave = debounce( this._save.bind( this ), this._debounceTime ); + this._debouncedSave = debounce( this._save.bind( this ), this._waitingTime ); /** * Last document version. diff --git a/tests/autosave.js b/tests/autosave.js index 460933d..95f659d 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -183,9 +183,53 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - sandbox.clock.tick( 500 ); + sandbox.clock.tick( 499 ); return Promise.resolve().then( () => { + sinon.assert.notCalled( editor.config.get( 'autosave' ).save ); + + sandbox.clock.tick( 1 ); + + return Promise.resolve(); + } ).then( () => { + // Callback should be called exactly after 500ms. + sinon.assert.calledOnce( editor.config.get( 'autosave' ).save ); + } ); + } ); + } ); + + it( 'should be default to 2000', () => { + element = document.createElement( 'div' ); + document.body.appendChild( element ); + + return ClassicTestEditor + .create( element, { + plugins: [ Autosave, Paragraph ], + autosave: { + save: sinon.spy() + } + } ) + .then( _editor => { + editor = _editor; + + const data = '

paragraph1

paragraph2

'; + editor.setData( data ); + + editor.model.change( writer => { + writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); + editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); + } ); + + sandbox.clock.tick( 1999 ); + + return Promise.resolve().then( () => { + sinon.assert.notCalled( editor.config.get( 'autosave' ).save ); + + sandbox.clock.tick( 1 ); + + return Promise.resolve(); + } ).then( () => { + // Callback should be called exactly after 2000ms by default. sinon.assert.calledOnce( editor.config.get( 'autosave' ).save ); } ); } ); From b6800cdaa5a649234c5b4fc9be4722ea3b719e65 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 13 Aug 2018 15:40:13 +0200 Subject: [PATCH 11/16] Fixed incorrect test, CC 100%. --- tests/autosave.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/tests/autosave.js b/tests/autosave.js index 95f659d..723f4a7 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -631,7 +631,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should wait on the editor initialization', () => { + it( 'should run callbacks until the editor is in the ready state', () => { element = document.createElement( 'div' ); document.body.appendChild( element ); editor = null; @@ -642,7 +642,7 @@ describe( 'Autosave', () => { } init() { - this.editor.once( 'ready', () => { + this.editor.once( 'dataReady', () => { const editor = this.editor; editor.model.change( writer => { @@ -650,10 +650,8 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'bar' ), editor.model.document.selection ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 10 ); } ); - - return Promise.resolve().then( () => Promise.resolve() ); } } @@ -661,18 +659,16 @@ describe( 'Autosave', () => { .create( element, { plugins: [ Autosave, Paragraph, AsyncPlugin ], autosave: { - save: sinon.spy( () => { - expect( editor ).to.not.be.null; - } ) + save: sinon.spy(), + waitingTime: 5 } } ) .then( _editor => { editor = _editor; - autosave = editor.plugins.get( Autosave ); const spy = editor.config.get( 'autosave' ).save; expect( editor.getData() ).to.equal( '

bar

' ); - sinon.assert.calledOnce( spy ); + sinon.assert.notCalled( spy ); } ) .then( () => { document.body.removeChild( element ); From c8a3c177fb18caf53637688c1989e8a7b7d47e55 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 13 Aug 2018 16:49:11 +0200 Subject: [PATCH 12/16] Cleaned up the API and docs. --- src/autosave.js | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index da6107a..a51b1b6 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -66,6 +66,10 @@ export default class Autosave extends Plugin { const config = editor.config.get( 'autosave' ) || {}; + // A minimum amount of time that needs to pass after the last action. + // After that time the provided save callbacks are being called. + const waitingTime = config.waitingTime || 2000; + /** * The adapter is an object with a `save()` method. That method will be called whenever * the data changes. It might be called some time after the change, @@ -88,26 +92,18 @@ export default class Autosave extends Plugin { this.set( 'state', 'synchronized' ); /** - * A minimum amount of time that needs to pass after the last action. - * After that time the provided save callbacks are being called. + * Debounced save method. The `save` method is called the specified `waitingTime` after the `debouncedSave` is called, + * unless new action happens in the meantime. * - * @protected - * @type {Number} - */ - this._waitingTime = config.waitingTime || 2000; - - /** - * Throttled save method. - * - * @protected + * @private * @type {Function} */ - this._debouncedSave = debounce( this._save.bind( this ), this._waitingTime ); + this._debouncedSave = debounce( this._save.bind( this ), waitingTime ); /** * Last document version. * - * @protected + * @private * @type {Number} */ this._lastDocumentVersion = editor.model.document.version; @@ -121,19 +117,19 @@ export default class Autosave extends Plugin { this._domEmitter = Object.create( DomEmitterMixin ); /** - * An action that will be added to pending action manager for actions happening in that plugin. + * The config of this plugins. * * @private - * @member {Object} #_action + * @type {Object} */ + this._config = config; /** - * The config of this plugins. + * An action that will be added to pending action manager for actions happening in that plugin. * * @private - * @type {Object} + * @member {Object} #_action */ - this._config = config; /** * Editor's pending actions manager. From 3ae597c27eafbe8c7ba0c8f255feab76ac07787d Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 13 Aug 2018 16:57:16 +0200 Subject: [PATCH 13/16] Added missing test. --- tests/autosave.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/autosave.js b/tests/autosave.js index 723f4a7..0a56145 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -67,6 +67,10 @@ describe( 'Autosave', () => { sandbox.clock.tick( 2000 ); } ).to.not.throw(); } ); + + it( 'should start with the `synchronized` state', () => { + expect( autosave.state ).to.equal( 'synchronized' ); + } ); } ); describe( 'config.autosave.save', () => { From aa35629b7ad3f47461baac45f8374a2b77a1585a Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 13 Aug 2018 18:13:26 +0200 Subject: [PATCH 14/16] Improved comment. --- src/autosave.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/autosave.js b/src/autosave.js index a51b1b6..5404344 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -164,7 +164,9 @@ export default class Autosave extends Plugin { this._debouncedSave(); } - // Do nothing if the plugin is in `external-saving` state. + // If the plugin is in `saving` state, it will change its state later basing on the `document.version`. + // If the `document.version` will be higher than stored `#_lastDocumentVersion`, then it means, that some `change:data` + // event has fired in the meantime. } ); // Flush on the editor's destroy listener with the highest priority to ensure that From fe7f4eb77fbf0f12b7cafd2b131715c03615bd23 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 13 Aug 2018 18:20:24 +0200 Subject: [PATCH 15/16] Improved scenarios in the manual tests. --- tests/manual/autosave.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/manual/autosave.md b/tests/manual/autosave.md index 3a4cdf3..4dded0e 100644 --- a/tests/manual/autosave.md +++ b/tests/manual/autosave.md @@ -1,5 +1,9 @@ -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. Play with the editor. You should logs of the changing autosave's states. 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. + +1. Type something without big time gaps. Once you stop typing there should be the first `waiting -> saving` and then the response should show up with the whole editor's content. + +1. Type something. Once you'll see the `waiting -> saving` change, type something else. Then you should see the response and the `saving->waiting` change. Then you should see another response from the fake server. From 5e66b2a61290b687605fadd9701e2433c5d7c6f0 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Tue, 14 Aug 2018 17:05:00 +0200 Subject: [PATCH 16/16] Modified default waitingTime to 1000ms. --- src/autosave.js | 4 ++-- tests/autosave.js | 44 ++++++++++++++++++++++---------------------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/autosave.js b/src/autosave.js index 5404344..2b0fd09 100644 --- a/src/autosave.js +++ b/src/autosave.js @@ -68,7 +68,7 @@ export default class Autosave extends Plugin { // A minimum amount of time that needs to pass after the last action. // After that time the provided save callbacks are being called. - const waitingTime = config.waitingTime || 2000; + const waitingTime = config.waitingTime || 1000; /** * The adapter is an object with a `save()` method. That method will be called whenever @@ -347,7 +347,7 @@ mix( Autosave, ObservableMixin ); * save( editor ) { * return saveData( editor.getData() ); * }, - * waitingTime: 1000 + * waitingTime: 2000 * } * } ); * .then( ... ) diff --git a/tests/autosave.js b/tests/autosave.js index 0a56145..ecbbea8 100644 --- a/tests/autosave.js +++ b/tests/autosave.js @@ -64,7 +64,7 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); } ).to.not.throw(); } ); @@ -109,7 +109,7 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return Promise.resolve().then( () => { sinon.assert.calledOnce( editor.config.get( 'autosave' ).save ); @@ -128,7 +128,7 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return Promise.resolve().then( () => { sinon.assert.calledOnce( autosave.adapter.save ); @@ -148,7 +148,7 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return Promise.resolve().then( () => { sinon.assert.calledWithExactly( autosave.adapter.save, editor ); @@ -202,7 +202,7 @@ describe( 'Autosave', () => { } ); } ); - it( 'should be default to 2000', () => { + it( 'should be default to 1000', () => { element = document.createElement( 'div' ); document.body.appendChild( element ); @@ -224,7 +224,7 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - sandbox.clock.tick( 1999 ); + sandbox.clock.tick( 999 ); return Promise.resolve().then( () => { sinon.assert.notCalled( editor.config.get( 'autosave' ).save ); @@ -233,7 +233,7 @@ describe( 'Autosave', () => { return Promise.resolve(); } ).then( () => { - // Callback should be called exactly after 2000ms by default. + // Callback should be called exactly after 1000ms by default. sinon.assert.calledOnce( editor.config.get( 'autosave' ).save ); } ); } ); @@ -274,7 +274,7 @@ describe( 'Autosave', () => { editor.model.insertContent( new ModelText( 'foo' ), editor.model.document.selection ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return Promise.resolve().then( () => { sinon.assert.calledOnce( autosave.adapter.save ); @@ -314,7 +314,7 @@ describe( 'Autosave', () => { sinon.assert.notCalled( spy ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return Promise.resolve().then( () => { sinon.assert.calledOnce( spy ); @@ -343,7 +343,7 @@ describe( 'Autosave', () => { expect( pendingActions.hasAny ).to.be.true; expect( pendingActions.first.message ).to.equal( 'Saving changes' ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); sinon.assert.notCalled( serverActionSpy ); expect( pendingActions.hasAny ).to.be.true; @@ -376,7 +376,7 @@ describe( 'Autosave', () => { expect( pendingActions.hasAny ).to.be.true; - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return runPromiseCycles().then( () => { sinon.assert.calledOnce( serverActionSpy ); @@ -403,7 +403,7 @@ describe( 'Autosave', () => { expect( pendingActions.hasAny ).to.be.true; expect( pendingActions.first.message ).to.equal( 'Saving changes' ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); expect( autosave.state ).to.equal( 'saving' ); return Promise.resolve().then( () => { @@ -423,7 +423,7 @@ describe( 'Autosave', () => { expect( pendingActions.hasAny ).to.be.true; sinon.assert.calledOnce( serverActionSpy ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return runPromiseCycles(); } ).then( () => { @@ -451,7 +451,7 @@ describe( 'Autosave', () => { writer.setSelection( ModelRange.createIn( editor.model.document.getRoot().getChild( 0 ) ) ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return runPromiseCycles().then( () => { sinon.assert.notCalled( autosave.adapter.save ); @@ -470,13 +470,13 @@ describe( 'Autosave', () => { writer.addMarker( 'name', { usingOperation: true, range } ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); editor.model.change( writer => { writer.updateMarker( 'name', { range: range2 } ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return runPromiseCycles().then( () => { sinon.assert.notCalled( autosave.adapter.save ); @@ -495,13 +495,13 @@ describe( 'Autosave', () => { writer.addMarker( 'name', { usingOperation: false, range } ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); editor.model.change( writer => { writer.updateMarker( 'name', { range: range2 } ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return runPromiseCycles().then( () => { sinon.assert.notCalled( autosave.adapter.save ); @@ -519,7 +519,7 @@ describe( 'Autosave', () => { writer.addMarker( 'name', { usingOperation: true, affectsData: true, range } ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return runPromiseCycles().then( () => { sinon.assert.calledOnce( autosave.adapter.save ); @@ -538,7 +538,7 @@ describe( 'Autosave', () => { writer.addMarker( 'name', { usingOperation: false, affectsData: true, range } ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return runPromiseCycles().then( () => { sinon.assert.calledOnce( autosave.adapter.save ); @@ -547,7 +547,7 @@ describe( 'Autosave', () => { writer.updateMarker( 'name', { range: range2 } ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return runPromiseCycles().then( () => { sinon.assert.calledTwice( autosave.adapter.save ); @@ -567,7 +567,7 @@ describe( 'Autosave', () => { writer.addMarker( 'marker-affecting-data', { usingOperation: false, affectsData: false, range } ); } ); - sandbox.clock.tick( 2000 ); + sandbox.clock.tick( 1000 ); return runPromiseCycles().then( () => { sinon.assert.calledOnce( autosave.adapter.save );