r98839 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r98838‎ | r98839 | r98840 >
Date:00:12, 4 October 2011
Author:amire80
Status:resolved (Comments)
Tags:
Comment:
The general fix is setting the directionality of added or removed bytes in recent changes. It is always supposed to be LTR.

Details:
* Spelling of variable names and comments: formated -> formatted
* Replaced "<$tag" with Xml::element
* Added a dirmark before the username (otherwise the added or removed appear on the other side)
Modified paths:
  • /trunk/phase3/includes/ChangesList.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/ChangesList.php
——@@ -193,10 +193,10 @@
194194 $fastCharDiff[$code] = $wgMiserMode || wfMsgNoTrans( 'rc-change-size' ) === '$1';
195195 }
196196
197 - $formatedSize = $wgLang->formatNum($szdiff);
 197+ $formattedSize = $wgLang->formatNum($szdiff);
198198
199199 if ( !$fastCharDiff[$code] ) {
200 - $formatedSize = wfMsgExt( 'rc-change-size', array( 'parsemag', 'escape' ), $formatedSize );
 200+ $formattedSize = wfMsgExt( 'rc-change-size', array( 'parsemag', 'escape' ), $formattedSize );
201201 }
202202
203203 if( abs( $szdiff ) > abs( $wgRCChangedSizeThreshold ) ) {
——@@ -204,13 +204,20 @@
205205 } else {
206206 $tag = 'span';
207207 }
208 - if( $szdiff === 0 ) {
209 - return "<$tag class='mw-plusminus-null'>($formatedSize)</$tag>";
210 - } elseif( $szdiff > 0 ) {
211 - return "<$tag class='mw-plusminus-pos'>(+$formatedSize)</$tag>";
212 - } else {
213 - return "<$tag class='mw-plusminus-neg'>($formatedSize)</$tag>";
 208+ $formattedSizeClass = 'mw-plusminus-';
 209+ if ( $szdiff === 0 ) {
 210+ $formattedSizeClass .= 'null';
214211 }
 212+ if ( $szdiff > 0 ) {
 213+ $formattedSize = '+' . $formattedSize;
 214+ $formattedSizeClass .= 'pos';
 215+ }
 216+ if ( $szdiff < 0 ) {
 217+ $formattedSizeClass .= 'neg';
 218+ }
 219+ return Xml::element($tag,
 220+ array('dir' => 'ltr', 'class' => $formattedSizeClass),
 221+ "($formattedSize)");
215222 }
216223
217224 /**
——@@ -344,7 +351,7 @@
345352 if( $this->isDeleted( $rc, Revision::DELETED_USER ) ) {
346353 $s .= ' <span class="history-deleted">' . wfMsgHtml( 'rev-deleted-user' ) . '</span>';
347354 } else {
348 - $s .= Linker::userLink( $rc->mAttribs['rc_user'], $rc->mAttribs['rc_user_text'] );
 355+ $s .= $this->getLang()->getDirMark() . Linker::userLink( $rc->mAttribs['rc_user'], $rc->mAttribs['rc_user_text'] );
349356 $s .= Linker::userToolLinks( $rc->mAttribs['rc_user'], $rc->mAttribs['rc_user_text'] );
350357 }
351358 }
——@@ -1068,7 +1075,7 @@
10691076 * Enhanced RC ungrouped line.
10701077 *
10711078 * @param $rcObj RecentChange
1072 - * @return String: a HTML formated line (generated using $r)
 1079+ * @return String: a HTML formatted line (generated using $r)
10731080 */
10741081 protected function recentChangesBlockLine( $rcObj ) {
10751082 global $wgRCShowChangedSize;

Follow-up revisions

RevisionCommit summaryAuthorDate
r98847Followup to r98839. Removed 'escape' from wfMsgExt (suggested by Nikerabbit);...amire8003:08, 4 October 2011
r98984Followup to r98839 and r98851 according to Nikerabbit's suggestions: Xml -> H...amire8006:45, 5 October 2011

Comments

#Comment by Amire80 (talk | contribs)   00:14, 4 October 2011

This is my first functional PHP commit. Please be gentle :)

#Comment by Siebrand (talk | contribs)   02:03, 4 October 2011

Congratulations. Your first FIXME is here :). (see Krinkle's comments.)

#Comment by Nikerabbit (talk | contribs)   19:22, 4 October 2011

Second actually :)

#Comment by Sumanah (talk | contribs)   00:18, 4 October 2011

If nothing else, I'm happy you changed "formated" to "formatted".  :-)

#Comment by Krinkle (talk | contribs)   00:24, 4 October 2011 
-		if( $szdiff === 0 ) {
-			return "<$tag class='mw-plusminus-null'>($formatedSize)</$tag>";
-		} elseif( $szdiff > 0 ) {
-			return "<$tag class='mw-plusminus-pos'>(+$formatedSize)</$tag>";
-		} else {
-			return "<$tag class='mw-plusminus-neg'>($formatedSize)</$tag>";
+		$formattedSizeClass = 'mw-plusminus-';
+		if ( $szdiff === 0 ) {
+			$formattedSizeClass .= 'null';
 		}
+		if ( $szdiff > 0 ) {
+			$formattedSize = '+' . $formattedSize;
+			$formattedSizeClass .= 'pos';
+		}
+		if ( $szdiff < 0 ) {
+			$formattedSizeClass .= 'neg';
+		}

A small tip. In order to be able to easily search where message and classnames are used, it is recommended not not split up class names, or atleast mention the full name somewhere in a comment, like 

		// mw-plusminus-null, mw-plusminus-pos, mw-plusminus-neg
		$formattedSizeClass = 'mw-plusminus-';
		if ( $szdiff === 0 ) {
			$formattedSizeClass .= 'null';
		}
		if ( $szdiff > 0 ) {
			$formattedSize = '+' . $formattedSize;
			$formattedSizeClass .= 'pos';
		}
		if ( $szdiff < 0 ) {
			$formattedSizeClass .= 'neg';
		}

or simply: 

		if ( $szdiff === 0 ) {
			$formattedSizeClass = 'mw-plusminus-null';
		}
		if ( $szdiff > 0 ) {
			$formattedSize = '+' . $formattedSize;
			$formattedSizeClass = 'mw-plusminus-pos';
		}
		if ( $szdiff < 0 ) {
			$formattedSizeClass = 'mw-plusminus-neg';
		}
#Comment by Nikerabbit (talk | contribs)   19:24, 4 October 2011

Only small issues left: Xml -> Html and some whitespace around ().

Status & tagging log