jenkins-bot has submitted this change and it was merged.
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 .docs/categories.json
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
16 files changed, 141 insertions(+), 126 deletions(-)
Approvals:
Robmoen: Looks good to me, approved
jenkins-bot: Verified
diff --git a/.docs/categories.json b/.docs/categories.json
index 0c6b621..65df31c 100644
--- a/.docs/categories.json
+++ b/.docs/categories.json
@@ -112,6 +112,10 @@
"classes": ["ve.ui.*Factory"]
},
{
+ "name": "Registries",
+ "classes": ["ve.ui.*Registry"]
+ },
+ {
"name": "Tools",
"classes": ["ve.ui.*Tool"]
},
diff --git a/VisualEditor.php b/VisualEditor.php
index 52fae40..a8ee9c8 100644
--- a/VisualEditor.php
+++ b/VisualEditor.php
@@ -341,6 +341,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 a27e53f..ef28393 100644
--- a/modules/ve/actions/ve.AnnotationAction.js
+++ b/modules/ve/actions/ve.AnnotationAction.js
@@ -79,15 +79,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 3e6ff43..c6176e8 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 73bb34a..9368d33 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;
@@ -227,3 +227,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 500929d..af12b19 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( null, false ),
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,15 +183,15 @@
}
}
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();
if ( this.toolbar ) {
@@ -200,7 +200,7 @@
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: merged
Gerrit-Change-Id: Ic2bbcf072fdd87e5ce8a03fe1ae3e6d8d50e2593
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Trevor Parscal <[email protected]>
Gerrit-Reviewer: Robmoen <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
