r98563 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r98562‎ | r98563 | r98564 >
Date:21:08, 30 September 2011
Author:johnduhart
Status:ok (Comments)
Tags:
Comment:
Followup r98500, conversion of the Javascript to use jQuery fully. It also makes the Javascript work outside of debug=1 (oops)

There's a couple of WTF moments in the code, this is really due to some issues with the PHP side of stuff. It really needs some TLC but for right now it works, I'll come back to it before 1.19 and clean it up.
Modified paths:
  • /trunk/extensions/CategoryTree/CategoryTree.php (modified) (history)
  • /trunk/extensions/CategoryTree/CategoryTreeFunctions.php (modified) (history)
  • /trunk/extensions/CategoryTree/modules/ext.categoryTree.js (modified) (history)

Diff [purge]

Index: trunk/extensions/CategoryTree/CategoryTree.php
——@@ -144,6 +144,20 @@
145145 'remoteExtPath' => 'CategoryTree/modules',
146146 'styles' => 'ext.categoryTree.css',
147147 'scripts' => 'ext.categoryTree.js',
 148+ 'messages' => array(
 149+ 'categorytree-collapse',
 150+ 'categorytree-expand',
 151+ 'categorytree-collapse-bullet',
 152+ 'categorytree-expand-bullet',
 153+ 'categorytree-load',
 154+ 'categorytree-loading',
 155+ 'categorytree-nothing-found',
 156+ 'categorytree-no-subcategories',
 157+ 'categorytree-no-parent-categories',
 158+ 'categorytree-no-pages',
 159+ 'categorytree-error',
 160+ 'categorytree-retry',
 161+ ),
