From 60190469296776011db5cf43268ac2d0d9d370cb Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Mon, 16 Dec 2019 13:42:53 +0100 Subject: [PATCH 1/6] Internal: Ported force disabling logic from the command class. --- src/widgettoolbarrepository.js | 73 ++++++++++++++++++++++++++++++ tests/widgettoolbarrepository.js | 76 ++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+) diff --git a/src/widgettoolbarrepository.js b/src/widgettoolbarrepository.js index bca8857d..6cf9b536 100644 --- a/src/widgettoolbarrepository.js +++ b/src/widgettoolbarrepository.js @@ -70,6 +70,55 @@ export default class WidgetToolbarRepository extends Plugin { }, { priority: 'high' } ); } + /** + * Flag indicating whether a command is enabled or disabled. + * A disabled command will do nothing when executed. + * + * A concrete command class should control this value by overriding the {@link #refresh `refresh()`} method. + * + * It is possible to disable a command from "outside". For instance, in your integration you may want to disable + * a certain set of commands for the time being. To do that, you can use the fact that `isEnabled` is observable + * and it fires the `set:isEnabled` event every time anyone tries to modify its value: + * + * function disableCommand( cmd ) { + * cmd.on( 'set:isEnabled', forceDisable, { priority: 'highest' } ); + * + * cmd.isEnabled = false; + * + * // Make it possible to enable the command again. + * return () => { + * cmd.off( 'set:isEnabled', forceDisable ); + * cmd.refresh(); + * }; + * + * function forceDisable( evt ) { + * evt.return = false; + * evt.stop(); + * } + * } + * + * // Usage: + * + * // Disabling the command. + * const enableBold = disableCommand( editor.commands.get( 'bold' ) ); + * + * // Enabling the command again. + * enableBold(); + * + * @observable + * @readonly + * @member {Boolean} #isEnabled + */ + this.set( 'isEnabled', true ); + + /** + * Holds identifiers for {@link #forceDisabled} mechanism. + * + * @type {Set.} + * @private + */ + this._disableStack = new Set(); + /** * A map of toolbar definitions. * @@ -142,6 +191,24 @@ export default class WidgetToolbarRepository extends Plugin { } ); } + forceDisabled( id ) { + this._disableStack.add( id ); + + if ( this._disableStack.size == 1 ) { + this.on( 'set:isEnabled', forceDisable, { priority: 'highest' } ); + this.isEnabled = false; + } + } + + clearForceDisabled( id ) { + this._disableStack.delete( id ); + + if ( this._disableStack.size == 0 ) { + this.off( 'set:isEnabled', forceDisable ); + this.isEnabled = true; + } + } + /** * Iterates over stored toolbars and makes them visible or hidden. * @@ -293,3 +360,9 @@ function isWidgetSelected( selection ) { * there is no such element). The function accepts an instance of {@link module:engine/view/selection~Selection}. * @property {String} balloonClassName CSS class for the widget balloon when a toolbar is displayed. */ + +// Helper function that forces command to be disabled. +function forceDisable( evt ) { + evt.return = false; + evt.stop(); +} diff --git a/tests/widgettoolbarrepository.js b/tests/widgettoolbarrepository.js index a8e845b6..f2dcac12 100644 --- a/tests/widgettoolbarrepository.js +++ b/tests/widgettoolbarrepository.js @@ -510,6 +510,82 @@ describe( 'WidgetToolbarRepository - integration with the BalloonToolbar', () => expect( balloon.visibleView ).to.equal( balloonToolbar.toolbarView ); } ); + + describe.only( 'disableable', () => { + describe( 'isEnabled', () => { + it( 'is enabled by default', () => { + expect( widgetToolbarRepository.isEnabled ).to.be.true; + } ); + + it( 'fires change event', () => { + const spy = sinon.spy(); + + widgetToolbarRepository.on( 'change:isEnabled', spy ); + + widgetToolbarRepository.isEnabled = false; + + expect( spy.calledOnce ).to.be.true; + } ); + } ); + + describe( 'forceDisabled() / clearForceDisabled()', () => { + it( 'forceDisabled() should disable the plugin', () => { + widgetToolbarRepository.forceDisabled( 'foo' ); + widgetToolbarRepository.isEnabled = true; + + expect( widgetToolbarRepository.isEnabled ).to.be.false; + } ); + + it( 'clearForceDisabled() should enable the plugin', () => { + widgetToolbarRepository.forceDisabled( 'foo' ); + widgetToolbarRepository.clearForceDisabled( 'foo' ); + + expect( widgetToolbarRepository.isEnabled ).to.be.true; + } ); + + it( 'clearForceDisabled() used with wrong identifier should not enable the plugin', () => { + widgetToolbarRepository.forceDisabled( 'foo' ); + widgetToolbarRepository.clearForceDisabled( 'bar' ); + widgetToolbarRepository.isEnabled = true; + + expect( widgetToolbarRepository.isEnabled ).to.be.false; + } ); + + it( 'using forceDisabled() twice with the same identifier should not have any effect', () => { + widgetToolbarRepository.forceDisabled( 'foo' ); + widgetToolbarRepository.forceDisabled( 'foo' ); + widgetToolbarRepository.clearForceDisabled( 'foo' ); + + expect( widgetToolbarRepository.isEnabled ).to.be.true; + } ); + + it( 'plugin is enabled only after all disables were cleared', () => { + widgetToolbarRepository.forceDisabled( 'foo' ); + widgetToolbarRepository.forceDisabled( 'bar' ); + widgetToolbarRepository.clearForceDisabled( 'foo' ); + widgetToolbarRepository.isEnabled = true; + + expect( widgetToolbarRepository.isEnabled ).to.be.false; + + widgetToolbarRepository.clearForceDisabled( 'bar' ); + + expect( widgetToolbarRepository.isEnabled ).to.be.true; + } ); + + it( 'plugin should remain disabled if isEnabled has a callback disabling it', () => { + widgetToolbarRepository.on( 'set:isEnabled', evt => { + evt.return = false; + evt.stop(); + } ); + + widgetToolbarRepository.forceDisabled( 'foo' ); + widgetToolbarRepository.clearForceDisabled( 'foo' ); + widgetToolbarRepository.isEnabled = true; + + expect( widgetToolbarRepository.isEnabled ).to.be.false; + } ); + } ); + } ); } ); function getSelectedFakeWidget( selection ) { From 2eb02f57cebe299f2296eb016c3eee1c42455256 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Mon, 16 Dec 2019 14:48:43 +0100 Subject: [PATCH 2/6] Internal: Make visibility functions aware of the fact that plugin could be disabled. --- src/widgettoolbarrepository.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/widgettoolbarrepository.js b/src/widgettoolbarrepository.js index 6cf9b536..c6fe6b86 100644 --- a/src/widgettoolbarrepository.js +++ b/src/widgettoolbarrepository.js @@ -132,6 +132,10 @@ export default class WidgetToolbarRepository extends Plugin { */ this._balloon = this.editor.plugins.get( 'ContextualBalloon' ); + this.on( 'change:isEnabled', () => { + this._updateToolbarsVisibility(); + } ); + this.listenTo( editor.ui, 'update', () => { this._updateToolbarsVisibility(); } ); @@ -222,7 +226,11 @@ export default class WidgetToolbarRepository extends Plugin { for ( const definition of this._toolbarDefinitions.values() ) { const relatedElement = definition.getRelatedElement( this.editor.editing.view.document.selection ); - if ( !this.editor.ui.focusTracker.isFocused ) { + if ( !this.isEnabled ) { + if ( this._isToolbarInBalloon( definition ) ) { + this._hideToolbar( definition ); + } + } else if ( !this.editor.ui.focusTracker.isFocused ) { if ( this._isToolbarVisible( definition ) ) { this._hideToolbar( definition ); } From 1597f5914e013f54e0de7ea46b58d079cc9ce744 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Mon, 16 Dec 2019 15:04:06 +0100 Subject: [PATCH 3/6] Tests: Added more tests for code coverage for disabling plugin. --- tests/widgettoolbarrepository.js | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/tests/widgettoolbarrepository.js b/tests/widgettoolbarrepository.js index f2dcac12..853f2d52 100644 --- a/tests/widgettoolbarrepository.js +++ b/tests/widgettoolbarrepository.js @@ -141,6 +141,32 @@ describe( 'WidgetToolbarRepository', () => { expect( balloon.visibleView ).to.equal( fakeWidgetToolbarView ); } ); + it( 'toolbar should be hidden when the plugin gets disabled', () => { + widgetToolbarRepository.register( 'fake', { + items: editor.config.get( 'fake.toolbar' ), + getRelatedElement: getSelectedFakeWidget + } ); + + setData( model, 'foo[]' ); + + widgetToolbarRepository.isEnabled = false; + + expect( balloon.visibleView ).to.be.null; + } ); + + it( 'toolbar should be hidden when the plugin was disabled prior changing selection', () => { + widgetToolbarRepository.register( 'fake', { + items: editor.config.get( 'fake.toolbar' ), + getRelatedElement: getSelectedFakeWidget + } ); + + widgetToolbarRepository.isEnabled = false; + + setData( model, 'foo[]' ); + + expect( balloon.visibleView ).to.be.null; + } ); + it( 'toolbar should be hidden when the `getRelatedElement` callback returns null', () => { widgetToolbarRepository.register( 'fake', { items: editor.config.get( 'fake.toolbar' ), @@ -511,7 +537,7 @@ describe( 'WidgetToolbarRepository - integration with the BalloonToolbar', () => expect( balloon.visibleView ).to.equal( balloonToolbar.toolbarView ); } ); - describe.only( 'disableable', () => { + describe( 'disableable', () => { describe( 'isEnabled', () => { it( 'is enabled by default', () => { expect( widgetToolbarRepository.isEnabled ).to.be.true; From 418257df7d4ae8c1f1e423bf475fccc027f39548 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Mon, 16 Dec 2019 16:39:44 +0100 Subject: [PATCH 4/6] Docs: Ported API docs for forceDisable-related methods from the command class. --- src/widgettoolbarrepository.js | 95 ++++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 38 deletions(-) diff --git a/src/widgettoolbarrepository.js b/src/widgettoolbarrepository.js index c6fe6b86..31b20716 100644 --- a/src/widgettoolbarrepository.js +++ b/src/widgettoolbarrepository.js @@ -71,39 +71,15 @@ export default class WidgetToolbarRepository extends Plugin { } /** - * Flag indicating whether a command is enabled or disabled. - * A disabled command will do nothing when executed. + * Flag indicating whether a plugin is enabled or disabled. + * A disabled plugin won't show any toolbar. * - * A concrete command class should control this value by overriding the {@link #refresh `refresh()`} method. + * Plugin can be simply disabled like that: * - * It is possible to disable a command from "outside". For instance, in your integration you may want to disable - * a certain set of commands for the time being. To do that, you can use the fact that `isEnabled` is observable - * and it fires the `set:isEnabled` event every time anyone tries to modify its value: + * // Disable the plugin so that no toolbars are visible. + * editor.plugins.get( 'WidgetToolbarRepository' ).isEnabled = false; * - * function disableCommand( cmd ) { - * cmd.on( 'set:isEnabled', forceDisable, { priority: 'highest' } ); - * - * cmd.isEnabled = false; - * - * // Make it possible to enable the command again. - * return () => { - * cmd.off( 'set:isEnabled', forceDisable ); - * cmd.refresh(); - * }; - * - * function forceDisable( evt ) { - * evt.return = false; - * evt.stop(); - * } - * } - * - * // Usage: - * - * // Disabling the command. - * const enableBold = disableCommand( editor.commands.get( 'bold' ) ); - * - * // Enabling the command again. - * enableBold(); + * You can also use {@link #forceDisabled} method. * * @observable * @readonly @@ -111,14 +87,6 @@ export default class WidgetToolbarRepository extends Plugin { */ this.set( 'isEnabled', true ); - /** - * Holds identifiers for {@link #forceDisabled} mechanism. - * - * @type {Set.} - * @private - */ - this._disableStack = new Set(); - /** * A map of toolbar definitions. * @@ -132,6 +100,14 @@ export default class WidgetToolbarRepository extends Plugin { */ this._balloon = this.editor.plugins.get( 'ContextualBalloon' ); + /** + * Holds identifiers for {@link #forceDisabled} mechanism. + * + * @type {Set.} + * @private + */ + this._disableStack = new Set(); + this.on( 'change:isEnabled', () => { this._updateToolbarsVisibility(); } ); @@ -195,6 +171,44 @@ export default class WidgetToolbarRepository extends Plugin { } ); } + /** + * Forces the plugin to be disabled. + * + * Plugin may be disabled by multiple features or algorithms (at once). When disabling a plugin, unique id should be passed + * (e.g. feature name). The same identifier should be used when {@link #clearForceDisabled enabling back} the plugin. + * The plugin becomes enabled only after all features {@link #clearForceDisabled enabled it back}. + * + * Disabling and enabling a plugin: + * + * const plugin = editor.plugins.get( 'WidgetToolbarRepository' ); + * + * plugin.isEnabled; // -> true + * plugin.forceDisabled( 'MyFeature' ); + * plugin.isEnabled; // -> false + * plugin.clearForceDisabled( 'MyFeature' ); + * plugin.isEnabled; // -> true + * + * Plugin disabled by multiple features: + * + * plugin.forceDisabled( 'MyFeature' ); + * plugin.forceDisabled( 'OtherFeature' ); + * plugin.clearForceDisabled( 'MyFeature' ); + * plugin.isEnabled; // -> false + * plugin.clearForceDisabled( 'OtherFeature' ); + * plugin.isEnabled; // -> true + * + * Multiple disabling with the same identifier is redundant: + * + * plugin.forceDisabled( 'MyFeature' ); + * plugin.forceDisabled( 'MyFeature' ); + * plugin.clearForceDisabled( 'MyFeature' ); + * plugin.isEnabled; // -> true + * + * **Note:** some plugins or algorithms may have more complex logic when it comes to enabling or disabling certain plugins, + * so the plugin might be still disabled after {@link #clearForceDisabled} was used. + * + * @param {String} id Unique identifier for disabling. Use the same id when {@link #clearForceDisabled enabling back} the plugin. + */ forceDisabled( id ) { this._disableStack.add( id ); @@ -204,6 +218,11 @@ export default class WidgetToolbarRepository extends Plugin { } } + /** + * Clears forced disable previously set through {@link #clearForceDisabled}. See {@link #clearForceDisabled}. + * + * @param {String} id Unique identifier, equal to the one passed in {@link #forceDisabled} call. + */ clearForceDisabled( id ) { this._disableStack.delete( id ); From 90de677d5d489cf889894fe6cb3aa5e05d6cce40 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Mon, 16 Dec 2019 16:57:12 +0100 Subject: [PATCH 5/6] Docs: Minor correction. --- src/widgettoolbarrepository.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/widgettoolbarrepository.js b/src/widgettoolbarrepository.js index 31b20716..a651559e 100644 --- a/src/widgettoolbarrepository.js +++ b/src/widgettoolbarrepository.js @@ -219,7 +219,7 @@ export default class WidgetToolbarRepository extends Plugin { } /** - * Clears forced disable previously set through {@link #clearForceDisabled}. See {@link #clearForceDisabled}. + * Clears forced disable previously set through {@link #forceDisabled}. See {@link #forceDisabled}. * * @param {String} id Unique identifier, equal to the one passed in {@link #forceDisabled} call. */ From 86ed3af918d459b37b71ba2840e7499e2e0c2082 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Mon, 16 Dec 2019 17:34:15 +0100 Subject: [PATCH 6/6] Internal: Combined common conditions that results with hiding a toolbar. --- src/widgettoolbarrepository.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/widgettoolbarrepository.js b/src/widgettoolbarrepository.js index a651559e..dccbd39a 100644 --- a/src/widgettoolbarrepository.js +++ b/src/widgettoolbarrepository.js @@ -245,7 +245,7 @@ export default class WidgetToolbarRepository extends Plugin { for ( const definition of this._toolbarDefinitions.values() ) { const relatedElement = definition.getRelatedElement( this.editor.editing.view.document.selection ); - if ( !this.isEnabled ) { + if ( !this.isEnabled || !relatedElement ) { if ( this._isToolbarInBalloon( definition ) ) { this._hideToolbar( definition ); } @@ -253,10 +253,6 @@ export default class WidgetToolbarRepository extends Plugin { if ( this._isToolbarVisible( definition ) ) { this._hideToolbar( definition ); } - } else if ( !relatedElement ) { - if ( this._isToolbarInBalloon( definition ) ) { - this._hideToolbar( definition ); - } } else { const relatedElementDepth = relatedElement.getAncestors().length;