r55302 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r55301‎ | r55302 | r55303 >
Date:02:07, 19 August 2009
Author:brion
Status:ok
Tags:
Comment:
Revert image crop for now:
r54746 "Update formatting for r54745"
r54745 "Added crop support for inline images."
r54748 "Making demon happy (adding public/protected to function definitions) and add some comments along the way."

Several issues:
* Implementation is ImageMagick-specific, no support for GD backend
* Specification syntax is pretty ugly and non-obvious imo... [[File:foo.jpg|<width>px|<left>x<top>x<width>x<height]]
* Crop syntax seems to be tied to pixels... I _presume_ source pixels? This would break if a file is re-uploaded with a new size.
* In general I'm very leery of tacking on more infinite-options-in-infinte-combinations for image rendering; our treatment of resizing already has implications for CPU and disk usage and this just adds another level of arbitrary-rendered horror. ;)

I'd much rather we move towards limiting the number of rendered variants we have, not increase them... IMO cropping would be better served with an interface allowing for explicit, visible cropping which creates either a new saved version, or a 'cloned' virtual file which can be referred to by name... and more importantly, uses of it would be trackable so that if crops needs to be updated they can be cleanly.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/media/Bitmap.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/media/Bitmap.php
——@@ -8,75 +8,8 @@
99 * @ingroup Media
1010 */
1111 class BitmapHandler extends ImageHandler {
12 - /**
13 - * Override parent method to add crop to param map.
14 - */
15 - public function getParamMap() {
16 - return array(
17 - 'img_width' => 'width',
18 - 'img_crop' => 'crop',
19 - );
20 - }
21 -
22 - /**
23 - * Override param map to validate the crop param.
24 - */
25 - public function validateParam( $name, $value ) {
26 - if ( $name == 'crop' ) {
27 - return $this->splitCropParam( $value ) !== false;
28 - } else {
29 - return parent::validateParam( $name, $value );
30 - }
31 - }
32 - /**
33 - * Split the crop param into up to 4 parts and convert them to integers.
34 - * Returns false in case of malformed param.
35 - */
36 - protected function splitCropParam( $value ) {
37 - $parts = explode( 'x', $value );
38 - if ( count( $parts ) > 4 )
39 - return false;
40 - foreach ( $parts as &$part ) {
41 - $intVal = intval( $part );
42 - if ( $intVal === 0 && !( $part === '0' || $part === '' ) )
43 - return false;
44 - if ( $intVal < 0 )
45 - return false;
46 -
47 - $part = $intVal;
48 - }
49 -
50 - return $parts;
51 - }
52 -
53 - /**
54 - * Override parent method to check for optional crop parameter in param
55 - * string.
56 - */
57 - public function parseParamString( $str ) {
58 - $res = parent::parseParamString( $str );
59 - if ( $res === false ) {
60 - $m = false;
61 - if ( preg_match( '/^(\d+)px-([x0-9])crop$/', $str, $m ) ) {
62 - return array( 'width' => $m[1], 'crop' => $m[2] );
63 - } else {
64 - return false;
65 - }
66 - }
67 - }
68 - /**
69 - * Add the crop parameter the string generated by the parent.
70 - */
71 - public function makeParamString( $params ) {
72 - $res = parent::makeParamString( $params );
73 - if ( !empty( $params['crop'] ) )
74 - $res .= '-'.implode( 'x', $params['crop'] ).'crop';
75 - return $res;
76 - }
77 -
78 - public function normaliseParams( $image, &$params ) {
 12+ function normaliseParams( $image, &$params ) {
7913 global $wgMaxImageArea;
80 - # Parent fills in width, height and page and normalizes them.
8114 if ( !parent::normaliseParams( $image, $params ) ) {
8215 return false;
8316 }
——@@ -103,58 +36,18 @@
10437 $params['physicalHeight'] = $srcHeight;
10538 return true;
10639 }
107 -
108 - # Validate crop params
109 - if ( isset( $params['crop'] ) ) {
110 - # $cropParams = array( x, y, width, height );
111 - $cropParams = $this->splitCropParam( $params['crop'] );
112 - # Fill up params
113 - switch ( count( $cropParams ) ) {
114 - # All fall through
115 - case 1:
116 - $cropParams[1] = 0;
117 - case 2:
118 - $cropParams[2] = $srcWidth - $cropParams[0];
119 - case 3:
120 - $cropParams[3] = $srcHeight - $cropParams[1];
121 - }
122 - $cx = $cropParams[0] + $cropParams[2];
123 - $cy = $cropParams[1] + $cropParams[3];
124 - $targetWidth = $cropParams[2];
125 - $targetHeight = $cropParams[3];
126 - # Can't go outside image
127 - if ( $cx > $srcWidth || $cy > $srcHeight ) {
128 - # TODO: Maybe should fail gracefully?
129 - return false;
130 - }
131 - if ( $targetWidth == $srcWidth && $targetHeight == $srcHeight )
132 - {
133 - # No need to crop
134 - $params['crop'] = false;
135 - }
136 - else
137 - {
138 - $params['crop'] = $cropParams;
139 - $params['height'] = $params['physicalHeight'] = File::scaleHeight(
140 - $targetWidth, $targetHeight, $params['width'] );
141 - }
142 - } else {
143 - $params['crop'] = false;
144 - }
14540
14641 return true;
14742 }
14843
14944
150 - /**
151 - * Function that returns the number of pixels to be thumbnailed.
152 - * Intended for animated GIFs to multiply by the number of frames.
153 - */
154 - protected function getImageArea( $image, $width, $height ) {
 45+ // Function that returns the number of pixels to be thumbnailed.
 46+ // Intended for animated GIFs to multiply by the number of frames.
 47+ function getImageArea( $image, $width, $height ) {
15548 return $width * $height;
15649 }
15750
158 - public function doTransform( $image, $dstPath, $dstUrl, $params, $flags = 0 ) {
 51+ function doTransform( $image, $dstPath, $dstUrl, $params, $flags = 0 ) {
15952 global $wgUseImageMagick, $wgImageMagickConvertCommand, $wgImageMagickTempDir;
16053 global $wgCustomConvertCommand, $wgUseImageResize;
16154 global $wgSharpenParameter, $wgSharpenReductionThreshold;
——@@ -243,13 +136,6 @@
244137 } else {
245138 $tempEnv = '';
246139 }
247 -
248 - if ( $params['crop'] ) {
249 - $crop = $params['crop'];
250 - $cropCmd = "-crop {$crop[2]}x{$crop[3]}+{$crop[0]}+{$crop[1]}";
251 - }
252 - else
253 - $cropCmd = '';
254140
255141 # Specify white background color, will be used for transparent images
256142 # in Internet Explorer/Windows instead of default black.
——@@ -261,7 +147,7 @@
262148 $cmd =
263149 $tempEnv .
264150 wfEscapeShellArg($wgImageMagickConvertCommand) .
265 - " {$cropCmd} {$quality} -background white -size {$physicalWidth} ".
 151+ " {$quality} -background white -size {$physicalWidth} ".
266152 wfEscapeShellArg($srcPath . $frame) .
267153 $animation .
268154 // For the -resize option a "!" is needed to force exact size,
——@@ -361,13 +247,13 @@
362248 }
363249 }
364250
365 - public static function imageJpegWrapper( $dst_image, $thumbPath ) {
 251+ static function imageJpegWrapper( $dst_image, $thumbPath ) {
366252 imageinterlace( $dst_image );
367253 imagejpeg( $dst_image, $thumbPath, 95 );
368254 }
369255
370256
371 - public function getMetadata( $image, $filename ) {
 257+ function getMetadata( $image, $filename ) {
372258 global $wgShowEXIF;
373259 if( $wgShowEXIF && file_exists( $filename ) ) {
374260 $exif = new Exif( $filename );
——@@ -383,11 +269,11 @@
384270 }
385271 }
386272
387 - public function getMetadataType( $image ) {
 273+ function getMetadataType( $image ) {
388274 return 'exif';
389275 }
390276
391 - public function isMetadataValid( $image, $metadata ) {
 277+ function isMetadataValid( $image, $metadata ) {
392278 global $wgShowEXIF;
393279 if ( !$wgShowEXIF ) {
394280 # Metadata disabled and so an empty field is expected
——@@ -415,7 +301,7 @@
416302 * @return array of strings
417303 * @access private
418304 */
419 - public function visibleMetadataFields() {
 305+ function visibleMetadataFields() {
420306 $fields = array();
421307 $lines = explode( "\n", wfMsgForContent( 'metadata-fields' ) );
422308 foreach( $lines as $line ) {
——@@ -428,7 +314,7 @@
429315 return $fields;
430316 }
431317
432 - public function formatMetadata( $image ) {
 318+ function formatMetadata( $image ) {
433319 $result = array(
434320 'visible' => array(),
435321 'collapsed' => array()
Index: trunk/phase3/languages/messages/MessagesEn.php
——@@ -288,7 +288,6 @@
289289 'img_text_bottom' => array( 1, 'text-bottom' ),
290290 'img_link' => array( 1, 'link=$1' ),
291291 'img_alt' => array( 1, 'alt=$1' ),
292 - 'img_crop' => array( 1, 'crop=$1' ),
293292 'int' => array( 0, 'INT:' ),
294293 'sitename' => array( 1, 'SITENAME' ),
295294 'ns' => array( 0, 'NS:' ),
Index: trunk/phase3/RELEASE-NOTES
——@@ -195,7 +195,6 @@
196196 ** Unnecessary type="" attribute removed for CSS and JS.
197197 ** If $wgWellFormedXml is set to false, some bytes will be shaved off of HTML
198198 output by omitting some things like quotation marks where HTML 5 allows.
199 -* Added crop for inline images.
200199 * The description message in $wgExtensionCredits can be an array with parameters
201200 * New hook SpecialRandomGetRandomTitle allows extensions to modify the selection
202201 criteria used by Special:Random and subclasses, or substitute a custom result,

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r54745Added crop support for inline images....btongminh21:02, 10 August 2009
r54746Update formatting for r54745siebrand21:14, 10 August 2009
r54748Making demon happy (adding public/protected to function definitions) and add ...btongminh21:16, 10 August 2009

Status & tagging log