148162 );
149163
150164 /**
——@@ -193,6 +207,8 @@
194208 } else {
195209 $wgHooks['OutputPageParserOutput'][] = 'efCategoryTreeParserOutput';
196210 }
 211+
 212+ $wgHooks['ResourceLoaderGetConfigVars'][] = 'efCategoryTreeGetConfigVars';
197213 }
198214
199215 function efCategoryTreeSetHooks( $parser ) {
——@@ -368,7 +384,7 @@
369385 }
370386
371387 function efCategoryTreeSkinJoinCategoryLinks( $skin, &$links, &$result ) {
372 - $embed = '<div class="CategoryTreePretendInlineMSIE CategoryTreeCategoryBarItem">';
 388+ $embed = '<div class="CategoryTreeCategoryBarItem">';
373389 $pop = '</div>';
374390 $sep = ' ';
375391
——@@ -376,3 +392,12 @@
377393
378394 return false;
379395 }
 396+
 397+function efCategoryTreeGetConfigVars( &$vars ) {
 398+ global $wgCategoryTreeCategoryPageOptions;
 399+
 400+ // Look this is pretty bad but Category tree is just whacky, it needs to be rewritten
 401+ $ct = new CategoryTree( $wgCategoryTreeCategoryPageOptions );
 402+ $vars['wgCategoryTreePageCategoryOptions'] = $ct->getOptionsAsJsStructure();
 403+ return true;
 404+}
\ No newline at end of file
Index: trunk/extensions/CategoryTree/modules/ext.categoryTree.js
——@@ -12,151 +12,144 @@
1313 * in CategoryTree.php to avoid users getting stale copies from cache.
1414 */
1515
16 -// Default messages if new code loaded with old cached page
17 -var categoryTreeErrorMsg = "Problem loading data.";
18 -var categoryTreeRetryMsg = "Please wait a moment and try again.";
 16+new ( function( $, mw ) {
 17+ /**
 18+ * Reference to this
 19+ *
 20+ * @var {this}
 21+ */
 22+ var that = this;
1923
20 -function categoryTreeNextDiv(e) {
21 - var n= e.nextSibling;
22 - while ( n && ( n.nodeType != 1 || n.nodeName != 'DIV') ) {
23 - //alert('nodeType: ' + n.nodeType + '; nodeName: ' + n.nodeName);
24 - n= n.nextSibling;
25 - }
 24+ /**
 25+ * Sets display inline to tree toggle
 26+ */
 27+ this.showToggles = function() {
 28+ $( 'span.CategoryTreeToggle' ).css( 'display', 'inline' );
 29+ };
2630
27 - return n;
28 -}
29 -
30 -function categoryTreeExpandNode(cat, options, lnk) {
31 - var div= categoryTreeNextDiv( lnk.parentNode.parentNode );
32 -
33 - div.style.display= 'block';
34 - lnk.innerHTML= categoryTreeCollapseBulletMsg;
35 - lnk.title= categoryTreeCollapseMsg;
36 - lnk.onclick= function() { categoryTreeCollapseNode(cat, options, lnk) };
37 -
38 - if (!lnk.className.match(/(^| )CategoryTreeLoaded($| )/)) {
39 - categoryTreeLoadNode(cat, options, lnk, div);
 31+ /**
 32+ * Handles clicks on the expand buttons, and calls the appropriate function
 33+ */
 34+ this.handleNode = function() {
 35+ $link = $( this );
 36+ if ( $link.data( 'ctState' ) == 'collapsed' ) {
 37+ that.expandNode( $link );
 38+ } else {
 39+ that.collapseNode( $link );
 40+ }
4041 }
41 -}
4242
43 -function categoryTreeCollapseNode(cat, options, lnk) {
44 - var div= categoryTreeNextDiv( lnk.parentNode.parentNode );
 43+ /**
 44+ * Expands a given node (loading it's children if not loaded)
 45+ *
 46+ * @param {jQuery} $link
 47+ */
 48+ this.expandNode = function( $link ) {
 49+ // Show the children node
 50+ $children = $link.parents( '.CategoryTreeItem' )
 51+ .siblings( '.CategoryTreeChildren' );
 52+ $children.show();
4553
46 - div.style.display= 'none';
47 - lnk.innerHTML= categoryTreeExpandBulletMsg;
48 - lnk.title= categoryTreeExpandMsg;
49 - lnk.onclick= function() { categoryTreeExpandNode(cat, options, lnk) }
50 -}
 54+ $link
 55+ .html( mw.msg( 'categorytree-collapse-bullet' ) )
 56+ .attr( 'title', mw.msg( 'categorytree-collapse' ) )
 57+ .data( 'ctState', 'expanded' );
5158
52 -function categoryTreeLoadNode(cat, options, lnk, div) {
53 - div.style.display= 'block';
54 - lnk.className= 'CategoryTreeLoaded';
55 - lnk.innerHTML= categoryTreeCollapseBulletMsg;
56 - lnk.title= categoryTreeCollapseMsg;
57 - lnk.onclick= function() { categoryTreeCollapseNode(cat, options, lnk) };
 59+ if ( !$link.data( 'ctLoaded' ) ) {
 60+ that.loadChildren( $link, $children );
 61+ }
 62+ };
5863
59 - categoryTreeLoadChildren(cat, options, div)
60 -}
 64+ /**
 65+ * Collapses a node
 66+ *
 67+ * @param {jQuery} $link
 68+ */
 69+ this.collapseNode = function( $link ) {
 70+ // Hide the children node
 71+ $link.parents( '.CategoryTreeItem' )
 72+ .siblings( '.CategoryTreeChildren' ).hide();
6173
62 -// FIXME Why can't this just use uneval()?
63 -function categoryTreeEncodeValue(value) {
64 - switch (typeof value) {
65 - case 'function':
66 - throw new Error("categoryTreeEncodeValue encountered a function");
67 - break;
68 - case 'string':
69 - s = '"' + value.replace(/([\\"'])/g, "\\$1") + '"';
70 - break;
71 - case 'number':
72 - case 'boolean':
73 - case 'null':
74 - s = String(value);
75 - break;
76 - case 'object':
77 - if ( !value ) {
78 - s = 'null';
79 - } else if (typeof value.length === 'number' && !(value.propertyIsEnumerable('length'))) {
80 - s = '';
81 - for (i = 0; i<value.length; i++) {
82 - v = value[i];
83 - if ( s!='' ) s += ', ';
84 - s += categoryTreeEncodeValue( v );
85 - }
86 - s = '[' + s + ']';
87 - } else {
88 - s = '';
89 - for (k in value) {
90 - v = value[k];
91 - if ( s!='' ) s += ', ';
92 - s += categoryTreeEncodeValue( k );
93 - s += ': ';
94 - s += categoryTreeEncodeValue( v );
95 - }
96 - s = '{' + s + '}';
97 - }
98 - break;
99 - default:
100 - throw new Error("categoryTreeEncodeValue encountered strange variable type " + (typeof value));
101 - }
 74+ $link
 75+ .html( mw.msg( 'categorytree-expand-bullet' ) )
 76+ .attr( 'title', mw.msg( 'categorytree-expand' ) )
 77+ .data( 'ctState', 'collapsed' );
 78+ };
10279
103 - return s;
104 -}
 80+ /**
 81+ * Loads children for a node
 82+ *
 83+ * @param {jQuery} $link
 84+ * @param {jQuery} $children
 85+ */
 86+ this.loadChildren = function( $link, $children ) {
 87+ $link.data( 'ctLoaded', true );
 88+ $children.html(
 89+ '<i class="CategoryTreeNotice">'
 90+ + mw.msg( 'categorytree-loading' ) + "</i>"
 91+ );
10592
106 -function categoryTreeLoadChildren(cat, options, div) {
107 - div.innerHTML= '<i class="CategoryTreeNotice">' + categoryTreeLoadingMsg + '</i>';
 93+ $parentTag = $link.parents( '.CategoryTreeTag' );
10894
109 - if ( typeof options == "string" ) { //hack for backward compatibility
110 - options = { mode : options };
111 - }
112 -
113 - function f( request ) {
114 - if (request.status != 200) {
115 - div.innerHTML = '<i class="CategoryTreeNotice">' + categoryTreeErrorMsg + ' </i>';
116 - var retryLink = document.createElement('a');
117 - retryLink.innerHTML = categoryTreeRetryMsg;
118 - retryLink.onclick = function() {
119 - categoryTreeLoadChildren(cat, options, div, enc);
120 - };
121 - div.appendChild(retryLink);
122 - return;
 95+ if ( $parentTag.length == 0 ) {
 96+ // Probably a CategoryPage
 97+ $parentTag = $( '<div />' )
 98+ .hide()
 99+ .data( 'ctOptions', mw.config.get( 'wgCategoryTreePageCategoryOptions' ) )
123100 }
124101
125 - result= request.responseText;
126 - result= result.replace(/^\s+|\s+$/, '');
127 -
128 - if ( result == '' ) {
129 - result= '<i class="CategoryTreeNotice">';
130 -
131 - if ( options.mode == 0 ) {
132 - result= categoryTreeNoSubcategoriesMsg;
133 - } else if ( options.mode == 10 ) {
134 - result= categoryTreeNoPagesMsg;
135 - } else if ( options.mode == 100 ) {
136 - result= categoryTreeNoParentCategoriesMsg;
137 - } else {
138 - result= categoryTreeNothingFoundMsg;
 102+ $.get(
 103+ mw.util.wikiScript(), {
 104+ action: 'ajax',
 105+ rs: 'efCategoryTreeAjaxWrapper',
 106+ rsargs: [$link.data( 'ctTitle' ), $parentTag.data( 'ctOptions' ), 'json'] // becomes &rsargs[]=arg1&rsargs[]=arg2...
139107 }
 108+ )
 109+ .success( function ( data ) {
 110+ data = data.replace(/^\s+|\s+$/, '');
 111+ data = data.replace(/##LOAD##/g, mw.msg( 'categorytree-expand' ) );
140112
141 - result+= '</i>';
142 - }
 113+ if ( data == '' ) {
 114+ switch ( $parentTag.data( 'ctMode' ) ) {
 115+ case 0:
 116+ data = mw.msg( 'categorytree-no-subcategories' );
 117+ break;
 118+ case 10:
 119+ data = mw.msg( 'categorytree-no-pages' );
 120+ break;
 121+ case 100:
 122+ data = mw.msg( 'categorytree-no-parent-categories' );
 123+ break;
 124+ default:
 125+ data = mw.msg( 'categorytree-nothing-found' );
 126+ }
143127
144 - result = result.replace(/##LOAD##/g, categoryTreeExpandMsg);
145 - div.innerHTML= result;
 128+ data = $( '<i class="CategoryTreeNotice" />' ).text( data );
 129+ }
146130
147 - categoryTreeShowToggles();
 131+ $children
 132+ .html( data )
 133+ .find( '.CategoryTreeToggle' )
 134+ .click( that.handleNode );
 135+ that.showToggles();
 136+ } )
 137+ .error( function() {
 138+ $retryLink = $( '<a />' )
 139+ .text( mw.msg( 'categorytree-retry' ) )
 140+ .attr( 'href', '#' )
 141+ .click( function() { that.loadChildren( $link, $children ) } );
 142+ $children
 143+ .text( mw.msg( 'categorytree-error' ) )
 144+ .append( $retryLink );
 145+ } );
148146 }
149147
150 - var opt = categoryTreeEncodeValue(options);
151 - sajax_do_call( "efCategoryTreeAjaxWrapper", [cat, opt, 'json'] , f );
152 -}
 148+ /**
 149+ * Register any click events
 150+ */
 151+ $( function( $ ) {
 152+ $( '.CategoryTreeToggle' ).click( that.handleNode );
153153
154 -function categoryTreeShowToggles() {
155 - var toggles = getElementsByClassName( document, 'span', 'CategoryTreeToggle' );
156 -
157 - for( var i = 0; i<toggles.length; ++i ) {
158 - toggles[i].style.display = 'inline';
159 - }
160 -}
161 -
162 -// Re-show the CategoryTreeToggles
163 -jQuery( categoryTreeShowToggles );
 154+ that.showToggles();
 155+ } );
 156+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: trunk/extensions/CategoryTree/CategoryTreeFunctions.php
——@@ -248,7 +248,7 @@
249249 }
250250
251251 function getOptionsAsJsString( $depth = NULL ) {
252 - return Xml::escapeJsString( $s );
 252+ return Xml::escapeJsString( $this->getOptionsAsJsStructure( $depth ) );
253253 }
254254
255255 function getOptionsAsUrlParameters() {
——@@ -339,6 +339,9 @@
340340 $attr['class'] = ' CategoryTreeTag';
341341 }
342342
 343+ $attr['data-ct-mode'] = $this->mOptions['mode'];
 344+ $attr['data-ct-options'] = Xml::escapeTagsOnly( $this->getOptionsAsJsStructure() );
 345+
343346 $html = '';
344347 $html .= Xml::openElement( 'div', $attr );
345348
——@@ -637,19 +640,20 @@
638641
639642 $linkattr[ 'class' ] = "CategoryTreeToggle";
640643 $linkattr['style'] = 'display: none;'; // Unhidden by JS
 644+ $linkattr['data-ct-title'] = $key;
641645
642646 if ( $children == 0 || $loadchildren ) {
643647 $tag = 'span';
644648 $txt = wfMsgNoTrans( 'categorytree-expand-bullet' );
645 - $linkattr[ 'onclick' ] = "if (this.href) this.href='javascript:void(0)'; categoryTreeExpandNode('" . Xml::escapeJsString( $key ) . "'," . $this->getOptionsAsJsStructure() . ",this);";
646649 # Don't load this message for ajax requests, so that we don't have to initialise $wgLang
647650 $linkattr[ 'title' ] = $this->mIsAjaxRequest ? '##LOAD##' : wfMsgNoTrans( 'categorytree-expand' );
 651+ $linkattr[ 'data-ct-state' ] = 'collapsed';
648652 } else {
649653 $tag = 'span';
650654 $txt = wfMsgNoTrans( 'categorytree-collapse-bullet' );
651 - $linkattr[ 'onclick' ] = "if (this.href) this.href='javascript:void(0)'; categoryTreeCollapseNode('" . Xml::escapeJsString( $key ) . "'," . $this->getOptionsAsJsStructure() . ",this);";
652655 $linkattr[ 'title' ] = wfMsgNoTrans( 'categorytree-collapse' );
653 - $linkattr[ 'class' ] .= ' CategoryTreeLoaded';
 656+ $linkattr[ 'data-ct-loaded' ] = true;
 657+ $linkattr[ 'data-ct-state' ] = 'expanded';
654658 }
655659
656660 if ( $tag == 'a' ) {

Sign-offs

UserFlagDate
Reedytested15:29, 27 January 2012 (struck 15:39, 27 January 2012)

Follow-up revisions

RevisionCommit summaryAuthorDate
r98565Followup r98563, don't need those messages anymorejohnduhart21:10, 30 September 2011
r98575Followup r98563, oops, forgot to use var for Javascript variables. Yikesjohnduhart22:13, 30 September 2011
r98702Followup r98563, improvement to javascript per CRjohnduhart19:25, 2 October 2011
r111218attempt to fix bug33989 - fix the mode parameter in api callbsitu23:41, 10 February 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r98500Basic ResourceLoader conversion, jQuery conversion and API use will come soonjohnduhart02:51, 30 September 2011

Comments

#Comment by Krinkle (talk | contribs)   17:06, 2 October 2011 
+new ( function( $, mw ) {

I'd recommend turning this into an object instead of an anonymous immediately-invoked object constructor. Nothing in here has "state", they're basically 'static' functions.

If at some point it would have state, or if you prefer to have this as a constructor/class, then assign the constructor to a variable and instantiate it later (ie. var categoryTree = function(){ .. }; categoryTree.prototype { showToggles: function(){ .. }, .. }; and instantiate it somewhere public (i.e. mw.categoryTree = new categoryTree();).

Please add a little more function documentation. 

+	 * @var {this}
+	 */
+	var that = this;

'this' is not a variable type. It would be {Function} or {Object}. This would be removed if turning it into an object or a named constructor though. 

+	/**
+	 * Handles clicks on the expand buttons, and calls the appropriate function
+	 */
+	this.handleNode = function() {
+		$link = $( this );

The reference to this here is an element node, however this is not documented anywhere, something like @context {Element} CategoryTreeToggle would make it clear that 'this' is not referring to the category tree object but to the elemtn. It also appears to not have any arguments, however when called like this .click( that.handleNode ); it would have a first arguments of @param e {jQuery.Event}. Please document this in the function, as the function format should be defined by the function, not by the caller.


Please validate through JSHint.com, there are some small potential issues with loose comparisons to 0 and empty strings (use === instead), and some missing semi-colons. 


+					data = $( '<i class="CategoryTreeNotice" />' ).text( data );

This will fail in IE as it is invalid HTML. Self-closing tags on elements that don't support this behavior cause the fragment to fail on IE6/7. Modern browsers today auto-correct it, but that's only up to a certain point. Should be valid HTML. 


+		$children.html(
+			'<i class="CategoryTreeNotice">'
+			+ mw.msg( 'categorytree-loading' ) + "</i>"
+		);

Potential html injection here. Should use .text() or mw.html.escape() instead

#Comment by Krinkle (talk | contribs)   17:10, 2 October 2011 
new ( function( $, mw ) {

I'd recommend turning this into an object instead of an anonymous immediately-invoked object constructor. Nothing in here has "state", they're basically 'static' functions. If at some point it would have state, or if you prefer to have this as a constructor/class, then assign the constructor to a variable and instantiate it later (ie. var categoryTree = function(){ .. }; categoryTree.prototype { showToggles: function(){ .. }, .. }; and instantiate it somewhere public (i.e. mw.categoryTree = new categoryTree();). </bockquote>

But since it doesn't have state, it's probably easiest to just but it into a static object var categoryTree = { showToggles: function(){ .. }, .. };.

#Comment by Johnduhart (talk | contribs)   18:06, 2 October 2011

Thank you very much for the feedback, I'm constantly looking to improve my JavaScript.

Potential html injection here. Should use .text() or mw.html.escape() instead

Are you referring the mw.msg() call?

#Comment by Krinkle (talk | contribs)   02:04, 3 October 2011

Yes. There is a raw message being inserted between other html. Either use $('<li.....>').text( mw.msg() ); Or create a message object with mw.message( 'key' ) and call mw.message( 'key' ).escaped(). Both are useful depending on what the surrounding code looks like.

#Comment by Krinkle (talk | contribs)   02:04, 3 October 2011

Yes. There is a raw message being inserted between other html. Either use $('<li.....>').text( mw.msg() ); Or create a message object with mw.message( 'key' ) and call mw.message( 'key' ).escaped(). Both are useful depending on what the surrounding code looks like.

#Comment by Johnduhart (talk | contribs)   20:58, 15 October 2011

Was fixed in r98702

#Comment by OverlordQ (talk | contribs)   19:34, 21 October 2011

Changed the output but didn't update the parser tests

#Comment by Johnduhart (talk | contribs)   20:31, 21 October 2011

What parser tests?

#Comment by OverlordQ (talk | contribs)   23:57, 21 October 2011

well, i had a brainfart moment, the tests are actually in Cite for some reason. Just saw it was CatTree that changed.

http://svn.wikimedia.org/viewvc/mediawiki/trunk/extensions/Cite/citeCatTreeParserTests.txt

still should probably be updated by somebody who knows what the proper output should be.

#Comment by Nikerabbit (talk | contribs)   09:57, 5 January 2012

Don't you want to use .text here and elsewhere: 

+			.html( mw.msg( 'categorytree-collapse-bullet' ) )
#Comment by Catrope (talk | contribs)   18:04, 9 January 2012

Did you ever "come back before 1.19 and clean it up" like you promised in the commit message?

#Comment by Reedy (talk | contribs)   15:33, 27 January 2012

See bug 33989, might be this revision at fault

#Comment by Brion VIBBER (talk | contribs)   19:21, 14 February 2012

was resolved r111218

#Comment by Brion VIBBER (talk | contribs)   19:21, 14 February 2012

Issues appear to have been resolved.

Status & tagging log