r83791 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r83790‎ | r83791 | r83792 >
Date:22:49, 12 March 2011
Author:btongminh
Status:resolved (Comments)
Tags:core 
Comment:
(bug 2581, bug 6834) Added links to thumbnail in several resolutions to the file description page. The sizes are set by $wgImageLimits.
Not really happy about how it looks, perhaps only the numbers should be linked? See http://www.mediawiki.org/wiki/User:Bryan/Bug_2581 for options.
Removed message show-big-image-thumb, added show-big-image-preview, show-big-image-other, show-big-image-size.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/ImagePage.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/maintenance/language/messages.inc (modified) (history)

Diff [purge]

Index: trunk/phase3/RELEASE-NOTES
@@ -112,6 +112,8 @@
113113 * (bug 14706) Added support for the Imagick PHP extension.
114114 * (bug 18691) Added support for SVG rasterization using the Imagick PHP
115115 extension
 116+* (bug 2581, bug 6834) Added links to thumbnail in several resolutions to the
 117+ file description page. The sizes are set by $wgImageLimits.
116118
117119 === Bug fixes in 1.18 ===
118120 * (bug 23119) WikiError class and subclasses are now marked as deprecated
Index: trunk/phase3/languages/messages/MessagesEn.php
@@ -3641,7 +3641,9 @@
36423642 'file-nohires' => '<small>No higher resolution available.</small>',
36433643 'svg-long-desc' => 'SVG file, nominally $1 × $2 pixels, file size: $3',
36443644 'show-big-image' => 'Full resolution',
3645 -'show-big-image-thumb' => '<small>Size of this preview: $1 × $2 pixels</small>',
 3645+'show-big-image-preview' => '<small> Size of this preview: $1.</small>',
 3646+'show-big-image-other' => '<small>Other resolutions: $1.</small>',
 3647+'show-big-image-size' => '$1 × $2 pixels',
36463648 'file-info-gif-looped' => 'looped',
36473649 'file-info-gif-frames' => '$1 {{PLURAL:$1|frame|frames}}',
36483650 'file-info-png-looped' => 'looped',
Index: trunk/phase3/includes/ImagePage.php
@@ -360,16 +360,25 @@
361361 # because of rounding.
362362 }
363363 $msgbig = wfMsgHtml( 'show-big-image' );
364 - $msgsmall = wfMsgExt( 'show-big-image-thumb', 'parseinline',
365 - $wgLang->formatNum( $width ),
366 - $wgLang->formatNum( $height )
367 - );
 364+ $otherSizes = array();
 365+ foreach ( $wgImageLimits as $size ) {
 366+ if ( $size[0] < $width_orig && $size[1] < $height_orig &&
 367+ $size[0] != $width && $size[1] != $height ) {
 368+ $otherSizes[] = $this->makeSizeLink( $params, $size[0], $size[1] );
 369+ }
 370+ }
 371+ $msgsmall = wfMessage( 'show-big-image-preview' )->
 372+ rawParams( $this->makeSizeLink( $params, $width, $height ) )->
 373+ parse() . ' ' .
 374+ wfMessage( 'show-big-image-other' )->
 375+ rawParams( $wgLang->pipeList( $otherSizes ) )->parse();
368376 } else {
369377 # Image is small enough to show full size on image page
370378 $msgsmall = wfMsgExt( 'file-nohires', array( 'parseinline' ) );
371379 }
372380
373381 $params['width'] = $width;
 382+ $params['height'] = $height;
