r108203 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r108202‎ \| r108203 \| r108204 >
Date:	09:14, 6 January 2012
Author:	santhosh
Status:	resolved (Comments)
Tags:	i18nreview
Comment:	Use mw.jqueryMsg parser for message parsing to support PLURAL and GENDER Follow up r107556 and based on the discussions on wikitech-l about this. mediawiki.jqueryMsg is now loaded always. mw.msg uses the parser if required. Add qunit test cases.
Modified paths:	/trunk/phase3/RELEASE-NOTES-1.19 (modified) (history) /trunk/phase3/includes/OutputPage.php (modified) (history) /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history) /trunk/phase3/tests/qunit/suites/resources/mediawiki/mediawiki.test.js (modified) (history)

Diff [purge]

Index: trunk/phase3/RELEASE-NOTES-1.19
—	—	@@ -118,6 +118,7 @@
119	119	loaded in some contexts.
120	120	* (bug 33456) Show $wgQueryCacheLimit on cached query pages.
121	121	* (bug 10574) Add an option to allow all pages to be exported by Special:Export.
	122	+* Use mw.jqueryMsg parser for message parsing to support PLURAL and GENDER
122	123
123	124	=== Bug fixes in 1.19 ===
124	125	* $wgUploadNavigationUrl should be used for file redlinks if.
Index: trunk/phase3/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
—	—	@@ -80,7 +80,7 @@
81	81	});
82	82
83	83	test( 'mw.message & mw.messages', function() {
84		~~- expect(17);~~
	84	+ expect(20);
85	85
86	86	ok( mw.messages, 'messages defined' );
87	87	ok( mw.messages instanceof mw.Map, 'mw.messages instance of mw.Map' );
—	—	@@ -116,15 +116,31 @@
117	117	equal( goodbye.plain(), '<goodbye>', 'Message.toString returns plain <key> if format is "plain" and key does not exist' );
118	118	// bug 30684
119	119	equal( goodbye.escaped(), '<goodbye>', 'Message.toString returns properly escaped <key> if format is "escaped" and key does not exist' );
	120	+
	121	+ ok( mw.messages.set( 'pluraltestmsg', 'There {{PLURAL:$1\|is\|are}} $1 {{PLURAL:$1\|result\|results}}' ), 'mw.messages.set: Register' );
	122	+ var pluralMessage = mw.message( 'pluraltestmsg' , 6 );
	123	+ equal( pluralMessage.plain(), 'There are 6 results', 'plural get resolved when format is plain' );
	124	+ equal( pluralMessage.parse(), 'There are 6 results', 'plural get resolved when format is parse' );
	125	+
120	126	});
121	127
122	128	test( 'mw.msg', function() {
123		~~- expect(3);~~
	129	+ expect(11);
124	130
125	131	ok( mw.messages.set( 'hello', 'Hello <b>awesome</b> world' ), 'mw.messages.set: Register' );
126		-
127	132	equal( mw.msg( 'hello' ), 'Hello <b>awesome</b> world', 'Gets message with default options (existing message)' );
128	133	equal( mw.msg( 'goodbye' ), '<goodbye>', 'Gets message with default options (nonexistent message)' );
	134	+
	135	+ ok( mw.messages.set( 'plural-item' , 'Found $1 {{PLURAL:$1\|item\|items}}' ) );
	136	+ equal( mw.msg( 'plural-item', 5 ), 'Found 5 items', 'Apply plural for count 5' );
	137	+ equal( mw.msg( 'plural-item', 0 ), 'Found 0 items', 'Apply plural for count 0' );
	138	+ equal( mw.msg( 'plural-item', 1 ), 'Found 1 item', 'Apply plural for count 1' );
	139	+
	140	+ ok( mw.messages.set('gender-plural-msg' , '{{GENDER:$1\|he\|she\|they}} {{PLURAL:$2\|is\|are}} awesome' ) );
	141	+ equal( mw.msg( 'gender-plural-msg', 'male', 1 ), 'he is awesome', 'Gender test for male, plural count 1' );
	142	+ equal( mw.msg( 'gender-plural-msg', 'female', '1' ), 'she is awesome', 'Gender test for female, plural count 1' );
	143	+ equal( mw.msg( 'gender-plural-msg', 'unknown', 10 ), 'they are awesome', 'Gender test for neutral, plural count 10' );
	144	+
129	145	});
130	146
131	147	test( 'mw.loader', function() {
Index: trunk/phase3/includes/OutputPage.php
—	—	@@ -2446,6 +2446,7 @@
2447	2447	'mediawiki.util',
2448	2448	'mediawiki.page.startup',
2449	2449	'mediawiki.page.ready',
	2450	+ 'mediawiki.jqueryMsg',
2450	2451	) );
2451	2452	if ( $wgIncludeLegacyJavaScript ){
2452	2453	$this->addModules( 'mediawiki.legacy.wikibits' );
Index: trunk/phase3/resources/mediawiki/mediawiki.js
—	—	@@ -8,7 +8,7 @@
9	9	/* Private Members */
10	10
11	11	var hasOwn = Object.prototype.hasOwnProperty;
12		-
	12	+ var parser;
13	13	/* Object constructors */
14	14
15	15	/**
—	—	@@ -125,6 +125,7 @@
126	126	this.format = 'plain';
127	127	this.map = map;
128	128	this.key = key;
	129	+ parser = parser \|\| mw.jqueryMsg.getMessageFunction( );
129	130	this.parameters = parameters === undefined ? [] : $.makeArray( parameters );
130	131	return this;
131	132	}
—	—	@@ -150,7 +151,7 @@
151	152	* @return string Message as a string in the current form or <key> if key does not exist.
152	153	*/
153	154	toString: function() {
154		~~- if ( !this.map.exists( this.key ) ) {~~
	155	+ if ( !this.exists( ) ) {
155	156	// Use <key> as text if key does not exist
156	157	if ( this.format !== 'plain' ) {
157	158	// format 'escape' and 'parse' need to have the brackets and key html escaped
—	—	@@ -161,25 +162,28 @@
162	163	var text = this.map.get( this.key ),
163	164	parameters = this.parameters;
164	165
165		~~- text = text.replace( /\$(\d+)/g, function ( str, match ) {~~
166		~~- var index = parseInt( match, 10 ) - 1;~~
167		~~- return parameters[index] !== undefined ? parameters[index] : '$' + match;~~
168		~~- } );~~
169		-
170	166	if ( this.format === 'plain' ) {
171		~~- return text;~~
	167	+ // Do not use parser unless required.
	168	+ if ( text.indexOf( '{{' ) < 0 ) {
	169	+ text = text.replace( /\$(\d+)/g, function ( str, match ) {
	170	+ var index = parseInt( match, 10 ) - 1;
	171	+ return parameters[index] !== undefined ? parameters[index] : '$' + match;
	172	+ } );
	173	+ }
	174	+ else{
	175	+ text = parser( this.key, this.parameters );
	176	+ }
172	177	}
	178	+
173	179	if ( this.format === 'escaped' ) {
174		~~- // According to Message.php this needs {{-transformation, which is~~
175		~~- // still todo~~
176		~~- return mw.html.escape( text );~~
	180	+ text = parser( this.key, this.parameters );
	181	+ text = mw.html.escape( text );
177	182	}
	183	+
	184	+ if ( this.format === 'parse' ) {
	185	+ text = parser( this.key, this.parameters );
	186	+ }
178	187
179		~~- /* This should be fixed up when we have a parser~~
180		~~- if ( this.format === 'parse' && 'language' in mw ) {~~
181		~~- text = mw.language.parse( text );~~
182		~~- }~~
183		~~- */~~
184	188	return text;
185	189	},
186	190

Follow-up revisions

Revision	Commit summary	Author	Date
r108230	Fix up r108203: just loading mw.jqueryMsg in the bottom queue, then assuming ...	catrope	14:11, 6 January 2012
r108231	Fix broken oldParser call in r108230	catrope	14:17, 6 January 2012
r108259	release-notes: remove notes for r108203; adding new ones for r108230	krinkle	17:35, 6 January 2012

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r107556	Use jqueryMsg wikitext parser to parse interface messages at client side. Sup...	santhosh	09:29, 29 December 2011

Comments

#Comment by Catrope (talk | contribs) 12:09, 6 January 2012

This is broken, causing mw.loader::execute> Exception thrown by ext.vector.simpleSearch: mw.jqueryMsg is undefined on TWN. Will look into fixing it.

#Comment by Krinkle (talk | contribs) 12:31, 6 January 2012

I don't see how this is any different than r107556, please revert. The mediawiki module can't have dependencies, and adding it to another random module isn't helping the obvious race condition.

And branching Message into a separate module is not a solution in the short term because of backwards compatibility.

As I've asked before, what's wrong with modules needing the parser simply adding "mediawiki.jqueryMsg" as a dependency and using it ? Or if you want it to be available by default, create a "mediawiki.jqueryMsg.init" module that will extend/overwrite the message prototype and use the parser by default (so that you don;t have to do mw.jqueryMsg.getMessageFunction( )). Either way, this isn't working.

#Comment by Catrope (talk | contribs) 12:41, 6 January 2012

Hrmph, the Message prototype is private, so jQueryMsg can't extend it. Maybe I should make it public?

Since you're commenting here and responding quite quickly, any chance you could get on IRC?

#Comment by Catrope (talk | contribs) 12:32, 6 January 2012

No need for hasty reactions, I'm already fixing this.

#Comment by Krinkle (talk | contribs) 12:33, 6 January 2012

Alrighty, red blur on the swarm freaked me out..

#Comment by Krinkle (talk | contribs) 12:33, 6 January 2012

Also, it appears this revision is introducing a regression in the unit tests.

6. mediawiki: mw.message & mw.messages
----
Message.escaped returns the escaped message
Expected:	
"Hello <b>awesome</b> world"
Result:	
"Hello <B>awesome</B> world"
Diff:	
 "Hello <b>awesome</b> <B>awesome</B>  world"

It looks like the message is parsed as HTML before it is escaped, this causes tagnames to become uppercase. Although this is not a big problem, there should be no need to parse it before escaping.

#Comment by Catrope (talk | contribs) 14:36, 6 January 2012

I don't know where this came from, but it's gone now. Perhaps that's because jqueryMsg notices the messages doesn't contain {{ and hands it off to the normal parser in that case (which it probably shouldn't do for HTML output? This conflation of HTML and non-HTML is a mess, as I remarked earlier).

Marking resolved, tests pass as of r108231.

#Comment by Siebrand (talk | contribs) 14:08, 6 January 2012

Lame comment to watch rev: yay!

Status & tagging log

18:40, 14 March 2012 Catrope (talk | contribs) changed the tags for r108203 [removed: roan]
00:51, 18 January 2012 Siebrand (talk | contribs) changed the tags for r108203 [removed: core]
14:36, 6 January 2012 Catrope (talk | contribs) changed the status of r108203 [removed: fixme added: resolved]
12:09, 6 January 2012 Catrope (talk | contribs) changed the status of r108203 [removed: new added: fixme]
09:15, 6 January 2012 Santhosh.thottingal (talk | contribs) changed the tags for r108203 [added: i18nreview,core,roan]