Răsfoiți Sursa

Merge pull request #276 from ckeditor/t/272

TreeView.addObserver should only allow a single observer of a type.
Szymon Cofalik 9 ani în urmă
părinte
comite
e9169d01d9

+ 1 - 17
packages/ckeditor5-engine/src/treeview/observer/domeventobserver.js

@@ -27,20 +27,11 @@
  *			}
  *		}
  *
- * Or:
- *
- *		const observer = new DomEventObserver();
- *		observer.domEventType = 'click';
- *		observer.onDomEvent = ( domEvt ) => observer.fire( 'click' );
- *
  * @memberOf core.treeView.observer
  * @extends core.treeView.observer.Observer
  */
 export default class DomEventObserver {
-	/**
-	 * @inheritDoc
-	 */
-	init( treeView ) {
+	constructor( treeView ) {
 		/**
 		 * Reference to the {@link core.treeView.TreeView} object.
 		 *
@@ -48,13 +39,6 @@ export default class DomEventObserver {
 		 */
 		this.treeView = treeView;
 
-		/**
-		 * State of the observer. If it is disabled not event should be fired.
-		 *
-		 * @member {Boolean} core.treeView.observer.DomEventObserver#isEnabled
-		 */
-		this.isEnabled = false;
-
 		/**
 		 * Type of the DOM event the observer should listen on.
 		 *

+ 13 - 23
packages/ckeditor5-engine/src/treeview/observer/mutationobserver.js

@@ -22,12 +22,8 @@ import Observer from './observer.js';
  * @extends core.treeView.observer.Observer
  */
 export default class MutationObserver extends Observer {
-	/**
-	 * Mutation observer constructor. Note that most of the initialization is done in
-	 * {@link core.treeView.observer.MutationObserver#init} method.
-	 */
-	constructor() {
-		super();
+	constructor( treeView ) {
+		super( treeView );
 
 		/**
 		 * Native mutation observer config.
@@ -41,18 +37,6 @@ export default class MutationObserver extends Observer {
 			characterDataOldValue: true,
 			subtree: true
 		};
-	}
-
-	/**
-	 * @inheritDoc
-	 */
-	init( treeView ) {
-		/**
-		 * Reference to the {@link core.treeView.TreeView} object.
-		 *
-		 * @member {core.treeView.TreeView} core.treeView.observer.MutationObserver#treeView
-		 */
-		this.treeView = treeView;
 
 		/**
 		 * Reference to the {@link core.treeView.TreeView#domConverter}.
@@ -68,22 +52,28 @@ export default class MutationObserver extends Observer {
 		 */
 		this.renderer = treeView.renderer;
 
+		/**
+		 * Observed DOM elements.
+		 *
+		 * @private
+		 * @member {Array.<HTMLElement>} core.treeView.observer.MutationObserver#_domElements
+		 */
+		this._domElements = [];
+
 		/**
 		 * Native mutation observer.
 		 *
 		 * @private
-		 * @member {window.MutationObserver} core.treeView.observer.MutationObserver#_mutationObserver
+		 * @member {MutationObserver} core.treeView.observer.MutationObserver#_mutationObserver
 		 */
 		this._mutationObserver = new window.MutationObserver( this._onMutations.bind( this ) );
-
-		this.domElements = [];
 	}
 
 	/**
 	 * @inheritDoc
 	 */
 	observe( domElement ) {
-		this.domElements.push( domElement );
+		this._domElements.push( domElement );
 
 		if ( this.isEnabled ) {
 			this._mutationObserver.observe( domElement, this._config );
@@ -96,7 +86,7 @@ export default class MutationObserver extends Observer {
 	enable() {
 		this.isEnabled = true;
 
-		for ( let domElement of this.domElements ) {
+		for ( let domElement of this._domElements ) {
 			this._mutationObserver.observe( domElement, this._config );
 		}
 	}

+ 19 - 4
packages/ckeditor5-engine/src/treeview/observer/observer.js

@@ -14,12 +14,27 @@
  */
 export default class Observer {
 	/**
-	 * Inits the observer class, caches all needed {@link core.treeView.TreeView} properties.
-	 * After being inited, the observer will be {@link core.treeView.observer.Observer#enabled enabled}.
+	 * Creates an instance of the observer.
 	 *
-	 * @method core.treeView.observer.Observer#init
-	 * @param {core.treeView.TreeView}
+	 * @param {core.treeView.TreeView} treeView
 	 */
+	constructor( treeView ) {
+		/**
+		 * Reference to the {@link core.treeView.TreeView} object.
+		 *
+		 * @readonly
+		 * @member {core.treeView.TreeView} core.treeView.observer.Observer#treeView
+		 */
+		this.treeView = treeView;
+
+		/**
+		 * State of the observer. If it is disabled events will not be fired.
+		 *
+		 * @readonly
+		 * @member {Boolean} core.treeView.observer.Observer#isEnabled
+		 */
+		this.isEnabled = false;
+	}
 
 	/**
 	 * Enables the observer. This method is called when then observer is registered to the

+ 39 - 18
packages/ckeditor5-engine/src/treeview/treeview.js

@@ -37,14 +37,6 @@ export default class TreeView {
 		 */
 		this.domRoots = new Map();
 
-		/**
-		 * Set of {@link core.treeView.Observer observers}.
-		 *
-		 * @readonly
-		 * @member {Set.<core.treeView.Observer>} core.treeView.TreeView#observers
-		 */
-		this.observers = new Set();
-
 		/**
 		 * Tree View writer.
 		 *
@@ -55,7 +47,7 @@ export default class TreeView {
 
 		/**
 		 * Instance of the {@link core.treeView.DomConverter domConverter} use by
-		 * {@link core.treeView.TreeView#renderer renderer} and {@link core.treeView.TreeView#observers observers}.
+		 * {@link core.treeView.TreeView#renderer renderer} and {@link core.treeView.observer.Observer observers}.
 		 *
 		 * @readonly
 		 * @member {core.treeView.DomConverter} core.treeView.TreeView#domConverter
@@ -77,17 +69,36 @@ export default class TreeView {
 		 * @member {core.treeView.Renderer} core.treeView.TreeView#renderer
 		 */
 		this.renderer = new Renderer( this.domConverter );
+
+		/**
+		 * Set of registered {@link core.treeView.Observer observers}.
+		 *
+		 * @protected
+		 * @member {Set.<core.treeView.Observer>} core.treeView.TreeView_#observers
+		 */
+		this._observers = new Set();
 	}
 
 	/**
-	 * Adds an observer to the set of observers. This method also {@link core.treeView.Observer#init initializes} and
-	 * {@link core.treeView.Observer#enable enables} the observer.
+	 * Creates observer of the given type if not yet created, {@link core.treeView.Observer#enable enables} it
+	 * and {@link core.treeView.observer.Observer#observe attaches} to all existing and future
+	 * {@link core.treeView.TreeView#domRoots DOM roots}.
 	 *
-	 * @param {core.treeView.Observer} observer The observer to add.
+	 * Note: Observers are recognized by their constructor (classes). A single observer will be instantiated and used only
+	 * when registered for the first time. This means that features and other components can register a single observer
+	 * multiple times without caring whether it has been already added or not.
+	 *
+	 * @param {Function} Observer The constructor of an observer to add.
+	 * Should create an instance inheriting from {@link core.treeView.observer.Observer}.
 	 */
-	addObserver( observer ) {
-		this.observers.add( observer );
-		observer.init( this );
+	addObserver( Observer ) {
+		if ( this._hasObserver( Observer ) ) {
+			return;
+		}
+
+		const observer = new Observer( this );
+
+		this._observers.add( observer );
 
 		for ( let [ name, domElement ] of this.domRoots ) {
 			observer.observe( domElement, name );
@@ -121,7 +132,7 @@ export default class TreeView {
 		this.domRoots.set( name, domRoot );
 		this.viewRoots.set( name, viewRoot );
 
-		for ( let observer of this.observers ) {
+		for ( let observer of this._observers ) {
 			observer.observe( domRoot, name );
 		}
 	}
@@ -131,16 +142,26 @@ export default class TreeView {
 	 * before rendering and reattached after that.
 	 */
 	render() {
-		for ( let observer of this.observers ) {
+		for ( let observer of this._observers ) {
 			observer.disable();
 		}
 
 		this.renderer.render();
 
-		for ( let observer of this.observers ) {
+		for ( let observer of this._observers ) {
 			observer.enable();
 		}
 	}
+
+	/**
+	 * Checks whether the given observer was already added.
+	 *
+	 * @private
+	 * @param {Function} Observer The observer constructor to check.
+	 */
+	_hasObserver( Observer ) {
+		return Array.from( this._observers ).some( ( observer ) => observer.constructor === Observer );
+	}
 }
 
 utils.mix( TreeView, EmitterMixin );

+ 23 - 10
packages/ckeditor5-engine/tests/treeview/manual/clickobserver.js

@@ -11,13 +11,15 @@ import TreeView from '/ckeditor5/core/treeview/treeview.js';
 import DomEventObserver from '/ckeditor5/core/treeview/observer/domeventobserver.js';
 
 const treeView = new TreeView();
+let observer1, observer2;
 
-class ClickObserver extends DomEventObserver {
-	constructor( id ) {
-		super();
+class ClickObserver1 extends DomEventObserver {
+	constructor( treeView ) {
+		super( treeView );
 
-		this.id = id;
+		this.id = 1;
 		this.domEventType = 'click';
+		observer1 = this;
 	}
 
 	onDomEvent( domEvt ) {
@@ -25,18 +27,29 @@ class ClickObserver extends DomEventObserver {
 	}
 }
 
-const observer1 = new ClickObserver( 1 );
-const observer2 = new ClickObserver( 2 );
+class ClickObserver2 extends DomEventObserver {
+	constructor( treeView ) {
+		super( treeView );
+
+		this.id = 2;
+		this.domEventType = 'click';
+		observer2 = this;
+	}
+
+	onDomEvent( domEvt ) {
+		this.fire( 'click', this.id, domEvt.target.id );
+	}
+}
 
 treeView.on( 'click', ( evt, eventId, elementId ) => console.log( 'click', eventId, elementId ) );
 document.getElementById( 'enable1' ).addEventListener( 'click', () => observer1.enable() );
 document.getElementById( 'disable1' ).addEventListener( 'click', () => observer1.disable() );
 
-// Random order
-treeView.addObserver( observer1 );
+// Random order.
+treeView.addObserver( ClickObserver1 );
 
 treeView.createRoot( document.getElementById( 'clickerA' ), 'clickerA' );
 
-treeView.addObserver( observer2 );
+treeView.addObserver( ClickObserver2 );
 
-treeView.createRoot( document.getElementById( 'clickerB' ), 'clickerB' );
+treeView.createRoot( document.getElementById( 'clickerB' ), 'clickerB' );

+ 1 - 2
packages/ckeditor5-engine/tests/treeview/manual/mutationobserver.js

@@ -14,11 +14,10 @@ import MutationObserver from '/ckeditor5/core/treeview/observer/mutationobserver
 
 const treeView = new TreeView();
 treeView.createRoot( document.getElementById( 'editor' ), 'editor' );
-const mutationObserver = new MutationObserver();
 
 treeView.on( 'mutations', ( evt, mutations ) => console.log( mutations ) );
 
-treeView.addObserver( mutationObserver );
+treeView.addObserver( MutationObserver );
 
 treeView.viewRoots.get( 'editor' ).appendChildren( [
 	new Element( 'p', [], [ new Text( 'foo' ) ] ),

+ 1 - 1
packages/ckeditor5-engine/tests/treeview/manual/typing.js

@@ -18,7 +18,7 @@ treeView.createRoot( document.getElementById( 'editor' ), 'editor' );
 treeView.on( 'mutations', ( evt, mutations ) => console.log( mutations ) );
 treeView.on( 'mutations', handleTyping );
 
-treeView.addObserver( new MutationObserver() );
+treeView.addObserver( MutationObserver );
 
 treeView.viewRoots.get( 'editor' ).appendChildren( [ new Element( 'p', [], [ new Text( 'foo' ) ] ) ] );
 

+ 19 - 20
packages/ckeditor5-engine/tests/treeview/observer/domeventobserver.js

@@ -11,29 +11,31 @@ import DomEventObserver from '/ckeditor5/core/treeview/observer/domeventobserver
 import TreeView from '/ckeditor5/core/treeview/treeview.js';
 
 class TestObserver extends DomEventObserver {
-	constructor( id ) {
-		super();
-
-		this.id = id;
+	constructor( treeView ) {
+		super( treeView );
 
 		this.domEventType = 'click';
 	}
 
 	onDomEvent( domEvt ) {
-		this.fire( 'click', this.id, domEvt );
+		this.fire( 'click', 'foo', domEvt );
 	}
 }
 
 describe( 'DomEventObserver', () => {
-	it( 'should add event lister', () => {
-		const testObserver = new TestObserver( 'foo' );
+	let treeView;
+
+	beforeEach( () => {
+		treeView = new TreeView();
+	} );
+
+	it( 'should add event listener', () => {
 		const domElement = document.createElement( 'p' );
 		const domEvent = new MouseEvent( 'click' );
-		const treeView = new TreeView();
 		const evtSpy = sinon.spy();
 
 		treeView.createRoot( domElement, 'root' );
-		treeView.addObserver( testObserver );
+		treeView.addObserver( TestObserver );
 		treeView.on( 'click', evtSpy );
 
 		domElement.dispatchEvent( domEvent );
@@ -44,16 +46,15 @@ describe( 'DomEventObserver', () => {
 	} );
 
 	it( 'should not fire event if observer is disabled', () => {
-		const testObserver = new TestObserver( 'foo' );
 		const domElement = document.createElement( 'p' );
 		const domEvent = new MouseEvent( 'click' );
-		const treeView = new TreeView();
 		const evtSpy = sinon.spy();
 
 		treeView.createRoot( domElement, 'root' );
-		treeView.addObserver( testObserver );
+		treeView.addObserver( TestObserver );
 		treeView.on( 'click', evtSpy );
 
+		const testObserver = Array.from( treeView._observers )[ 0 ];
 		testObserver.disable();
 
 		domElement.dispatchEvent( domEvent );
@@ -61,17 +62,18 @@ describe( 'DomEventObserver', () => {
 		expect( evtSpy.called ).to.be.false;
 	} );
 
-	it( 'should be fire event if observer is disabled and re-enabled', () => {
-		const testObserver = new TestObserver( 'foo' );
+	it( 'should fire event if observer is disabled and re-enabled', () => {
 		const domElement = document.createElement( 'p' );
 		const domEvent = new MouseEvent( 'click' );
 		const treeView = new TreeView();
 		const evtSpy = sinon.spy();
 
 		treeView.createRoot( domElement, 'root' );
-		treeView.addObserver( testObserver );
+		treeView.addObserver( TestObserver );
 		treeView.on( 'click', evtSpy );
 
+		const testObserver = Array.from( treeView._observers )[ 0 ];
+
 		testObserver.disable();
 
 		domElement.dispatchEvent( domEvent );
@@ -87,12 +89,9 @@ describe( 'DomEventObserver', () => {
 
 	describe( 'fire', () => {
 		it( 'should do nothing if observer is disabled', () => {
-			const testObserver = new TestObserver();
-			const treeView = new TreeView();
+			const testObserver = new TestObserver( treeView );
 			const fireSpy = sinon.spy( treeView, 'fire' );
 
-			testObserver.init( treeView );
-
 			testObserver.disable();
 
 			testObserver.fire( 'click' );
@@ -106,4 +105,4 @@ describe( 'DomEventObserver', () => {
 			expect( fireSpy.calledOnce ).to.be.true;
 		} );
 	} );
-} );
+} );

+ 3 - 2
packages/ckeditor5-engine/tests/treeview/observer/mutationobserver.js

@@ -18,12 +18,13 @@ describe( 'MutationObserver', () => {
 	beforeEach( () => {
 		treeView = new TreeView();
 		domEditor = document.getElementById( 'editor' );
-		mutationObserver = new MutationObserver();
 		lastMutations = null;
 
 		treeView.createRoot( domEditor, 'editor' );
 
-		treeView.addObserver( mutationObserver );
+		treeView.addObserver( MutationObserver );
+		mutationObserver = Array.from( treeView._observers )[ 0 ];
+
 		treeView.on( 'mutations', ( evt, mutations ) => lastMutations = mutations );
 
 		viewRoot = treeView.viewRoots.get( 'editor' );

+ 61 - 14
packages/ckeditor5-engine/tests/treeview/treeview.js

@@ -24,7 +24,7 @@ describe( 'TreeView', () => {
 
 			expect( count( treeView.domRoots ) ).to.equal( 0 );
 			expect( count( treeView.viewRoots ) ).to.equal( 0 );
-			expect( count( treeView.observers ) ).to.equal( 0 );
+			expect( count( treeView._observers ) ).to.equal( 0 );
 			expect( treeView ).to.have.property( 'renderer' ).that.is.instanceOf( Renderer );
 			expect( treeView ).to.have.property( 'writer' ).that.is.instanceOf( Writer );
 			expect( treeView ).to.have.property( 'domConverter' ).that.is.instanceOf( DomConverter );
@@ -55,33 +55,80 @@ describe( 'TreeView', () => {
 		} );
 	} );
 
-	describe( 'observer', () => {
-		let observerMock, treeView;
+	describe( 'addObserver', () => {
+		let ObserverMock, treeView;
 
 		beforeEach( () => {
-			observerMock = new Observer();
-			observerMock.enable = sinon.spy();
-			observerMock.disable = sinon.spy();
-			observerMock.init = sinon.spy();
+			ObserverMock = class extends Observer {
+				constructor( treeView ) {
+					super( treeView );
+
+					this.enable = sinon.spy();
+					this.disable = sinon.spy();
+				}
+			};
 
 			treeView = new TreeView( document.createElement( 'div' ) );
 			treeView.renderer.render = sinon.spy();
 		} );
 
-		it( 'should be inited and enabled on adding', () => {
-			treeView.addObserver( observerMock );
+		it( 'should be instantiated and enabled on adding', () => {
+			treeView.addObserver( ObserverMock );
+
+			expect( treeView._observers.size ).to.equal( 1 );
 
-			expect( treeView.observers.has( observerMock ) ).to.be.true;
-			sinon.assert.calledOnce( observerMock.init );
-			sinon.assert.calledWith( observerMock.init, treeView );
+			const observerMock = Array.from( treeView._observers )[ 0 ];
+
+			expect( observerMock ).to.have.property( 'treeView', treeView );
 			sinon.assert.calledOnce( observerMock.enable );
 		} );
 
+		it( 'should instantiate one observer only once', () => {
+			let instantiated = 0;
+			let enabled = 0;
+
+			class ObserverMock2 extends Observer {
+				constructor( treeView ) {
+					super( treeView );
+					instantiated++;
+				}
+
+				enable() {
+					enabled++;
+				}
+			}
+
+			treeView.addObserver( ObserverMock2 );
+			treeView.addObserver( ObserverMock2 );
+
+			expect( treeView._observers.size ).to.equal( 1 );
+			expect( instantiated ).to.equal( 1 );
+			expect( enabled ).to.equal( 1 );
+
+			treeView.addObserver( ObserverMock );
+			expect( treeView._observers.size ).to.equal( 2 );
+		} );
+
+		it( 'should instantiate child class of already registered observer', () => {
+			class ObserverMock extends Observer {
+				enable() {}
+			}
+			class ChildObserverMock extends ObserverMock {
+				enable() {}
+			}
+
+			treeView.addObserver( ObserverMock );
+			treeView.addObserver( ChildObserverMock );
+
+			expect( treeView._observers.size ).to.equal( 2 );
+		} );
+
 		it( 'should be disabled and re-enabled on render', () => {
-			treeView.addObserver( observerMock );
+			treeView.addObserver( ObserverMock );
 			treeView.render();
 
-			expect( treeView.observers.has( observerMock ) ).to.be.true;
+			const observerMock = Array.from( treeView._observers )[ 0 ];
+
 			sinon.assert.calledOnce( observerMock.disable );
 			sinon.assert.calledOnce( treeView.renderer.render );
 			sinon.assert.calledTwice( observerMock.enable );