r69216 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r69215‎ | r69216 | r69217 >
Date:20:24, 9 July 2010
Author:bawolff
Status:resolved (Comments)
Tags:
Comment:
Some fixups. esp related to GPSAltitude tag.
Modified paths:
  • /branches/img_metadata/phase3/includes/Exif.php (modified) (history)
  • /branches/img_metadata/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /branches/img_metadata/phase3/maintenance/language/messageTypes.inc (modified) (history)
  • /branches/img_metadata/phase3/maintenance/language/messages.inc (modified) (history)

Diff [purge]

Index: branches/img_metadata/phase3/maintenance/language/messages.inc
——@@ -2880,8 +2880,8 @@
28812881 'exif-gpslongitude-w',
28822882 ),
28832883 'exif-altituderef' => array(
2884 - 'exif-exif-gpsaltitude-0',
2885 - 'exif-exif-gpsaltitude-1',
 2884+ 'exif-gpsaltitude-above-sealevel',
 2885+ 'exif-gpsaltitude-below-sealevel',
28862886 ),
28872887 'exif-gpsstatus' => array(
28882888 'exif-gpsstatus-a',
Index: branches/img_metadata/phase3/maintenance/language/messageTypes.inc
——@@ -584,8 +584,8 @@
585585 'exif-gpslatitude-s',
586586 'exif-gpslongitude-e',
587587 'exif-gpslongitude-w',
588 - 'exif-gpsaltitude-0',
589 - 'exif-gpsaltitude-1',
 588+ 'exif-gpsaltitude-above-sealevel',
 589+ 'exif-gpsaltitude-below-sealevel',
590590 'exif-gpsstatus-a',
591591 'exif-gpsstatus-v',
592592 'exif-gpsmeasuremode-2',
Index: branches/img_metadata/phase3/includes/Exif.php
——@@ -97,7 +97,7 @@
9898 *
9999 * @param $file String: filename.
100100 * @fixme the following are broke:
101 - * SubjectArea, GPSAltitudeRef, possibly more, need more testing.
 101+ * SubjectArea. Need to test the more obscure tags.
102102 *
103103 * DigitalZoomRatio = 0/0 is rejected. need to determine if thats valid.
104104 * possibly should treat 0/0 = 0. need to read exif spec on that.
——@@ -232,12 +232,12 @@
233233
234234 # GPS Attribute Information (p52)
235235 'GPS' => array(
236 - 'GPSVersionID' => array( Exif::BYTE, 4 ), # GPS tag version
 236+ 'GPSVersion' => Exif::UNDEFINED, # array( Exif::BYTE, 4 ) GPS tag version. Note exif standard calls this GPSVersionID, but php doesn't like the id. also php thinks its wrong type
237237 'GPSLatitudeRef' => Exif::ASCII, # North or South Latitude #p52-53
238238 'GPSLatitude' => array( Exif::RATIONAL, 3 ), # Latitude
239239 'GPSLongitudeRef' => Exif::ASCII, # East or West Longitude #p53
240240 'GPSLongitude' => array( Exif::RATIONAL, 3), # Longitude
241 - 'GPSAltitudeRef' => Exif::BYTE, # Altitude reference
 241+ 'GPSAltitudeRef' => Exif::UNDEFINED, # Altitude reference. Note, the exif standard says this should be an EXIF::Byte, but php seems to disagree.
242242 'GPSAltitude' => Exif::RATIONAL, # Altitude
243243 'GPSTimeStamp' => array( Exif::RATIONAL, 3), # GPS time (atomic clock)
244244 'GPSSatellites' => Exif::ASCII, # Satellites used for measurement
——@@ -281,7 +281,6 @@
282282 $this->mRawExifData = $data ? $data : array();
283283 $this->makeFilteredData();
284284 $this->collapseData();
285 - $this->makeFormattedData();
286285 $this->debugFile( __FUNCTION__, false );
287286 }
288287
——@@ -342,20 +341,21 @@
343342 $this->exifGPStoNumber( 'GPSDestLongitude' );
344343
345344 if ( isset( $this->mFilteredExifData['GPSAltitude'] ) && isset( $this->mFilteredExifData['GPSAltitudeRef'] ) ) {
346 - if ( $this->mFilteredExifData['GPSAltitudeRef'] === 1 ) {
 345+ if ( $this->mFilteredExifData['GPSAltitudeRef'] === "\1" ) {
347346 $this->mFilteredExifData['GPSAltitude'] *= - 1;
348347 }
349 - unset( $this->mFilteredExifData );
 348+ unset( $this->mFilteredExifData['GPSAltitudeRef'] );
350349 }
351350
352 - $this->toHex( 'FileSource', true );
353 - $this->toHex( 'SceneType', true );
 351+ $this->getOrd( 'FileSource' );
 352+ $this->getOrd( 'SceneType' );
354353
355354 $this->charCodeString( 'UserComment' );
356355 $this->charCodeString( 'GPSProcessingMethod');
357356 $this->charCodeString( 'GPSAreaInformation' );
358357
359358 //ComponentsConfiguration should really be an array instead of a string...
 359+
360360 if ( isset ( $this->mFilteredExifData['ComponentsConfiguration'] ) ) {
361361 $val = $this->mFilteredExifData['ComponentsConfiguration'];
362362 $ccVals = array();
——@@ -365,6 +365,21 @@
366366 $ccVals['_type'] = 'ol'; //this is for formatting later.
367367 $this->mFilteredExifData['ComponentsConfiguration'] = $ccVals;
368368 }
 369+
 370+ //GPSVersion(ID) is just very screwed up by php
 371+ //put into a version string, and change the tag name.
 372+ if ( isset ( $this->mFilteredExifData['GPSVersion'] ) ) {
 373+ $val = $this->mFilteredExifData['GPSVersion'];
 374+ $newVal = '';
 375+ for ($i = 0; $i < strlen($val); $i++) {
 376+ if ( $i !== 0 ) {
 377+ $newVal .= '.';
 378+ }
 379+ $newVal .= ord( substr($val, strlen($val) - $i - 1, 1) );
 380+ }
 381+ $this->mFilteredExifData['GPSVersionID'] = $newVal;
 382+ unset( $this->mFilteredExifData['GPSVersion'] );
 383+ }
369384
370385 }
371386 /**
——@@ -375,7 +390,7 @@
376391 * @param $prop String prop name.
377392 * @todo this is in need of testing.
378393 */
379 - function charCodeString ( $prop ) {
 394+ private function charCodeString ( $prop ) {
380395 if ( isset( $this->mFilteredExifData[$prop] ) ) {
381396
382397 if ( strlen($this->mFilteredExifData[$prop]) <= 8 ) {
——@@ -414,21 +429,13 @@
415430 }
416431 /**
417432 * Convert an Exif::UNDEFINED from a raw binary string
418 - * to a hex string. This is sometimes needed depending on
 433+ * to its value. This is sometimes needed depending on
419434 * the type of UNDEFINED field
420435 * @param $prop String name of property
421 - * @param $stripLeadingZero boolean should it strip first leading zero
422 - * if present on result (bit of a hack).
423436 */
424 - function toHex ( $prop, $stripLeadingZero = false ) {
 437+ private function getOrd ( $prop ) {
425438 if ( isset( $this->mFilteredExifData[$prop] ) ) {
426 - $val = bin2hex( $this->mFilteredExifData[$prop] );
427 - if ( $stripLeadingZero ) {
428 - if (substr( $val, 0, 1) === '0') {
429 - $val = substr( $val, 1);
430 - }
431 - }
432 - $this->mFilteredExifData[$prop] = $val;
 439+ $this->mFilteredExifData[$prop] = ord( $this->mFilteredExifData[$prop] );
433440 }
434441 }
435442 /**
——@@ -436,7 +443,7 @@
437444 * for example 10 degress 20`40`` S -> -10.34444
438445 * @param String $prop a gps coordinate exif tag name (like GPSLongitude)
439446 */
440 - function exifGPStoNumber ( $prop ) {
 447+ private function exifGPStoNumber ( $prop ) {
441448 $loc =& $this->mFilteredExifData[$prop];
442449 $dir =& $this->mFilteredExifData[$prop . 'Ref'];
443450 $res = false;
——@@ -466,9 +473,10 @@
467474 }
468475
469476 /**
470 - * @todo document
 477+ * Use FormatExif to create formatted values for display to user
 478+ * (is this ever used?)
471479 */
472 - function makeFormattedData( ) {
 480+ private function makeFormattedData( ) {
473481 $format = new FormatExif( $this->getFilteredData() );
474482 $this->mFormattedExifData = $format->getFormattedData();
475483 }
——@@ -495,6 +503,9 @@
496504 * Get $this->mFormattedExifData
497505 */
498506 function getFormattedData() {
 507+ if (!$this->mFormattedExifData) {
 508+ $this->makeFormattedData();
 509+ }
499510 return $this->mFormattedExifData;
500511 }
501512 /**#@-*/
——@@ -523,7 +534,7 @@
524535 * @param $in Mixed: the input value to check
525536 * @return bool
526537 */
527 - function isByte( $in ) {
 538+ private function isByte( $in ) {
528539 if ( !is_array( $in ) && sprintf('%d', $in) == $in && $in >= 0 && $in <= 255 ) {
529540 $this->debug( $in, __FUNCTION__, true );
530541 return true;
——@@ -533,7 +544,7 @@
534545 }
535546 }
536547
537 - function isASCII( $in ) {
 548+ private function isASCII( $in ) {
538549 if ( is_array( $in ) ) {
539550 return false;
540551 }
——@@ -551,7 +562,7 @@
552563 return true;
553564 }
554565
555 - function isShort( $in ) {
 566+ private function isShort( $in ) {
556567 if ( !is_array( $in ) && sprintf('%d', $in) == $in && $in >= 0 && $in <= 65536 ) {
557568 $this->debug( $in, __FUNCTION__, true );
558569 return true;
——@@ -561,7 +572,7 @@
562573 }
563574 }
564575
565 - function isLong( $in ) {
 576+ private function isLong( $in ) {
566577 if ( !is_array( $in ) && sprintf('%d', $in) == $in && $in >= 0 && $in <= 4294967296 ) {
567578 $this->debug( $in, __FUNCTION__, true );
568579 return true;
——@@ -571,7 +582,7 @@
572583 }
573584 }
574585
575 - function isRational( $in ) {
 586+ private function isRational( $in ) {
576587 $m = array();
577588 if ( !is_array( $in ) && @preg_match( '/^(\d+)\/(\d+[1-9]|[1-9]\d*)$/', $in, $m ) ) { # Avoid division by zero
578589 return $this->isLong( $m[1] ) && $this->isLong( $m[2] );
——@@ -581,7 +592,7 @@
582593 }
583594 }
584595
585 - function isUndefined( $in ) {
 596+ private function isUndefined( $in ) {
586597
587598 $this->debug( $in, __FUNCTION__, true );
588599 return true;
——@@ -599,7 +610,7 @@
600611 */
601612 }
602613
603 - function isSlong( $in ) {
 614+ private function isSlong( $in ) {
604615 if ( $this->isLong( abs( $in ) ) ) {
605616 $this->debug( $in, __FUNCTION__, true );
606617 return true;
——@@ -609,7 +620,7 @@
610621 }
611622 }
612623
613 - function isSrational( $in ) {
 624+ private function isSrational( $in ) {
614625 $m = array();
615626 if ( !is_array( $in ) && preg_match( '/^(\d+)\/(\d+[1-9]|[1-9]\d*)$/', $in, $m ) ) { # Avoid division by zero
616627 return $this->isSlong( $m[0] ) && $this->isSlong( $m[1] );
——@@ -630,7 +641,7 @@
631642 * @param $recursive Boolean: true if called recursively for array types.
632643 * @return bool
633644 */
634 - function validate( $section, $tag, $val, $recursive = false ) {
 645+ private function validate( $section, $tag, $val, $recursive = false ) {
635646 $debug = "tag is '$tag'";
636647 $etype = $this->mExifTags[$section][$tag];
637648 $ecount = 1;
——@@ -699,7 +710,7 @@
700711 * @param $fname String:
701712 * @param $action Mixed: , default NULL.
702713 */
703 - function debug( $in, $fname, $action = null ) {
 714+ private function debug( $in, $fname, $action = null ) {
704715 if ( !$this->log ) {
705716 return;
706717 }
——@@ -726,7 +737,7 @@
727738 * @param $fname String: the name of the function calling this function
728739 * @param $io Boolean: Specify whether we're beginning or ending
729740 */
730 - function debugFile( $fname, $io ) {
 741+ private function debugFile( $fname, $io ) {
731742 if ( !$this->log ) {
732743 return;
733744 }
——@@ -1126,6 +1137,14 @@
11271138 }
11281139 break;
11291140
 1141+ case 'GPSAltitude':
 1142+ if ( $val < 0 ) {
 1143+ $val = $this->msg( 'GPSAltitude', 'below-sealevel', $this->formatNum( -$val ) );
 1144+ } else {
 1145+ $val = $this->msg( 'GPSAltitude', 'above-sealevel', $this->formatNum( $val ) );
 1146+ }
 1147+ break;
 1148+
11301149 case 'GPSStatus':
11311150 switch( $val ) {
11321151 case 'A': case 'V':
——@@ -1166,17 +1185,6 @@
11671186 $val = $wgLang->date( substr( $val, 0, 4 ) . substr( $val, 5, 2 ) . substr( $val, 8, 2 ) . '000000' );
11681187 break;
11691188
1170 - case 'GPSAltitudeRef':
1171 - switch( $val ) {
1172 - case 0: case 1:
1173 - $tags[$tag] = $this->msg( 'GPSAltitude', $val );
1174 - break;
1175 - default:
1176 - $tags[$tag] = $val;
1177 - break;
1178 - }
1179 - break;
1180 -
11811189 case 'GPSLatitude':
11821190 case 'GPSDestLatitude':
11831191 $val = $this->formatCoords( $val, 'latitude' );
——@@ -1240,6 +1248,7 @@
12411249 case 'ImageDescription':
12421250 case 'Artist':
12431251 case 'Copyright':
 1252+ case 'GPSVersionID':
12441253 $val = htmlspecialchars( $val );
12451254 break;
12461255
——@@ -1419,6 +1428,7 @@
14201429 function formatCoords( $coord, $type ) {
14211430 $ref = '';
14221431 if ( $coord < 0 ) {
 1432+ $nCoord = -$coord;
14231433 if ( $type === 'latitude' ) {
14241434 $ref = 'S';
14251435 }
——@@ -1427,6 +1437,7 @@
14281438 }
14291439 }
14301440 else {
 1441+ $nCoord = $coord;
14311442 if ( $type === 'latitude' ) {
14321443 $ref = 'N';
14331444 }
——@@ -1435,9 +1446,9 @@
14361447 }
14371448 }
14381449
1439 - $deg = floor( $coord );
1440 - $min = floor( ( $coord - $deg ) * 60.0 );
1441 - $sec = round( ( ( $coord - $deg ) - $min / 60 ) * 3600, 2 );
 1450+ $deg = floor( $nCoord );
 1451+ $min = floor( ( $nCoord - $deg ) * 60.0 );
 1452+ $sec = round( ( ( $nCoord - $deg ) - $min / 60 ) * 3600, 2 );
14421453
14431454 $deg = $this->formatNum( $deg );
14441455 $min = $this->formatNum( $min );
Index: branches/img_metadata/phase3/languages/messages/MessagesEn.php
——@@ -3915,9 +3915,9 @@
39163916 'exif-gpslongitude-e' => 'East longitude',
39173917 'exif-gpslongitude-w' => 'West longitude',
39183918
3919 -# Pseudotags used for GPSAltitudeRef
3920 -'exif-gpsaltitude-0' => 'Metres above sea level',
3921 -'exif-gpsaltitude-1' => 'Metres below sea level',
 3919+# Pseudotags used for GPSAltitude
 3920+'exif-gpsaltitude-above-sealevel' => '$1 Metres above sea level',
 3921+'exif-gpsaltitude-below-sealevel' => '$1 Metres below sea level',
39223922
39233923 'exif-gpsstatus-a' => 'Measurement in progress',
39243924 'exif-gpsstatus-v' => 'Measurement interoperability',

Follow-up revisions

RevisionCommit summaryAuthorDate
r69273Follow up to r69216. Address comments.bawolff21:07, 11 July 2010

Comments

#Comment by Nikerabbit (talk | contribs)   07:59, 10 July 2010 
+				'GPSVersion' => Exif::UNDEFINED,			 # array( Exif::BYTE, 4 )  GPS tag version. Note exif standard calls this GPSVersionID, but php doesn't like the id. also php thinks its wrong type

I'd suggest putting the comments on their own line (multiple if needed). 

+'exif-gpsaltitude-above-sealevel' => '$1 Metres above sea level',

Use "meters", compare with: 

'exif-subjectdistance-value' => '$1 meters',

+		//GPSVersion(ID) is just very screwed up by php
+		//put into a version string, and change the tag name.

Put what? Since the second line starts with small letter and the first one doesn't contain a dot, I was even more confused about this comment.

I also don't quite understand what the following loop does, maybe add a comment describing the input and output? 

+	private function getOrd ( $prop ) {

I wouldn't call something getOrd which always returns null and has side effects.

#Comment by Bawolff (talk | contribs)   21:08, 11 July 2010

Thank you for the comments. I tried to address them in r69273.

Status & tagging log