r72278 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r72277‎ | r72278 | r72279 >
Date:11:59, 3 September 2010
Author:daniel
Status:resolved (Comments)
Tags:
Comment:
fixed bugs in imageinfo parser, especially effecting images generated by photoshop. should fix some (most?) problems reported in bug 24984
Modified paths:
  • /trunk/extensions/PagedTiffHandler/PagedTiffHandler.image.php (modified) (history)
  • /trunk/extensions/PagedTiffHandler/PagedTiffHandler.php (modified) (history)

Diff [purge]

Index: trunk/extensions/PagedTiffHandler/PagedTiffHandler.php
——@@ -123,7 +123,9 @@
124124 $wgMediaHandlers['image/tiff'] = 'PagedTiffHandler';
125125 $wgHooks['LanguageGetMagic'][] = 'PagedTiffHandler::addTiffLossyMagicWordLang';
126126
127 -define('TIFF_METADATA_VERSION', '1.0');
 127+define('TIFF_METADATA_VERSION', '1.1');
 128+# 1.0: initial
 129+# 1.1: fixed bugs in imageinfo parser
128130
129131 //$wgHooks['PagedTiffHandlerRenderCommand'][] = 'PagedTiffHandler::renderCommand';
130132 //$wgHooks['PagedTiffHandlerTiffData'][] = 'PagedTiffImage::tiffData';
Index: trunk/extensions/PagedTiffHandler/PagedTiffHandler.image.php
——@@ -186,7 +186,7 @@
187187 return $this->_meta;
188188 }
189189
190 - private function addPageEntry( $entry, &$metadata, &$prevPage ) {
 190+ private function addPageEntry( &$entry, &$metadata, &$prevPage ) {
191191 if ( !isset( $entry['page'] ) ) {
192192 $entry['page'] = $prevPage +1;
193193 } else {
——@@ -266,7 +266,6 @@
267267
268268 if ( !$bypass ) {
269269 $data['warnings'][] = $msg;
270 - break;
271270 }
272271 } else if ( preg_match('/^\s*(.*?)\s*:\s*(.*?)\s*$/', $row, $m) ) {
273272 $key = $m[1];

Comments

#Comment by Duesentrieb (talk | contribs)   12:07, 3 September 2010

please merge to deployment, this should fix most tiffs that are currently not rendered. Consider including r72279 as well.

#Comment by Tim Starling (talk | contribs)   09:32, 7 September 2010

The problem with having a function called addPageEntry() that, in addition to adding a page entry, also destroys its input argument, is that for developers other than yourself, the behaviour is likely to be unexpected and confusing and so it may lead to bugs in the future. As I see it, this can be resolved in one of two ways:

  • Have the caller reset $entry instead. The code would be longer, and it still leaves the unexpected change to $prevPage, which would have to be documented.
  • Use object context. Methods are expected to change the contents of $this, they're usually not expected to change their inputs. A dedicated tiffinfo parser class could have $prevPage, $entry and $metadata as object members instead of local variables, and then a method called, say, nextPage() could do what it needed to do without having any reference arguments at all.
#Comment by Duesentrieb (talk | contribs)   09:46, 7 September 2010

i agree that it's rather ugly. having the called reset $entry would work, but the other params would still be modified. using object context is better, but using the *handler* as the context would suck. it would have to be a tiffinfo-parser-state object. which feels like a lot of overkill for the two places where this code is actually called from.

using a private method with reference params seems the simplest way to do it, but i agree that it's not terribly clear. Not really sure what to do now, though.

#Comment by Duesentrieb (talk | contribs)   09:49, 7 September 2010

another thing: refactoring this now will mix stuff from r72278 and r72279. don't know how to pick this apart... perhaps merge it as it is now, then fix the addPageEntry() uglyness later?

#Comment by Duesentrieb (talk | contribs)   09:50, 10 September 2010

I have tried to address these issues in r72706. However, that change is based on a later version, so it's not quite a replacement for this patch.

Status & tagging log