Trevor Parscal has uploaded a new change for review.
https://gerrit.wikimedia.org/r/59330
Change subject: UI "Views" refactor
......................................................................
UI "Views" refactor
Objective:
Make it possible for inspectors to inspect nodes or annotations, rather than
only annotations. Meanwhile, also make it possible for dialogs to edit an
annotation.
Strategy:
Switch from using type patterns to associate inspectors with annotations to
using arrays of classes, similar to how dialogs already work. Introduce a view
registry which provides lookups for relationships between models and views.
This is more centralized and less repetitive than implement matching functions
for both annotations and nodes in both the dialog and inspector factories.
Changes:
*.php
* Added links to new file
ve.AnnotationAction.js
* Removed unused parameter to filter annotations using a string or regexp
ve.dm.AnnotationSet.js
* Switched from property/value arguments to callbacks
ve.ui.*(Dialog|Inspector).js
* Replaced type patterns with class lists
* Added class to view registry
ve.ui.*Tool.js, ve.ui.Context.js
* Updated model/view relationship lookup
ve.ui.*Factory.js
* Removed overly-specific lookup functions
ve.ui.Inspector.js
* Removed typePattern property
* Updated model/view relationship lookup
ve.ui.ViewRegistry.js
* New class!
* Migrated node and annotation lookup functions from factories
Change-Id: Ic2bbcf072fdd87e5ce8a03fe1ae3e6d8d50e2593
---
M VisualEditor.php
M demos/ve/index.php
M modules/ve/actions/ve.AnnotationAction.js
M modules/ve/dm/ve.dm.AnnotationSet.js
M modules/ve/test/index.php
M modules/ve/ui/dialogs/ve.ui.MediaDialog.js
M modules/ve/ui/inspectors/ve.ui.LinkInspector.js
M modules/ve/ui/inspectors/ve.ui.MWLinkInspector.js
M modules/ve/ui/tools/ve.ui.DialogButtonTool.js
M modules/ve/ui/tools/ve.ui.InspectorButtonTool.js
M modules/ve/ui/ve.ui.Context.js
M modules/ve/ui/ve.ui.DialogFactory.js
M modules/ve/ui/ve.ui.Inspector.js
M modules/ve/ui/ve.ui.InspectorFactory.js
A modules/ve/ui/ve.ui.ViewRegistry.js
15 files changed, 137 insertions(+), 126 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/VisualEditor
refs/changes/30/59330/1
diff --git a/VisualEditor.php b/VisualEditor.php
index 2453d51..136c4ab 100644
--- a/VisualEditor.php
+++ b/VisualEditor.php
@@ -337,6 +337,7 @@
've/ui/ve.ui.Frame.js',
've/ui/ve.ui.Window.js',
've/ui/ve.ui.WindowSet.js',
+ 've/ui/ve.ui.ViewRegistry.js',
've/ui/ve.ui.Inspector.js',
've/ui/ve.ui.InspectorFactory.js',
've/ui/ve.ui.Dialog.js',
diff --git a/demos/ve/index.php b/demos/ve/index.php
index 57cd489..372a30f 100644
--- a/demos/ve/index.php
+++ b/demos/ve/index.php
@@ -215,6 +215,7 @@
<script src="../../modules/ve/ui/ve.ui.Frame.js"></script>
<script src="../../modules/ve/ui/ve.ui.Window.js"></script>
<script src="../../modules/ve/ui/ve.ui.WindowSet.js"></script>
+ <script
src="../../modules/ve/ui/ve.ui.ViewRegistry.js"></script>
<script src="../../modules/ve/ui/ve.ui.Inspector.js"></script>
<script
src="../../modules/ve/ui/ve.ui.InspectorFactory.js"></script>
<script src="../../modules/ve/ui/ve.ui.Dialog.js"></script>
diff --git a/modules/ve/actions/ve.AnnotationAction.js
b/modules/ve/actions/ve.AnnotationAction.js
index d6dea7f..fa9f234 100644
--- a/modules/ve/actions/ve.AnnotationAction.js
+++ b/modules/ve/actions/ve.AnnotationAction.js
@@ -77,15 +77,12 @@
* Clear all annotations.
*
* @method
- * @param {string|RegExp} [filter] Annotation name or RegExp that matches types
*/
-ve.AnnotationAction.prototype.clearAll = function ( filter ) {
+ve.AnnotationAction.prototype.clearAll = function () {
var i, len, arr,
fragment = this.surface.getModel().getFragment(),
annotations = fragment.getAnnotations( true );
- if ( typeof filter === 'string' || filter instanceof RegExp ) {
- annotations = annotations.getAnnotationsByName( filter );
- }
+
arr = annotations.get();
// TODO: Allow multiple annotations to be set or cleared by
ve.dm.SurfaceFragment, probably
// using an annotation set and ideally building a single transaction
diff --git a/modules/ve/dm/ve.dm.AnnotationSet.js
b/modules/ve/dm/ve.dm.AnnotationSet.js
index c61870d..39e4f83 100644
--- a/modules/ve/dm/ve.dm.AnnotationSet.js
+++ b/modules/ve/dm/ve.dm.AnnotationSet.js
@@ -44,22 +44,22 @@
* Get an annotation set containing only annotations within the set with a
specific name.
*
* @method
- * @param {string|RegExp} name Regular expression or string to compare types
with
+ * @param {string} name Type name
* @returns {ve.dm.AnnotationSet} Copy of annotation set
*/
ve.dm.AnnotationSet.prototype.getAnnotationsByName = function ( name ) {
- return this.filter( 'name', name );
+ return this.filter( function ( annotation ) { return annotation.name
=== name; } );
};
/**
* Check if any annotations in the set have a specific name.
*
* @method
- * @param {string|RegExp} name Regular expression or string to compare names
with
+ * @param {string} name Type name
* @returns {boolean} Annotation of given type exists in the set
*/
ve.dm.AnnotationSet.prototype.hasAnnotationWithName = function ( name ) {
- return this.containsMatching( 'name', name );
+ return this.containsMatching( function ( annotation ) { return
annotation.name === name; } );
};
/**
@@ -177,19 +177,17 @@
/**
* Filter the set by an item property.
*
- * This returns a new set with all values in the set for which value.property
matches filter (if
- * filter is a RegExp) or is equal to filter,
+ * This returns a new set with all values in the set for which the callback
returned true for.
*
* @method
- * @param {string} property Property to check
- * @param {Mixed|RegExp} filter Regular expression or value to filter for
+ * @param {Function} callback Function that takes an annotation and returns
boolean true to include
* @param {boolean} [returnBool] For internal use only
* @returns {ve.dm.AnnotationSet} New set containing only the matching values
*/
-ve.dm.AnnotationSet.prototype.filter = function ( property, filter, returnBool
) {
+ve.dm.AnnotationSet.prototype.filter = function ( callback, returnBool ) {
var i, length, result, value;
+
if ( !returnBool ) {
- // TODO: Consider alternative ways to instantiate a new set of
the same type as the subclass
result = this.clone();
// TODO: Should we be returning this on all methods that modify
the original? Might help
// with chainability, but perhaps it's also confusing because
most chainable methods return
@@ -198,10 +196,7 @@
}
for ( i = 0, length = this.getLength(); i < length; i++ ) {
value = this.getStore().value( this.getIndex( i ) );
- if (
- ( filter instanceof RegExp && filter.test(
value[property] ) ) ||
- ( typeof filter === 'string' && value[property] ===
filter )
- ) {
+ if ( callback( value ) ) {
if ( returnBool ) {
return true;
} else {
@@ -220,12 +215,11 @@
* @see ve.dm.AnnotationSet#filter
*
* @method
- * @param {string} property
- * @param {Mixed|RegExp} filter
+ * @param {Function} callback Function that takes an annotation and returns
boolean true to include
* @returns {boolean} True if at least one value matches, false otherwise
*/
-ve.dm.AnnotationSet.prototype.containsMatching = function ( property, filter )
{
- return this.filter( property, filter, true );
+ve.dm.AnnotationSet.prototype.containsMatching = function ( callback ) {
+ return this.filter( callback, true );
};
/**
diff --git a/modules/ve/test/index.php b/modules/ve/test/index.php
index fe9ccda..679ecd8 100644
--- a/modules/ve/test/index.php
+++ b/modules/ve/test/index.php
@@ -158,6 +158,7 @@
<script src="../../ve/ui/ve.ui.Frame.js"></script>
<script src="../../ve/ui/ve.ui.Window.js"></script>
<script src="../../ve/ui/ve.ui.WindowSet.js"></script>
+ <script src="../../ve/ui/ve.ui.ViewRegistry.js"></script>
<script src="../../ve/ui/ve.ui.Inspector.js"></script>
<script src="../../ve/ui/ve.ui.InspectorFactory.js"></script>
<script src="../../ve/ui/ve.ui.Dialog.js"></script>
diff --git a/modules/ve/ui/dialogs/ve.ui.MediaDialog.js
b/modules/ve/ui/dialogs/ve.ui.MediaDialog.js
index 838adae..7482f57 100644
--- a/modules/ve/ui/dialogs/ve.ui.MediaDialog.js
+++ b/modules/ve/ui/dialogs/ve.ui.MediaDialog.js
@@ -30,8 +30,10 @@
ve.ui.MediaDialog.static.icon = 'picture';
-ve.ui.MediaDialog.static.nodeClasses = [ve.dm.MWImageNode];
+ve.ui.MediaDialog.static.modelClasses = [ ve.dm.MWImageNode ];
/* Registration */
ve.ui.dialogFactory.register( 'media', ve.ui.MediaDialog );
+
+ve.ui.viewRegistry.register( 'media', ve.ui.MediaDialog );
diff --git a/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
b/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
index 713cb95..25d72cf 100644
--- a/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
+++ b/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
@@ -33,7 +33,7 @@
ve.ui.LinkInspector.static.titleMessage = 'visualeditor-linkinspector-title';
-ve.ui.LinkInspector.static.typePattern = /^link(\/|$)/;
+ve.ui.LinkInspector.static.modelClasses = [ ve.dm.LinkAnnotation ];
ve.ui.LinkInspector.static.linkTargetInputWidget = ve.ui.LinkTargetInputWidget;
@@ -210,3 +210,5 @@
/* Registration */
ve.ui.inspectorFactory.register( 'link', ve.ui.LinkInspector );
+
+ve.ui.viewRegistry.register( 'link', ve.ui.LinkInspector );
diff --git a/modules/ve/ui/inspectors/ve.ui.MWLinkInspector.js
b/modules/ve/ui/inspectors/ve.ui.MWLinkInspector.js
index c9d6fb8..ee008bd 100644
--- a/modules/ve/ui/inspectors/ve.ui.MWLinkInspector.js
+++ b/modules/ve/ui/inspectors/ve.ui.MWLinkInspector.js
@@ -27,7 +27,9 @@
/* Static properties */
-ve.ui.MWLinkInspector.static.typePattern = /^link\/MW(in|ex)ternal$/;
+ve.ui.MWLinkInspector.static.modelClasses = [
+ ve.dm.MWExternalLinkAnnotation, ve.dm.MWInternalLinkAnnotation
+];
ve.ui.MWLinkInspector.static.linkTargetInputWidget =
ve.ui.MWLinkTargetInputWidget;
@@ -78,3 +80,5 @@
/* Registration */
ve.ui.inspectorFactory.register( 'mwLink', ve.ui.MWLinkInspector );
+
+ve.ui.viewRegistry.register( 'mwLink', ve.ui.MWLinkInspector );
diff --git a/modules/ve/ui/tools/ve.ui.DialogButtonTool.js
b/modules/ve/ui/tools/ve.ui.DialogButtonTool.js
index ee26540..feadbd3 100644
--- a/modules/ve/ui/tools/ve.ui.DialogButtonTool.js
+++ b/modules/ve/ui/tools/ve.ui.DialogButtonTool.js
@@ -59,7 +59,7 @@
ve.ui.DialogButtonTool.prototype.onUpdateState = function ( nodes ) {
if ( nodes.length ) {
this.setActive(
- ve.ui.dialogFactory.getDialogsForNode( nodes[0] )
+ ve.ui.viewRegistry.getViewsForNode( nodes[0] )
.indexOf( this.constructor.static.dialog ) !==
-1
);
}
diff --git a/modules/ve/ui/tools/ve.ui.InspectorButtonTool.js
b/modules/ve/ui/tools/ve.ui.InspectorButtonTool.js
index acfd875..9825e80 100644
--- a/modules/ve/ui/tools/ve.ui.InspectorButtonTool.js
+++ b/modules/ve/ui/tools/ve.ui.InspectorButtonTool.js
@@ -57,8 +57,7 @@
*/
ve.ui.InspectorButtonTool.prototype.onUpdateState = function ( nodes, full ) {
this.setActive(
- full.hasAnnotationWithName(
- ve.ui.inspectorFactory.getTypePattern(
this.constructor.static.inspector )
- )
+ ve.ui.viewRegistry.getViewsForAnnotations( full )
+ .indexOf( this.constructor.static.inspector ) !== -1
);
};
diff --git a/modules/ve/ui/ve.ui.Context.js b/modules/ve/ui/ve.ui.Context.js
index 83380f5..2ff9b64 100644
--- a/modules/ve/ui/ve.ui.Context.js
+++ b/modules/ve/ui/ve.ui.Context.js
@@ -164,7 +164,7 @@
* @chainable
*/
ve.ui.Context.prototype.update = function () {
- var i, nodes, items,
+ var i, nodes, views,
fragment = this.surface.getModel().getFragment(),
selection = fragment.getRange(),
inspector = this.inspectors.getCurrent();
@@ -174,7 +174,7 @@
this.show();
} else {
// No inspector is open, or the selection has changed, show a
menu of available inspectors
- items = ve.ui.inspectorFactory.getInspectorsForAnnotations(
fragment.getAnnotations() );
+ views = ve.ui.viewRegistry.getViewsForAnnotations(
fragment.getAnnotations() );
nodes = fragment.getLeafNodes();
for ( i = 0; i < nodes.length; i++ ) {
if ( nodes[i].range && nodes[i].range.isCollapsed() ) {
@@ -183,21 +183,21 @@
}
}
if ( nodes.length === 1 ) {
- items = items.concat(
ve.ui.dialogFactory.getDialogsForNode( nodes[0].node ) );
+ views = views.concat(
ve.ui.viewRegistry.getViewsForNode( nodes[0].node ) );
}
- for ( i = 0; i < items.length; i++ ) {
- if ( !ve.ui.toolFactory.lookup( items[i] ) ) {
- items.splice( i, 1 );
+ for ( i = 0; i < views.length; i++ ) {
+ if ( !ve.ui.toolFactory.lookup( views[i] ) ) {
+ views.splice( i, 1 );
i--;
}
}
- if ( items.length ) {
+ if ( views.length ) {
// There's at least one inspectable annotation, build a
menu and show it
this.$menu.empty();
this.toolbar = new ve.ui.Toolbar(
$( '<div class="ve-ui-context-toolbar"></div>'
),
this.surface,
- [{ 'name': 'inspectors', 'items' : items }]
+ [{ 'name': 'inspectors', 'items' : views }]
);
this.$menu.append( this.toolbar.$ );
this.show();
diff --git a/modules/ve/ui/ve.ui.DialogFactory.js
b/modules/ve/ui/ve.ui.DialogFactory.js
index 35e3f33..7a1af6f 100644
--- a/modules/ve/ui/ve.ui.DialogFactory.js
+++ b/modules/ve/ui/ve.ui.DialogFactory.js
@@ -21,35 +21,6 @@
ve.inheritClass( ve.ui.DialogFactory, ve.Factory );
-/* Methods */
-
-/**
- * Get a list of dialogs for a node.
- *
- * @method
- * @param {ve.dm.Node} node Node to be edited
- * @returns {string[]} Symbolic names of dialogs that can be used to edit node
- */
-ve.ui.DialogFactory.prototype.getDialogsForNode = function ( node ) {
- var i, len, dialog, j, nodeClasses,
- matches = [];
-
- for ( i = 0, len = this.entries.length; i < len; i++ ) {
- dialog = this.entries[i];
- nodeClasses = this.registry[dialog].static &&
this.registry[dialog].static.nodeClasses;
- if ( nodeClasses ) {
- j = nodeClasses.length;
- while ( nodeClasses[--j] ) {
- if ( node instanceof nodeClasses[j] ) {
- matches.push( dialog );
- break;
- }
- }
- }
- }
- return matches;
-};
-
/* Initialization */
ve.ui.dialogFactory = new ve.ui.DialogFactory();
diff --git a/modules/ve/ui/ve.ui.Inspector.js b/modules/ve/ui/ve.ui.Inspector.js
index 9566857..61207f4 100644
--- a/modules/ve/ui/ve.ui.Inspector.js
+++ b/modules/ve/ui/ve.ui.Inspector.js
@@ -9,13 +9,14 @@
* UserInterface inspector.
*
* @class
+ * @abstract
* @extends ve.ui.Window
*
* @constructor
* @param {ve.Surface} surface
*/
ve.ui.Inspector = function VeUiInspector( surface ) {
- // Inheritance
+ // Parent constructor
ve.ui.Window.call( this, surface );
// Properties
@@ -30,14 +31,6 @@
ve.inheritClass( ve.ui.Inspector, ve.ui.Window );
/* Static Properties */
-
-/**
- * Pattern to use when matching against annotation type strings.
- *
- * @static
- * @property {RegExp}
- */
-ve.ui.Inspector.static.typePattern = new RegExp();
ve.ui.Inspector.static.titleMessage = 've-ui-inspector-title';
@@ -145,7 +138,11 @@
* @returns {ve.dm.AnnotationSet} Matching annotations
*/
ve.ui.Inspector.prototype.getMatchingAnnotations = function ( fragment ) {
- return fragment.getAnnotations().getAnnotationsByName(
this.constructor.static.typePattern );
+ var constructor = this.constructor;
+
+ return fragment.getAnnotations().filter( function ( annnotation ) {
+ return ve.ui.viewRegistry.isViewRelatedToModel( constructor,
annnotation );
+ } );
};
/* Initialization */
diff --git a/modules/ve/ui/ve.ui.InspectorFactory.js
b/modules/ve/ui/ve.ui.InspectorFactory.js
index f2af2c5..7b41554 100644
--- a/modules/ve/ui/ve.ui.InspectorFactory.js
+++ b/modules/ve/ui/ve.ui.InspectorFactory.js
@@ -21,55 +21,6 @@
ve.inheritClass( ve.ui.InspectorFactory, ve.Factory );
-/* Methods */
-
-/**
- * Get an inspector constructor for a given annotation type.
- *
- * @method
- * @param {string} name Symbolic name of inspector to get pattern for
- * @returns {RegExp} Regular expression matching annotations relevant to a
given inspector
- * @throws {Error} Unknown inspector
- */
-ve.ui.InspectorFactory.prototype.getTypePattern = function ( name ) {
- if ( name in this.registry ) {
- return this.registry[name].static.typePattern;
- }
- throw new Error( 'Unknown inspector: ' + name );
-};
-
-/**
- * Get a list of inspectors from a set of annotations.
- *
- * @method
- * @param {ve.dm.AnnotationSet} annotations Annotations to be inspected
- * @returns {string[]} Symbolic names of inspectors that can be used to
inspect annotations
- */
-ve.ui.InspectorFactory.prototype.getInspectorsForAnnotations = function (
annotations ) {
- if ( annotations.isEmpty() ) {
- return [];
- }
-
- var i, len, j, annotation,
- matches = [],
- inspectors = this.entries.slice( 0 ),
- inspector,
- arr = annotations.get();
-
- for ( i = 0, len = arr.length; i < len; i++ ) {
- annotation = arr[i];
- j = inspectors.length;
- while ( inspectors[--j] ) {
- inspector = inspectors[j];
- if ( this.registry[inspector].static.typePattern.test(
annotation.name ) ) {
- matches.push( inspector );
- break;
- }
- }
- }
- return matches;
-};
-
/* Initialization */
ve.ui.inspectorFactory = new ve.ui.InspectorFactory();
diff --git a/modules/ve/ui/ve.ui.ViewRegistry.js
b/modules/ve/ui/ve.ui.ViewRegistry.js
new file mode 100644
index 0000000..49e76ac
--- /dev/null
+++ b/modules/ve/ui/ve.ui.ViewRegistry.js
@@ -0,0 +1,91 @@
+/*!
+ * VisualEditor UserInterface ViewRegistry class.
+ *
+ * @copyright 2011-2013 VisualEditor Team and others; see AUTHORS.txt
+ * @license The MIT License (MIT); see LICENSE.txt
+ */
+
+/**
+ * UserInterface view registry.
+ *
+ * @class
+ * @extends ve.Registry
+ *
+ * @constructor
+ */
+ve.ui.ViewRegistry = function VeUiViewRegistry() {
+ // Parent constructor
+ ve.Registry.call( this );
+};
+
+/* Inheritance */
+
+ve.inheritClass( ve.ui.ViewRegistry, ve.Registry );
+
+/* Methods */
+
+ve.ui.ViewRegistry.prototype.isViewRelatedToModel = function ( view, model ) {
+ var classes = view.static.modelClasses || [],
+ i = classes.length;
+
+ while ( classes[--i] ) {
+ if ( model instanceof classes[i] ) {
+ return true;
+ }
+ }
+ return false;
+};
+
+/**
+ * Get a list of views from a set of annotations.
+ *
+ * @method
+ * @param {ve.dm.AnnotationSet} annotations Annotations to be inspected
+ * @returns {string[]} Symbolic names of views that can be used to inspect
annotations
+ */
+ve.ui.ViewRegistry.prototype.getViewsForAnnotations = function ( annotations )
{
+ if ( annotations.isEmpty() ) {
+ return [];
+ }
+
+ var i, len, annotation, name, view,
+ arr = annotations.get(),
+ matches = [];
+
+ for ( i = 0, len = arr.length; i < len; i++ ) {
+ annotation = arr[i];
+ for ( name in this.registry ) {
+ view = this.registry[name];
+ if ( this.isViewRelatedToModel( view, annotation ) ) {
+ matches.push( name );
+ break;
+ }
+ }
+ }
+ return matches;
+};
+
+/**
+ * Get a list of views for a node.
+ *
+ * @method
+ * @param {ve.dm.Node} node Node to be edited
+ * @returns {string[]} Symbolic names of views that can be used to edit node
+ */
+ve.ui.ViewRegistry.prototype.getViewsForNode = function ( node ) {
+ var name, view,
+ matches = [];
+
+ for ( name in this.registry ) {
+ view = this.registry[name];
+ if ( this.isViewRelatedToModel( view, node ) ) {
+ matches.push( name );
+ break;
+ }
+ }
+ return matches;
+};
+
+/* Initialization */
+
+ve.ui.viewRegistry = new ve.ui.ViewRegistry();
--
To view, visit https://gerrit.wikimedia.org/r/59330
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic2bbcf072fdd87e5ce8a03fe1ae3e6d8d50e2593
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Trevor Parscal <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
