r91083 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r91082‎ | r91083 | r91084 >
Date:17:29, 29 June 2011
Author:krinkle
Status:resolved (Comments)
Tags:
Comment:
WikiLove minor fixes:
* Trailing comma's in object literals brakes IE (in rememberInputData and currentRememberData), unlike PHP.
* Using fast strict comparison to undefined instead of typeof + string comparison
* Whitespace (space before opening parenthesis after "if")
* Fix reference to deprecated global ('skin' and 'wgFormattedNamespaces')
* Fix missing case in addFilePrefix (NS_FILE has "File" and "Image" as alias, a certain localization as primary one but also a localiztion of "Image" exists as alias (a total of four)). The latter case was still missing. Fixing by using wgNamespaceIds. Also made it case- and whitespace-insensitive (like the Parser is)
* Removed redundant IIFE wrapper and used the closure we already have.


Local configuration BREAKING CHANGE

I made an attempt to re-implement a local config that doesn't require declaring all of the options, and doesn't require the init-call to be part of the local config (which is dangerous. one syntax error and it wont init at all).

Solution:
- Making it into an object literal (instead of a function)
- Always loading the default configuration
- Moving init into init-module
- Loading local config with mw.loader.using instead of a dependancy in the module definition.

Result:
- Local config can be a copy-paste of defaultOptions.js and it will work
- But (here's the nice part) one can also just do this in MediaWiki:WikiLove.js:

$.wikiLoveOptions.types.barnstar.name='Awesome Originals';

And have it work!

Tested in Safari 5, Firefox 3.6 + 4, Opera 11
Modified paths:
  • /trunk/extensions/WikiLove/WikiLove.hooks.php (modified) (history)
  • /trunk/extensions/WikiLove/WikiLove.local.php (modified) (history)
  • /trunk/extensions/WikiLove/WikiLove.php (modified) (history)
  • /trunk/extensions/WikiLove/modules/ext.wikiLove/ext.wikiLove.core.js (modified) (history)
  • /trunk/extensions/WikiLove/modules/ext.wikiLove/ext.wikiLove.defaultOptions.js (modified) (history)
  • /trunk/extensions/WikiLove/modules/ext.wikiLove/ext.wikiLove.init.js (added) (history)

Diff [purge]

Index: trunk/extensions/WikiLove/WikiLove.php
——@@ -86,14 +86,19 @@
8787 'wikilove-type-makeyourown',
8888 );
8989
90 -// resources
91 -// it is much better to have a chain like: startup -> default -> local -> init,
92 -// but because of this bug that isn't possible right now: https://bugzilla.wikimedia.org/29608
 90+// Because of bug 29608 we can't make a dependancy on a wiki module yet
 91+// For now using 'using' to load the wiki module from within init.
