Daniel Werner has uploaded a new change for review.
https://gerrit.wikimedia.org/r/76744
Change subject: valueview experts have a clear definition of destroy now
......................................................................
valueview experts have a clear definition of destroy now
- documented destroy properly, removed related TODOs
- implemented missing destroy methods
- added tests for destroy to testExpert and improved some other existing tests
Change-Id: I634c67b27e30f3b8d3a3202f1908b9752aded7a3
---
M ValueView/resources/jquery.valueview/valueview.Expert.js
M
ValueView/resources/jquery.valueview/valueview.experts/experts.GlobeCoordinateInput.js
M ValueView/resources/jquery.valueview/valueview.experts/experts.StringValue.js
M ValueView/resources/jquery.valueview/valueview.experts/experts.TimeInput.js
M ValueView/resources/jquery.valueview/valueview.valueview.js
M ValueView/tests/qunit/jquery.valueview/valueview.tests.testExpert.js
6 files changed, 116 insertions(+), 37 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/DataValues
refs/changes/44/76744/1
diff --git a/ValueView/resources/jquery.valueview/valueview.Expert.js
b/ValueView/resources/jquery.valueview/valueview.Expert.js
index da531cb..56c532b 100644
--- a/ValueView/resources/jquery.valueview/valueview.Expert.js
+++ b/ValueView/resources/jquery.valueview/valueview.Expert.js
@@ -88,7 +88,7 @@
if( viewPortNode instanceof $
&& viewPortNode.length === 1
) {
- viewPortNode = viewPortNode[0];
+ viewPortNode = viewPortNode.get( 0 );
}
if( !( viewPortNode.nodeType ) ) { // IE8 can't check for
instanceof HTMLELement
@@ -158,18 +158,25 @@
_init: function() {},
/**
- * Gets called when the valueview's destroy function is called.
+ * Destroys the expert. All generated viewport output is being
deleted and all resources
+ * (private members, events handlers) will be released.
*
- * TODO: think about and document definition of this destroy.
What is the destroy supposed
- * to do exactly? E.g. when having an expert responsible for
displaying an input, should
- * the destroy leave the input or load an expert for
displaying the value statically first?
+ * This will not preserve the plain text of the last
represented value as one might expect
+ * when thinking about the common jQuery.Widget's behavior.
This is mostly because it is
+ * not the Expert's responsibility to be able to serve a plain
text representation of the
+ * value. If the value should be represented as plain text
after the expert's construction,
+ * let the responsible controller simply use a value formatter
for that.
*
* @since 0.1
*/
destroy: function() {
- this.$viewPort.removeClass( this.uiBaseClass );
+ this.$viewPort.removeClass( this.uiBaseClass ).empty();
+
this.$viewPort = null;
this._viewState = null;
+ this._viewNotifier = null;
+ this._messageProvider = null;
+ this._options = null;
},
/**
diff --git
a/ValueView/resources/jquery.valueview/valueview.experts/experts.GlobeCoordinateInput.js
b/ValueView/resources/jquery.valueview/valueview.experts/experts.GlobeCoordinateInput.js
index 1474c86..aee4ca3 100644
---
a/ValueView/resources/jquery.valueview/valueview.experts/experts.GlobeCoordinateInput.js
+++
b/ValueView/resources/jquery.valueview/valueview.experts/experts.GlobeCoordinateInput.js
@@ -148,18 +148,32 @@
* @see jQuery.valueview.Expert.destroy
*/
destroy: function() {
- this.$precision.data( 'listrotator' ).destroy();
- this.$precision.remove();
- this.$precisionContainer.remove();
+ if( !this.$input ) {
+ return; // destroyed already
+ }
- var previewElement = this.preview.element;
- this.preview.destroy();
- previewElement.remove();
+ if( this.preview ) {
+ this.preview.destroy();
+ this.preview.element.remove();
+ }
- this.$input.data( 'inputextender' ).destroy();
- this.$input.remove();
+ var listRotator = this.$precision.data( 'listrotator' );
+ if( listRotator ) {
+ listRotator.destroy();
+ }
- PARENT.prototype.destroy.call( this );
+ var inputExtender = this.$input.data( 'inputextender' );
+ if( inputExtender ) {
+ inputExtender.destroy();
+ }
+
+ this.$input.off( 'eachchange' );
+
+ this.$input = null;
+ this.$precision = null;
+ this.$precisionContainer = null;
+
+ PARENT.prototype.destroy.call( this ); // empties
viewport
},
/**
diff --git
a/ValueView/resources/jquery.valueview/valueview.experts/experts.StringValue.js
b/ValueView/resources/jquery.valueview/valueview.experts/experts.StringValue.js
index 39d1eea..8e6581f 100644
---
a/ValueView/resources/jquery.valueview/valueview.experts/experts.StringValue.js
+++
b/ValueView/resources/jquery.valueview/valueview.experts/experts.StringValue.js
@@ -52,9 +52,19 @@
* @see jQuery.valueview.Expert.destroy
*/
destroy: function() {
- PARENT.prototype.destroy.call( this );
- // TODO: destroy input. Should unbind "eachchange"
event and remove "inputAutoExpand"
- // functionality. Also see valueview.Expert's destroy
TODO.
+ if( !this.$input ) {
+ return; // destroyed already
+ }
+
+ var inputExtender = this.$input.data( 'inputextender' );
+ if( inputExtender ) {
+ inputExtender.destroy();
+ }
+
+ this.$input.off( 'eachchange' );
+ this.$input = null;
+
+ PARENT.prototype.destroy.call( this ); // empties
viewport
},
/**
diff --git
a/ValueView/resources/jquery.valueview/valueview.experts/experts.TimeInput.js
b/ValueView/resources/jquery.valueview/valueview.experts/experts.TimeInput.js
index 06d0dc2..7935158 100644
---
a/ValueView/resources/jquery.valueview/valueview.experts/experts.TimeInput.js
+++
b/ValueView/resources/jquery.valueview/valueview.experts/experts.TimeInput.js
@@ -245,25 +245,36 @@
* @see jQuery.valueview.Expert.destroy
*/
destroy: function() {
- this.$precision.data( 'listrotator' ).destroy();
- this.$precision.remove();
- this.$precisionContainer.remove();
+ if( !this.$input ) {
+ return; // destroyed already
+ }
- this.$calendar.data( 'listrotator' ).destroy();
- this.$calendar.remove();
- this.$calendarContainer.remove();
+ if( this.preview ) {
+ this.preview.destroy();
+ this.preview.element.remove();
+ }
- this.$calendarhint.remove();
+ var timeInput = this.$input.data( 'timeinput' );
+ if( timeInput ) {
+ timeInput.destroy();
+ }
- var previewElement = this.preview.element;
- this.preview.destroy();
- previewElement.remove();
+ var inputExtender = this.$input.data( 'inputextender' );
+ if( inputExtender ) {
+ // Explicitly destroy calendar and precision
list rotators:
+ inputExtender.$extension.find(
':ui-listrotator' ).listrotator( 'destroy' );
+ inputExtender.destroy();
+ }
- this.$input.data( 'inputextender' ).destroy();
- this.$input.data( 'timeinput' ).destroy();
- this.$input.remove();
+ this.$input.off( 'eachchange' );
- PARENT.prototype.destroy.call( this );
+ this.$input = null;
+ this.$precision = null;
+ this.$precisionContainer = null;
+ this.$calendar = null;
+ this.$calendarContainer = null;
+
+ PARENT.prototype.destroy.call( this ); // empties
viewport
},
/**
diff --git a/ValueView/resources/jquery.valueview/valueview.valueview.js
b/ValueView/resources/jquery.valueview/valueview.valueview.js
index fecda82..8ac1ec7 100644
--- a/ValueView/resources/jquery.valueview/valueview.valueview.js
+++ b/ValueView/resources/jquery.valueview/valueview.valueview.js
@@ -168,6 +168,10 @@
this._expert = null;
}
+ // TODO: destroying the expert will leave no plain text of the
last value. Once we
+ // implemented formatters, think about leaving a formatted
text here. On the other hand,
+ // formatting will be assynchronous which would be a problem
in this situation.
+
return PARENT.prototype.destroy.call( this );
},
diff --git
a/ValueView/tests/qunit/jquery.valueview/valueview.tests.testExpert.js
b/ValueView/tests/qunit/jquery.valueview/valueview.tests.testExpert.js
index 6d4dac0..0e91706 100644
--- a/ValueView/tests/qunit/jquery.valueview/valueview.tests.testExpert.js
+++ b/ValueView/tests/qunit/jquery.valueview/valueview.tests.testExpert.js
@@ -58,7 +58,7 @@
}, {
title: 'instance with notifier',
args: [
- $( '<div/>' ),
+ document.createElement( 'div' ),
new valueview.MockViewState(),
new Notifier()
]
@@ -126,9 +126,32 @@
args.expert instanceof valueview.Expert,
'expert instance of jQuery.valueview.Expert'
);
+
+ var viewPortArg = args.constructorArgs[0],
+ viewPortElement = viewPortArg instanceof $ ?
viewPortArg.get( 0 ) : viewPortArg;
+
+ assert.ok(
+ args.expert.$viewPort.get( 0 ) === viewPortElement,
+ 'View port node given to constructor is used as the
actual view port of the expert'
+ );
} );
- expertCases.test( 'parser', function( args, assert ) {
+ expertCasesTestAndCleanup( 'destroy', function( args, assert ) {
+ var $viewPort = $( args.constructorArgs[0] );
+
+ args.expert.destroy();
+
+ assert.ok(
+ $viewPort.children().length === 0 && $viewPort.text()
=== '',
+ 'Viewport is empty after expert\'s destruction'
+ );
+
+ args.expert.destroy();
+ assert.ok( true, 'Calling destroy() again will not lead to
unexpected error.' );
+
+ } );
+
+ expertCasesTestAndCleanup( 'parser', function( args, assert ) {
var valueParser = args.expert.parser();
assert.ok(
@@ -253,16 +276,26 @@
} );
} );
- var expertCasesMemberCallTest = function( memberName ) {
- expertCases.test( memberName, function( args, assert ) {
+ var expertCasesMemberCallTest = function( memberName,
additionalAssertionsFn ) {
+ expertCasesTestAndCleanup( memberName, function( args, assert )
{
args.expert[ memberName ]();
assert.ok(
true,
memberName + '() has been called'
);
+ if( additionalAssertionsFn ) {
+ additionalAssertionsFn( args, assert );
+ }
} );
};
- expertCasesMemberCallTest( 'draw' );
+ expertCasesMemberCallTest( 'draw', function( args, assert ) {
+ var $viewPort = $( args.constructorArgs[0] );
+
+ assert.ok(
+ $viewPort.text() !== '' || $viewPort.children.length >
0,
+ 'Viewport node is not empty after draw()'
+ );
+ } );
expertCasesMemberCallTest( 'focus' );
expertCasesMemberCallTest( 'blur' );
--
To view, visit https://gerrit.wikimedia.org/r/76744
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I634c67b27e30f3b8d3a3202f1908b9752aded7a3
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/DataValues
Gerrit-Branch: master
Gerrit-Owner: Daniel Werner <daniel.wer...@wikimedia.de>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