374383 $thumbnail = $this->displayImg->transform( $params );
375384
376385 $showLink = true;
@@ -515,6 +524,30 @@
516525 }
517526 }
518527 }
 528+
 529+ /**
 530+ * Creates an thumbnail of specified size and returns an HTML link to it
 531+ * @param array $params Scaler parameters
 532+ * @param int $width
 533+ * @param int $height
 534+ */
 535+ private function makeSizeLink( $params, $width, $height ) {
 536+ global $wgLang;
 537+
 538+ $params['width'] = $width;
 539+ $params['height'] = $height;
 540+ $thumbnail = $this->displayImg->transform( $params );
 541+ if ( $thumbnail && !$thumbnail->isError() ) {
 542+ return Html::rawElement( 'a', array(
 543+ 'href' => $thumbnail->getUrl(),
 544+ 'class' => 'mw-thumbnail-link'
 545+ ), wfMessage( 'show-big-image-size' )->numParams(
 546+ $thumbnail->getWidth(), $thumbnail->getHeight()
 547+ )->parse() );
 548+ } else {
 549+ return '';
 550+ }
 551+ }
519552
520553 /**
521554 * Show a notice that the file is from a shared repository
Index: trunk/phase3/maintenance/language/messages.inc
@@ -2589,7 +2589,9 @@
25902590 'file-nohires',
25912591 'svg-long-desc',
25922592 'show-big-image',
2593 - 'show-big-image-thumb',
 2593+ 'show-big-image-preview',
 2594+ 'show-big-image-other',
 2595+ 'show-big-image-size',
25942596 'file-info-gif-looped',
25952597 'file-info-gif-frames',
25962598 'file-info-png-looped',

Follow-up revisions

RevisionCommit summaryAuthorDate
r84049Follow-up r83791: Remove uselss leading spaceraymond19:55, 15 March 2011
r110179Follow-up r83791: do not show size links for wikis which do not use a 404 tra...btongminh22:44, 27 January 2012

Comments

#Comment by MZMcBride (talk | contribs)   23:46, 12 March 2011
+'show-big-image-preview' => '<small> Size of this preview: $1.</small>',

I'm not sure if the space following the <small> tag was intentional, but it looks a little odd.

Using <small> tags rather than CSS here is a bit odd, but it seems like you're not the first person to do this in a MediaWiki message. I suppose it's just as easy to customize the MediaWiki message as it is to customize the site CSS. That said, it's mixing localization/text with design/code, which isn't really ideal. It might make sense to switch to unconditional <span>s within the code.

Regarding [[User:Bryan/Bug 2581]], I like just the sizes being linked. Linking the word "pixels" (especially several times) looks weird. I'm not even sure you need to specify pixels in subsequent uses (e.g., "Size of this preview: 800 × 600 pixels. Other resolutions: 320 × 240; 640 × 480; 1,024 × 768."). I think that's pretty clear.

The separators for other sizes all have their own merit. I think the separator should be customizable with a MediaWiki message. I'm not sure if it is with pipeList in this code. Maybe a separate "separator" message would be a good idea? I don't know.

#Comment by Bawolff (talk | contribs)   00:14, 13 March 2011

Small is defined (in html5) as "The small element represents side comments such as small print.". I think this fits that description, but adding a class to the small tag wouldn't hurt so people can customize and what not.

#Comment by Bryan (talk | contribs)   11:45, 13 March 2011

The small tags were copied from the old show-big-image-thumb message. Perhaps CSS is a better way though.

Having a separator-message specifically for this purpose sounds a bit like customization-cruft to me, but on the other hand I guess hardcoding the separator is not very good either.

#Comment by MZMcBride (talk | contribs)   22:19, 13 March 2011

You could reuse the category separator message, but I'm not sure if that's more or less evil.

#Comment by Bryan (talk | contribs)   22:20, 13 March 2011

More.

#Comment by Raymond (talk | contribs)   20:06, 15 March 2011

pipeList() uses MediaWiki:Pipe-separator. This is used for all such usages.

#Comment by Tim Starling (talk | contribs)   01:48, 6 December 2011

You said on bug 2581 that doing this for wikis without a 404 handler was an unacceptable performance burden and that you weren't going to enable this feature on such wikis. But the relevant code seems to be missing, and small default installations do indeed have a large performance burden from this feature.

#Comment by Bryan (talk | contribs)   22:45, 27 January 2012

Status & tagging log