9392 $wgResourceModules += array(
9493 'ext.wikiLove.icon' => $extWikiLoveTpl + array(
9594 'styles' => 'ext.wikiLove.icon.css',
9695 'position' => 'top',
9796 ),
 97+ 'ext.wikiLove.defaultOptions' => $extWikiLoveTpl + array(
 98+ 'scripts' => array(
 99+ 'ext.wikiLove.defaultOptions.js',
 100+ ),
 101+ 'messages' => $wgWikiLoveOptionMessages,
 102+ ),
98103 'ext.wikiLove.startup' => $extWikiLoveTpl + array(
99104 'scripts' => array(
100105 'ext.wikiLove.core.js',
——@@ -136,6 +141,7 @@
137142 'wikilove-err-send-api',
138143 ),
139144 'dependencies' => array(
 145+ 'ext.wikiLove.defaultOptions',
140146 'jquery.ui.dialog',
141147 'jquery.ui.button',
142148 'jquery.localize',
——@@ -144,17 +150,14 @@
145151 ),
146152 'ext.wikiLove.local' => array(
147153 'class' => 'WikiLoveLocal',
148 - /* for information only, this is actually in the class!
149 - 'messages' => $wgWikiLoveOptionMessages,
150 - 'dependencies' => 'ext.wikiLove.startup'
151 - */
152154 ),
153 - 'ext.wikiLove.defaultOptions' => $extWikiLoveTpl + array(
 155+ 'ext.wikiLove.init' => $extWikiLoveTpl + array(
154156 'scripts' => array(
155 - 'ext.wikiLove.defaultOptions.js',
 157+ 'ext.wikiLove.init.js',
156158 ),
157 - 'messages' => $wgWikiLoveOptionMessages,
158 - 'dependencies' => 'ext.wikiLove.startup'
 159+ 'dependencies' => array(
 160+ 'ext.wikiLove.startup',
 161+ ),
159162 ),
160163 'jquery.elastic' => array(
161164 'localBasePath' => dirname( __FILE__ ) . '/modules/jquery.elastic',
Index: trunk/extensions/WikiLove/WikiLove.hooks.php
——@@ -59,18 +59,7 @@
6060
6161 $title = self::getUserTalkPage( $skin->getTitle() );
6262 if ( !is_null( $title ) ) {
63 - $out->addModules( 'ext.wikiLove.icon' );
64 -
65 - // it is much better to have a chain like: startup -> default -> local -> init,
66 - // but because of this bug that isn't possible right now: https://bugzilla.wikimedia.org/29608
67 - $optionsTitle = Title::newFromText( "MediaWiki:WikiLove.js" );
68 - if( $optionsTitle->exists() && $optionsTitle->isCssOrJsPage() ) {
69 - $out->addModules( 'ext.wikiLove.local' );
70 - }
71 - else {
72 - $out->addModules( 'ext.wikiLove.defaultOptions' );
73 - }
74 -
 63+ $out->addModules( array( 'ext.wikiLove.icon', 'ext.wikiLove.init' ) );
7564 self::$recipient = $title->getBaseText();
7665 }
7766 return true;
Index: trunk/extensions/WikiLove/WikiLove.local.php
——@@ -9,12 +9,6 @@
1010 );
1111 }
1212
13 - public function getDependencies() {
14 - return array(
15 - 'ext.wikiLove.startup',
16 - );
17 - }
18 -
1913 public function getMessages() {
2014 global $wgWikiLoveOptionMessages;
2115 return $wgWikiLoveOptionMessages;
Index: trunk/extensions/WikiLove/modules/ext.wikiLove/ext.wikiLove.defaultOptions.js
——@@ -1,5 +1,5 @@
22 ( function( $ ) {
3 -$.wikiLoveOptions = function() { return {
 3+$.wikiLoveOptions = {
44 defaultText: '{| style="background-color: $5; border: 1px solid $6;"\n\
55 |rowspan="2" style="vertical-align: middle; padding: 5px;" | [[$3|$4]]\n\
66 |style="font-size: x-large; padding: 3px; height: 1.5em;" | \'\'\'$2\'\'\'\n\
——@@ -388,10 +388,6 @@
389389 icon: mw.config.get( 'wgExtensionAssetsPath' ) + '/WikiLove/modules/ext.wikiLove/images/icons/wikilove-icon-create.png'
390390 }
391391 }
392 -}; };
 392+};
393393
394 -if( typeof $.wikiLove != 'undefined' ) $.wikiLove.init(); // this is required when copying this file to MediaWiki:WikiLove.js
395 -// because of https://bugzilla.wikimedia.org/29608 ; please leave it here as it does no harm being executed in defaultOptions.js
396 -// and it may be confusing if it is required to uncomment it when copying this to MediaWiki:WikiLove.js
397 -
398394 } )( jQuery );
Index: trunk/extensions/WikiLove/modules/ext.wikiLove/ext.wikiLove.init.js
——@@ -0,0 +1 @@
 2+mw.loader.using( 'ext.wikiLove.local', jQuery.wikiLove.init );
Property changes on: trunk/extensions/WikiLove/modules/ext.wikiLove/ext.wikiLove.init.js
___________________________________________________________________
Added: svn:eol-style
13 + native
Index: trunk/extensions/WikiLove/modules/ext.wikiLove/ext.wikiLove.core.js
——@@ -1,5 +1,4 @@
22 ( function( $ ) {
3 -$.wikiLove = (function(){
43
54 var options = {}, // options modifiable by the user
65 $dialog = null, // dialog jQuery object
——@@ -9,8 +8,8 @@
109 rememberData = null, // input data to remember when switching types or subtypes
1110 emailable = false,
1211 gallery = {};
13 -
14 -return {
 12+
 13+$.wikiLove = {
1514 /*
1615 * Opens the dialog and builds it if necessary.
1716 */
——@@ -85,7 +84,7 @@
8685 <span class="mw-wikilove-note" id="mw-wikilove-image-note"><html:msg key="wikilove-image-example"/></span>\
8786 <input type="text" class="text" id="mw-wikilove-image"/>\
8887 <div id="mw-wikilove-commons-text">\
89 - ' + mw.msg( 'wikilove-commons-text', '<a href="' + mw.msg( 'wikilove-commons-url' ) +'" target="_blank">' + mw.msg( 'wikilove-commons-link' ) +'</a>' ) + '\
 88+ ' + mw.msg( 'wikilove-commons-text', '<a href="' + mw.msg( 'wikilove-commons-url' ) + '" target="_blank">' + mw.msg( 'wikilove-commons-link' ) +'</a>' ) + '\
9089 </div>\
9190 <label for="mw-wikilove-message" id="mw-wikilove-message-label"><html:msg key="wikilove-enter-message"/></label>\
9291 <span class="mw-wikilove-note" id="mw-wikilove-message-note"><html:msg key="wikilove-omit-sig"/></span>\
——@@ -119,12 +118,21 @@
120119 resizable: false
121120 });
122121
123 - if ( skin == 'vector' ) {
124 - $( '#mw-wikilove-button-preview' ).button( { label: mw.msg( 'wikilove-button-preview' ), icons: { primary:'ui-icon-search' } } );
 122+ if ( mw.config.get( 'skin' ) == 'vector' ) {
 123+ $( '#mw-wikilove-button-preview' ).button( {
 124+ label: mw.msg( 'wikilove-button-preview' ),
 125+ icons: {
 126+ primary:'ui-icon-search'
 127+ }
 128+ } );
125129 } else {
126 - $( '#mw-wikilove-button-preview' ).button( { label: mw.msg( 'wikilove-button-preview' ) } );
 130+ $( '#mw-wikilove-button-preview' ).button( {
 131+ label: mw.msg( 'wikilove-button-preview' )
 132+ } );
127133 }
128 - $( '#mw-wikilove-button-send' ).button( { label: mw.msg( 'wikilove-button-send' ) } );
 134+ $( '#mw-wikilove-button-send' ).button( {
 135+ label: mw.msg( 'wikilove-button-send' )
 136+ } );
129137 $( '#mw-wikilove-add-details' ).hide();
130138 $( '#mw-wikilove-preview' ).hide();
131139 $( '#mw-wikilove-types' ).replaceWith( $typeList );
——@@ -135,7 +143,9 @@
136144 $( '#mw-wikilove-send-form' ).click( $.wikiLove.submitSend );
137145 $( '#mw-wikilove-message' ).elastic(); // have the message textarea grow automatically
138146
139 - if ( mw.config.get( 'wikilove-anon' ) === 0 ) $( '#mw-wikilove-anon-warning' ).hide();
 147+ if ( mw.config.get( 'wikilove-anon' ) === 0 ) {
 148+ $( '#mw-wikilove-anon-warning' ).hide();
 149+ }
140150
141151 // When the image changes, we want to reset the preview and error message.
142152 $( '#mw-wikilove-image' ).change( function() {
——@@ -220,31 +230,31 @@
221231 * Remember data the user entered if it is different from the default.
222232 */
223233 rememberInputData: function() {
224 - if( rememberData === null) {
 234+ if ( rememberData === null) {
225235 rememberData = {
226 - 'header' : '',
227 - 'title' : '',
228 - 'message': '',
229 - 'image' : '',
 236+ header : '',
 237+ title : '',
 238+ message: '',
 239+ image : ''
230240 };
231241 }
232 - if( currentTypeOrSubtype !== null ) {
233 - if( $.inArray( 'header', currentTypeOrSubtype.fields ) >= 0 &&
 242+ if ( currentTypeOrSubtype !== null ) {
 243+ if ( $.inArray( 'header', currentTypeOrSubtype.fields ) >= 0 &&
234244 ( !currentTypeOrSubtype.header || $( '#mw-wikilove-header' ).val() != currentTypeOrSubtype.header ) )
235245 {
236246 rememberData.header = $( '#mw-wikilove-header' ).val();
237247 }
238 - if( $.inArray( 'title', currentTypeOrSubtype.fields ) >= 0 &&
 248+ if ( $.inArray( 'title', currentTypeOrSubtype.fields ) >= 0 &&
239249 ( !currentTypeOrSubtype.title || $( '#mw-wikilove-title' ).val() != currentTypeOrSubtype.title ) )
240250 {
241251 rememberData.title = $( '#mw-wikilove-title' ).val();
242252 }
243 - if( $.inArray( 'message', currentTypeOrSubtype.fields ) >= 0 &&
 253+ if ( $.inArray( 'message', currentTypeOrSubtype.fields ) >= 0 &&
244254 ( !currentTypeOrSubtype.message || $( '#mw-wikilove-message' ).val() != currentTypeOrSubtype.message ) )
245255 {
246256 rememberData.message = $( '#mw-wikilove-message' ).val();
247257 }
248 - if( typeof currentTypeOrSubtype.gallery == 'undefined' && $.inArray( 'image', currentTypeOrSubtype.fields ) >= 0 &&
 258+ if ( currentTypeOrSubtype.gallery === undefined && $.inArray( 'image', currentTypeOrSubtype.fields ) >= 0 &&
249259 ( !currentTypeOrSubtype.image || $( '#mw-wikilove-image' ).val() != currentTypeOrSubtype.image ) )
250260 {
251261 rememberData.image = $( '#mw-wikilove-image' ).val();
——@@ -263,7 +273,7 @@
264274 'header' : ( $.inArray( 'header', currentTypeOrSubtype.fields ) >= 0 ? rememberData.header : '' ),
265275 'title' : ( $.inArray( 'title', currentTypeOrSubtype.fields ) >= 0 ? rememberData.title : '' ),
266276 'message': ( $.inArray( 'message', currentTypeOrSubtype.fields ) >= 0 ? rememberData.message : '' ),
267 - 'image' : ( $.inArray( 'image', currentTypeOrSubtype.fields ) >= 0 ? rememberData.image : '' ),
 277+ 'image' : ( $.inArray( 'image', currentTypeOrSubtype.fields ) >= 0 ? rememberData.image : '' )
268278 };
269279
270280 // only show the description if it exists for this type or subtype
——@@ -328,12 +338,12 @@
329339 $( '#mw-wikilove-dialog' ).find( '.mw-wikilove-error' ).remove();
330340
331341 // Check for a header if it is required
332 - if( $.inArray( 'header', currentTypeOrSubtype.fields ) >= 0 && $( '#mw-wikilove-header' ).val().length <= 0 ) {
 342+ if( $.inArray( 'header', currentTypeOrSubtype.fields ) >= 0 && $( '#mw-wikilove-header' ).val().length === 0 ) {
333343 $.wikiLove.showAddDetailsError( 'wikilove-err-header' ); return false;
334344 }
335345
336346 // Check for a title if it is required, and otherwise use the header text
337 - if( $.inArray( 'title', currentTypeOrSubtype.fields ) >= 0 && $( '#mw-wikilove-title' ).val().length <= 0 ) {
 347+ if( $.inArray( 'title', currentTypeOrSubtype.fields ) >= 0 && $( '#mw-wikilove-title' ).val().length === 0 ) {
338348 $( '#mw-wikilove-title' ).val( $( '#mw-wikilove-header' ).val() );
339349 }
340350
——@@ -347,7 +357,7 @@
348358 // Split image validation depending on whether or not it is a gallery
349359 if ( typeof currentTypeOrSubtype.gallery == 'undefined' ) { // not a gallery
350360 if ( $.inArray( 'image', currentTypeOrSubtype.fields ) >= 0 ) { // asks for an image
351 - if ( $( '#mw-wikilove-image' ).val().length <= 0 ) { // no image entered
 361+ if ( $( '#mw-wikilove-image' ).val().length === 0 ) { // no image entered
352362 // Give them the default image and continue with preview.
353363 $( '#mw-wikilove-image' ).val( options.defaultImage );
354364 $.wikiLove.submitPreview();
——@@ -388,7 +398,7 @@
389399 $.wikiLove.submitPreview();
390400 }
391401 } else { // a gallery
392 - if ( $( '#mw-wikilove-image' ).val().length <= 0 ) { // no image selected
 402+ if ( $( '#mw-wikilove-image' ).val().length === 0 ) { // no image selected
393403 // Display an error telling them to select an image.
394404 $.wikiLove.showAddDetailsError( 'wikilove-err-image' ); return false;
395405 } else { // image was selected
——@@ -441,7 +451,11 @@
442452 * Adds a "File:" prefix if there isn't already a media namespace prefix.
443453 */
444454 addFilePrefix: function( filename ) {
445 - if ( filename.indexOf( 'File:' ) !== 0 && filename.indexOf( 'Image:' ) !== 0 && filename.indexOf( wgFormattedNamespaces[6] + ':' ) !== 0 ) {
 455+ // Can't use mw.Title in 1.17
 456+ var prefix = filename.split( ':' )[0] || '',
 457+ normalized = $.trim( prefix ).toLowerCase().replace( /\s/g, '_' );
 458+ // wgNamespaceIds is missing 'file' in 1.17 on non-English wikis
 459+ if ( mw.config.get( 'wgNamespaceIds' )[normalized] !== 6 && normalized !== 'file' ) {
446460 filename = 'File:' + filename;
447461 }
448462 return filename;
——@@ -509,12 +523,12 @@
510524 $( '#mw-wikilove-dialog' ).find( '.mw-wikilove-error' ).remove();
511525
512526 // Check for a header if it is required
513 - if( $.inArray( 'header', currentTypeOrSubtype.fields ) >= 0 && $( '#mw-wikilove-header' ).val().length <= 0 ) {
 527+ if( $.inArray( 'header', currentTypeOrSubtype.fields ) >= 0 && $( '#mw-wikilove-header' ).val().length === 0 ) {
514528 $.wikiLove.showAddDetailsError( 'wikilove-err-header' ); return false;
515529 }
516530
517531 // Check for a title if it is required, and otherwise use the header text
518 - if( $.inArray( 'title', currentTypeOrSubtype.fields ) >= 0 && $( '#mw-wikilove-title' ).val().length <= 0 ) {
 532+ if( $.inArray( 'title', currentTypeOrSubtype.fields ) >= 0 && $( '#mw-wikilove-title' ).val().length === 0 ) {
519533 $( '#mw-wikilove-title' ).val( $( '#mw-wikilove-header' ).val() );
520534 }
521535
——@@ -689,13 +703,13 @@
690704 * Init function which is called upon page load. Binds the WikiLove icon to opening the dialog.
691705 */
692706 init: function() {
693 - if( typeof $.wikiLoveOptions == 'function' ) options = $.wikiLoveOptions();
694 -
 707+ options = $.wikiLoveOptions;
 708+
695709 var $wikiLoveLink = $( '#ca-wikilove' ).find( 'a' );
696710 $wikiLoveLink.unbind( 'click' );
697711 $wikiLoveLink.click( function( e ) {
 712+ e.preventDefault();
698713 $.wikiLove.openDialog();
699 - e.preventDefault();
700714 });
701715 }
702716
——@@ -782,7 +796,7 @@
783797 }
784798 }
785799 }
786 - if( gallery.length <= 0 ) {
 800+ if( gallery.length === 0 ) {
787801 $( '#mw-wikilove-gallery' ).hide();
788802 $( '#mw-wikilove-gallery-label' ).hide();
789803 }
——@@ -794,7 +808,6 @@
795809 */
796810 };
797811
798 -}());
799812
800813 $( document ).ready( $.wikiLove.init );
801814 } ) ( jQuery );

Follow-up revisions

RevisionCommit summaryAuthorDate
r91389WikiLove fixes:...krinkle23:50, 3 July 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r90880Fixed adding file prefix as mentioned in the comments of r90220janpaul12315:11, 27 June 2011
r91046adding a couple more validators to submitSend since we are doing just-in-time...kaldari02:15, 29 June 2011

Comments

#Comment by Catrope (talk | contribs)   10:37, 30 June 2011 
+		var prefix = filename.split( ':' )[0] || '',
+			normalized = $.trim( prefix ).toLowerCase().replace( /\s/g, '_' );
+		// wgNamespaceIds is missing 'file' in 1.17 on non-English wikis
+		if ( mw.config.get( 'wgNamespaceIds' )[normalized] !== 6 && normalized !== 'file' ) {

This is broken for the (unlikely, I admit) case that filename === 'File' (or 'Image' or a local alias).

OK otherwise.

#Comment by Krinkle (talk | contribs)   17:15, 30 June 2011

Yeah, checking length of the array is probably a good thing after splitting it.

There are other edgecases though, which is why mw.Title was created.

#Comment by Krinkle (talk | contribs)   23:51, 3 July 2011

Done in r91389.

Status & tagging log