r68325 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r68324‎ | r68325 | r68326 >
Date:16:13, 20 June 2010
Author:hartman
Status:ok (Comments)
Tags:
Comment:
Don't override the default Bitmap.php information in the longDescription() for GIF images, simply append.
Modified paths:
  • /trunk/phase3/includes/media/GIF.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/media/GIF.php
——@@ -63,16 +63,17 @@
6464 function getLongDesc( $image ) {
6565 global $wgUser, $wgLang;
6666 $sk = $wgUser->getSkin();
 67+ $original = parent::getLongDesc( $image );
6768
6869 wfSuppressWarnings();
6970 $metadata = unserialize($image->getMetadata());
7071 wfRestoreWarnings();
7172
72 - if (!$metadata) return parent::getLongDesc( $image );
 73+ if (!$metadata || $metadata['frameCount'] <= 1)
 74+ return $original;
7375
7476 $info = array();
75 - $info[] = $image->getMimeType();
76 - $info[] = $sk->formatSize( $image->getSize() );
 77+ $info[] = substr( $original, 1, strlen( $original )-2 );
7778
7879 if ($metadata['looped'])
7980 $info[] = wfMsgExt( 'file-info-gif-looped', 'parseinline' );

Follow-up revisions

RevisionCommit summaryAuthorDate
r79920Followup r68325, with comment describing intent of code.hartman22:54, 9 January 2011
r79964Move the () surrounding description strings of files, out of the description ...hartman22:18, 10 January 2011

Comments

#Comment by Bryan (talk | contribs)   20:05, 9 January 2011 
$info[] = substr( $original, 1, strlen( $original )-2 );

This is magic that needs a comment on what it is supposed to do.

#Comment by TheDJ (talk | contribs)   22:55, 9 January 2011

Done in r79920

#Comment by Bryan (talk | contribs)   07:47, 10 January 2011

Thanks.

You are making an assumption of the return value of parent::getLongDesc, which is based on the file-info message. As this can be localized, assuming that the last char is ) is fragile and unreliable. I think it would be better to introduce a new message. Also, I don't like the hard coding of () at the end of the function.

#Comment by TheDJ (talk | contribs)   08:42, 10 January 2011

Preferably, we should just remove the ( ) entirely from these descriptionstrings, and only add them in the HTML. But that is quite a lot of work since extensions make the same assumptions.

#Comment by Bryan (talk | contribs)   09:56, 10 January 2011

The parentheses could be in their own message, and then we could do something like wfMsg( 'file-info-wrapper', commaList( $info ) ) with file-info-wrapper ($1)

#Comment by TheDJ (talk | contribs)   21:50, 10 January 2011

I prefer to just do this: http://pastebin.com/ivKk6PZh

#Comment by Bryan (talk | contribs)   22:06, 10 January 2011

Good work, should be committed.

Status & tagging log