Trevor Parscal has uploaded a new change for review.
https://gerrit.wikimedia.org/r/57172
Change subject: No more confusing boolean argument for closing windows
......................................................................
No more confusing boolean argument for closing windows
window.close( true ) thing sucked, and was being named and used inconsistently
throughout the code.
The new approach uses an action string, so it looks more like window.close(
'accept' ) or window.close( 'back' ). This makes it easy to steer the behavior
at any point in the window close code path.
Most importantly for the link inspector, this allows us to now restore the
previous selection when the user presses escape or clicks the back button,
while still moving the cursor to the end and collapsing the selection upon
pressing enter and allowing removal by clicking the trash can.
This commit also cleans some things up, like the various ways we have to close
an inspector which all seem useless because we wouldn't want to just randomly
close an inspector on someone. An inspector should be closed only when the user
has dealt with it.
ve.InspectorAction.js
* Removed close method
ve.ui.LinkInspector.js
* Updated documentation
* Passing action to parent method
* Updated logic to deal with change from "remove" to "action" argument
* Added sélection restauration on "back" action
ve.ui.Context.js
* Added action to call to close
* Removed closeInspector method
ve.ui.Dialog.js
* Moved event handlers to the top
* Added actions to calls to close
* Added click block event handler to prevent focus changes
ve.ui.Inspector.js
* Added actions to calls to close
* Added storing of previous selection - this is different from initialSelection
because it's captured before the selection is modified by setup
ve.ui.Window.js
* Updated documentation
* Updated argument name from "remove" to "action"
ve.ui.WindowSet.js
* Updated documentation
* Removed auto-close, replaced it with error if trying to open a window when
another is already open
* Removed close method
Change-Id: Ie8f72504177dd6ba169fdddbb776fd5397b831c4
---
M modules/ve/actions/ve.InspectorAction.js
M modules/ve/ui/inspectors/ve.ui.LinkInspector.js
M modules/ve/ui/ve.ui.Context.js
M modules/ve/ui/ve.ui.Dialog.js
M modules/ve/ui/ve.ui.Inspector.js
M modules/ve/ui/ve.ui.Window.js
M modules/ve/ui/ve.ui.WindowSet.js
7 files changed, 64 insertions(+), 72 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/VisualEditor
refs/changes/72/57172/1
diff --git a/modules/ve/actions/ve.InspectorAction.js
b/modules/ve/actions/ve.InspectorAction.js
index c911997..462a32c 100644
--- a/modules/ve/actions/ve.InspectorAction.js
+++ b/modules/ve/actions/ve.InspectorAction.js
@@ -44,16 +44,6 @@
this.surface.getContext().openInspector( name );
};
-/**
- * Close any open inspector.
- *
- * @method
- * @param {boolean} remove Remove annotation being inspected
- */
-ve.InspectorAction.prototype.close = function ( remove ) {
- this.surface.getContext().closeInspector( remove );
-};
-
/* Registration */
ve.actionFactory.register( 'inspector', ve.InspectorAction );
diff --git a/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
b/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
index b45a359..7fae952 100644
--- a/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
+++ b/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
@@ -124,17 +124,18 @@
* Handle the inspector being opened.
*
* @method
- * @param {boolean} remove Annotation should be removed
+ * @param {string} action Action that caused the window to be closed
*/
-ve.ui.LinkInspector.prototype.onClose = function ( remove ) {
+ve.ui.LinkInspector.prototype.onClose = function ( action ) {
// Call parent method
- ve.ui.Inspector.prototype.onClose.call( this );
+ ve.ui.Inspector.prototype.onClose.call( this, action );
var i, len, annotations, selection,
insert = false,
undo = false,
clear = false,
set = false,
+ remove = action === 'remove',
target = this.targetInput.getValue(),
annotation = this.targetInput.getAnnotation(),
fragment = this.surface.getModel().getFragment(
this.initialSelection, false );
@@ -179,6 +180,9 @@
// Apply new annotation
fragment.annotateContent( 'set', annotation );
}
+ if ( action === 'back' ) {
+ selection = this.previousSelection;
+ }
// Selection changes may have occured in the insertion and annotation
hullabaloo - restore it
this.surface.execute(
'content', 'select', selection || new ve.Range(
fragment.getRange().end )
diff --git a/modules/ve/ui/ve.ui.Context.js b/modules/ve/ui/ve.ui.Context.js
index 020ba8a..4c1f616 100644
--- a/modules/ve/ui/ve.ui.Context.js
+++ b/modules/ve/ui/ve.ui.Context.js
@@ -274,7 +274,7 @@
if ( inspector ) {
// This will recurse, but inspector will be undefined next time
- inspector.close();
+ inspector.close( 'hide' );
return this;
}
@@ -295,17 +295,5 @@
*/
ve.ui.Context.prototype.openInspector = function ( name ) {
this.inspectors.open( name );
- return this;
-};
-
-/**
- * Closes currently open inspector.
- *
- * @method
- * @param {boolean} remove Remove annotation while closing
- * @chainable
- */
-ve.ui.Context.prototype.closeInspector = function ( remove ) {
- this.inspectors.close( remove );
return this;
};
diff --git a/modules/ve/ui/ve.ui.Dialog.js b/modules/ve/ui/ve.ui.Dialog.js
index 4aacce3..14c0a38 100644
--- a/modules/ve/ui/ve.ui.Dialog.js
+++ b/modules/ve/ui/ve.ui.Dialog.js
@@ -38,7 +38,35 @@
/* Methods */
/**
- * Handle frame ready events.
+ * Handle mouse down events.
+ *
+ * @method
+ * @param {jQuery.Event} e Mouse down event
+ */
+ve.ui.Dialog.prototype.onMouseDown = function () {
+ return false;
+};
+
+/**
+ * Handle cancel button click events.
+ *
+ * @method
+ */
+ve.ui.Dialog.prototype.onCancelButtonClick = function () {
+ this.close( 'cancel' );
+};
+
+/**
+ * Handle apply button click events.
+ *
+ * @method
+ */
+ve.ui.Dialog.prototype.onApplyButtonClick = function () {
+ this.close( 'apply' );
+};
+
+/**
+ * Initialize frame contents.
*
* @method
*/
@@ -60,22 +88,4 @@
// Initialization
this.$head.append( this.applyButton.$, this.cancelButton.$ );
-};
-
-/**
- * Handle cancel button click events.
- *
- * @method
- */
-ve.ui.Dialog.prototype.onCancelButtonClick = function () {
- this.close();
-};
-
-/**
- * Handle apply button click events.
- *
- * @method
- */
-ve.ui.Dialog.prototype.onApplyButtonClick = function () {
- this.close( true );
};
diff --git a/modules/ve/ui/ve.ui.Inspector.js b/modules/ve/ui/ve.ui.Inspector.js
index 71a17b6..7139285 100644
--- a/modules/ve/ui/ve.ui.Inspector.js
+++ b/modules/ve/ui/ve.ui.Inspector.js
@@ -85,7 +85,7 @@
* @method
*/
ve.ui.Inspector.prototype.onCloseButtonClick = function () {
- this.close();
+ this.close( 'back' );
};
/**
@@ -94,7 +94,7 @@
* @method
*/
ve.ui.Inspector.prototype.onRemoveButtonClick = function() {
- this.close( true );
+ this.close( 'remove' );
};
/**
@@ -104,7 +104,7 @@
* @param {jQuery.Event} e Form submit event
*/
ve.ui.Inspector.prototype.onFormSubmit = function () {
- this.close();
+ this.close( 'apply' );
return false;
};
@@ -117,13 +117,22 @@
ve.ui.Inspector.prototype.onFormKeyDown = function ( e ) {
// Escape
if ( e.which === 27 ) {
- this.close();
+ this.close( 'back' );
return false;
}
};
/**
- * Handle inspector initialize events.
+ * Handle inspector setup events.
+ *
+ * @method
+ */
+ve.ui.Inspector.prototype.onSetup = function () {
+ this.previousSelection = this.surface.getModel().getSelection();
+};
+
+/**
+ * Handle inspector open events.
*
* @method
*/
diff --git a/modules/ve/ui/ve.ui.Window.js b/modules/ve/ui/ve.ui.Window.js
index 9d5fe5e..93aacab 100644
--- a/modules/ve/ui/ve.ui.Window.js
+++ b/modules/ve/ui/ve.ui.Window.js
@@ -68,7 +68,7 @@
/**
* @event close
* @param {ve.ui.Window} win Window that's been closed
- * @param {boolean} accept Changes have been accepted
+ * @param {string} action Action that caused the window to be closed
*/
/* Static Properties */
@@ -172,7 +172,7 @@
* To be notified after this method is called, listen to the `close` event.
*
* @method
- * @param {boolean} accept Changes have been accepted
+ * @param {string} action Action that caused the window to be closed
*/
ve.ui.Window.prototype.onClose = function () {
// This is a stub, override functionality in child classes
@@ -272,18 +272,18 @@
* loop.
*
* @method
- * @param {boolean} accept Changes have been accepted
+ * @param {boolean} action Action that caused the window to be closed
* @emits close
*/
-ve.ui.Window.prototype.close = function ( remove ) {
+ve.ui.Window.prototype.close = function ( action ) {
if ( !this.closing ) {
this.closing = true;
this.$.hide();
this.visible = false;
- this.onClose( remove );
+ this.onClose( action );
this.closing = false;
this.frame.$content.find( ':focus' ).blur();
this.surface.getView().getDocument().getDocumentNode().$.focus();
- this.emit( 'close', remove );
+ this.emit( 'close', action );
}
};
diff --git a/modules/ve/ui/ve.ui.WindowSet.js b/modules/ve/ui/ve.ui.WindowSet.js
index 618ca9d..300fe61 100644
--- a/modules/ve/ui/ve.ui.WindowSet.js
+++ b/modules/ve/ui/ve.ui.WindowSet.js
@@ -48,7 +48,7 @@
/**
* @event close
* @param {ve.ui.Window} win Window that's been closed
- * @param {boolean} accept Changes have been accepted
+ * @param {string} action Action that caused the window to be closed
*/
/* Methods */
@@ -102,6 +102,8 @@
/**
* Opens a given window.
*
+ * Any already open dialog will be closed.
+ *
* @method
* @param {string} name Symbolic name of window
* @chainable
@@ -112,6 +114,9 @@
if ( !this.factory.lookup( name ) ) {
throw new Error( 'Unknown window: ' + name );
}
+ if ( this.currentWindow ) {
+ throw new Error( 'Cannot open another window while another one
is active' );
+ }
if ( !( name in this.windows ) ) {
win = this.windows[name] = this.factory.create( name,
this.surface );
win.on( 'setup', ve.bind( this.onWindowSetup, this, win ) );
@@ -119,22 +124,8 @@
win.on( 'close', ve.bind( this.onWindowClose, this, win ) );
this.$.append( win.$ );
}
- this.close();
+
this.windows[name].open();
- return this;
-};
-
-/**
- * Closes currently open window.
- *
- * @method
- * @param {boolean} accept Changes have been accepted
- * @chainable
- */
-ve.ui.WindowSet.prototype.close = function ( accept ) {
- if ( this.currentWindow ) {
- this.currentWindow.close( accept );
- }
return this;
};
--
To view, visit https://gerrit.wikimedia.org/r/57172
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie8f72504177dd6ba169fdddbb776fd5397b831c4
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Trevor Parscal <tpars...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
