r99941 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r99940‎ | r99941 | r99942 >
Date:04:39, 16 October 2011
Author:erik
Status:ok (Comments)
Tags:
Comment:
Applied modified patch by Ben Hartshorne <bhartshorne at wikimedia dot org>:
Leverage MW 1.18's improved image metadata extraction to automatically
extract GPS coordinates, add them to the review workflow, and add a
{{Location dec}} template to the file description page if they have been
provided. This also adds a latitude/longitude fields that the user can
manually fill in, which ultimately should be replacd with a location
picker widget.

Tested in IE7, IE8, Chrome 13, Opera 10, FF7, with single and multiple
files, with and without coordinates.
Modified paths:
  • /trunk/extensions/UploadWizard/UploadWizard.i18n.php (modified) (history)
  • /trunk/extensions/UploadWizard/UploadWizardHooks.php (modified) (history)
  • /trunk/extensions/UploadWizard/resources/mw.UploadWizardDetails.js (modified) (history)
  • /trunk/extensions/UploadWizard/resources/uploadWizard.css (modified) (history)

Diff [purge]

Index: trunk/extensions/UploadWizard/UploadWizardHooks.php
——@@ -213,6 +213,8 @@
214214 'mwe-upwiz-media-type',
215215 'mwe-upwiz-date-created',
216216 'mwe-upwiz-location',
 217+ 'mwe-upwiz-location-lat',
 218+ 'mwe-upwiz-location-lon',
217219 'mwe-upwiz-copyright-info',
218220 'mwe-upwiz-author',
219221 'mwe-upwiz-autoconverted',
——@@ -259,6 +261,7 @@
260262 'mwe-upwiz-tooltip-date',
261263 'mwe-upwiz-tooltip-categories',
262264 'mwe-upwiz-tooltip-other',
 265+ 'mwe-upwiz-tooltip-location',
263266 'mwe-upwiz-tooltip-more-info',
264267 'mwe-upwiz-file-need-file',
265268 'mwe-upwiz-deeds-need-deed',
Index: trunk/extensions/UploadWizard/UploadWizard.i18n.php
——@@ -121,6 +121,8 @@
122122 'mwe-upwiz-media-type' => 'Media type',
123123 'mwe-upwiz-date-created' => 'Date created',
124124 'mwe-upwiz-location' => 'Location',
 125+ 'mwe-upwiz-location-lat' => 'Latitude',
 126+ 'mwe-upwiz-location-lon' => 'Longitude',
125127 'mwe-upwiz-copyright-info' => 'Release rights',
126128 'mwe-upwiz-author' => 'Author(s)',
127129 'mwe-upwiz-autoconverted' => 'This file was automatically converted to the $1 format',
——@@ -170,6 +172,7 @@
171173 'mwe-upwiz-tooltip-date' => 'Date this work was created or first published (YYYY-MM-DD format).',
172174 'mwe-upwiz-tooltip-categories' => 'Add [$1 categories] to your file to make it easier to find.',
173175 'mwe-upwiz-tooltip-other' => 'Any other information you want to include about this work — geographic coordinates, links to other versions, etc.',
 176+ 'mwe-upwiz-tooltip-location' => 'Coordinates of the location where this media file was created.',
174177 'mwe-upwiz-tooltip-more-info' => 'Learn more.',
175178 'mwe-upwiz-file-need-file' => 'Please add an upload first.',
176179 'mwe-upwiz-deeds-need-deed' => 'Please explain where you got {{PLURAL:$1|this file|these files}} and how this site can use {{PLURAL:$1|it|them}}, by selecting one of the options.',
——@@ -436,7 +439,9 @@
437440
438441 {{Identical|Title}}',
439442 'mwe-upwiz-date-created' => '[[File:Commons-uw-L52P.png|right|thumb|Screenshot showing a sample of this message]]',
440 - 'mwe-upwiz-location' => '{{Identical|Location}}',
 443+ 'mwe-upwiz-location' => '{{Identical|Location}} - the location the media exists on the planet, further described by lat and lon',
 444+ 'mwe-upwiz-location-lat' => 'Latitude - the GPS coordinate, expressed in signed decimal degrees',
 445+ 'mwe-upwiz-location-lon' => 'Longitude - the GPS coordinate, expressed in signed decimal degrees',
441446 'mwe-upwiz-other' => '[[File:Commons-uw-L52P.png|right|thumb|Screenshot showing a sample of this message]]
442447
443448 {{Identical|Other information}}',
Index: trunk/extensions/UploadWizard/resources/uploadWizard.css
——@@ -706,3 +706,11 @@
707707 padding: 0.5em;
708708 }
709709
 710+.mwe-location-lat {
 711+ float:left;
 712+ font-style:italic;
 713+}
 714+
 715+.mwe-location-lon {
 716+ font-style:italic;
 717+}
Index: trunk/extensions/UploadWizard/resources/mw.UploadWizardDetails.js
——@@ -156,11 +156,30 @@
157157 .growTextArea();
158158
159159 var otherInformationDiv = $j('<div></div>')
160 - .append( $j( '<div class="mwe-upwiz-details-more-label">' ).append( gM( 'mwe-upwiz-other' ) ).addHint( 'other' ) )
 160+ .append( $j( '<div class="mwe-upwiz-details-more-label"></div>' ).append( gM( 'mwe-upwiz-other' ) ).addHint( 'other' ) )
161161 .append( _this.otherInformationInput );
162162
 163+ var latId = "location-latitude" + _this.upload.index;
 164+ var lonId = "location-longitude" + _this.upload.index;
 165+ _this.latInput = $j( '<input type="text" id="' + latId + '" name="' + latId + '" type="text" class="mwe-loc-lat" size="10"/>' );
 166+ _this.lonInput = $j( '<input type="text" id="' + lonId + '" name="' + lonId + '" type="text" class="mwe-loc-lon" size="10"/>' );
 167+
 168+ var latDiv = $j( '<div class="mwe-location-lat"></div>' )
 169+ .append( $j ( '<div class="mwe-location-lat-label"></div>' ).append( gM( 'mwe-upwiz-location-lat' ) ) )
 170+ .append( _this.latInput );
 171+ var lonDiv = $j( '<div class="mwe-location-lon"></div>' )
 172+ .append( $j ( '<div class="mwe-location-lon-label"></div>' ).append( gM( 'mwe-upwiz-location-lon' ) ) )
 173+ .append( _this.lonInput );
 174+
 175+ var locationDiv = $j( '<div class="mwe-location"></div>' )
 176+ .append( $j ('<div class="mwe-location-label"></div>' )
 177+ .append( gM( 'mwe-upwiz-location' ) )
 178+ .addHint( 'location' ) )
 179+ .append( latDiv )
 180+ .append( lonDiv );
 181+
163182 $j( moreDetailsDiv ).append(
164 - // location goes here
 183+ locationDiv,
165184 $categoriesDiv,
166185 otherInformationDiv
167186 );
——@@ -611,36 +630,23 @@
612631 /**
613632 * Prefill location inputs (and/or scroll to position on map) from image info and metadata
614633 *
615 - * At least for my test images, the EXIF parser on MediaWiki is not giving back any data for
616 - * GPSLatitude, GPSLongitude, or GPSAltitudeRef. It is giving the lat/long Refs, the Altitude, and the MapDatum
617 - * So, this is broken until we fix MediaWiki's parser, OR, parse it ourselves somehow
618 - *
619 - * in Image namespace
620 - * GPSTag Long ??
621 - *
622 - * in GPSInfo namespace
623 - * GPSVersionID byte* 2000 = 2.0.0.0
624 - * GPSLatitude rational
625 - * GPSLatitudeRef ascii (N | S) or North | South
626 - * GPSLongitude rational
627 - * GPSLongitudeRef ascii (E | W) or East | West
628 - * GPSAltitude rational
629 - * GPSAltitudeRef byte (0 | 1) above or below sea level
630 - * GPSImgDirection rational
631 - * GPSImgDirectionRef ascii (M | T) magnetic or true north
632 - * GPSMapDatum ascii "WGS-84" is the standard
633 - *
634 - * A 'rational' is a string like this:
635 - * "53/1 0/1 201867/4096" --> 53 deg 0 min 49.284 seconds
636 - * "2/1 11/1 64639/4096" --> 2 deg 11 min 15.781 seconds
637 - * "122/1" -- 122 m (altitude)
 634+ * As of MediaWiki 1.18, the exif parser translates the rational GPS data tagged by the camera
 635+ * to decimal format. Let's just use that.
 636+ * Leaving out altitude ref for now (for no good reason).
638637 */
639638 prefillLocation: function() {
640 - /* unimplemented -- awaiting bawolff's GSoC 2010 project to be committedd... */ return;
 639+ _this = this;
 640+ if ( _this.upload.imageinfo.metadata ) {
 641+ $j( _this.latInput ).val( _this.upload.imageinfo.metadata['gpslatitude'] );
 642+ $j( _this.lonInput ).val( _this.upload.imageinfo.metadata['gpslongitude'] );
 643+ }
641644 },
642645
643646 /**
644647 * Given a decimal latitude and longitude, return filled out {{Location}} template
 648+ *
 649+ * The {{Location dec}} template is preferred and makes this conversion unnecessary. This function is not used.
 650+ *
645651 * @param latitude decimal latitude ( -90.0 >= n >= 90.0 ; south = negative )
646652 * @param longitude decimal longitude ( -180.0 >= n >= 180.0 ; west = negative )
647653 * @param scale (optional) how rough the geocoding is.
——@@ -770,11 +776,14 @@
771777 }
772778
773779 wikiText += "=={{int:filedesc}}==\n";
 780+ var lat = $j.trim( $j( _this.latInput ).val() );
 781+ var lon = $j.trim( $j( _this.lonInput ).val() );
 782+ if( lat ){ //only add the tag if the data exists; assume that if lat exists, long will as well
 783+ wikiText += '{{Location dec|'+ lat + '|' + lon + '}}\n';
 784+ }
774785
775786 wikiText += '{{Information\n' + info + '}}\n\n';
776787
777 - // add a location template if possible
778 -
779788 // add an "anything else" template if needed
780789 var otherInfoWikiText = $j.trim( $j( _this.otherInformationInput ).val() );
781790 if ( ! mw.isEmpty( otherInfoWikiText ) ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r101324follow up to r99941, took care of Neils commentsjeroendedauw22:59, 30 October 2011

Comments

#Comment by NeilK (talk | contribs)   21:45, 18 October 2011

Great work -- marking okay

However, it's not quite ready for production yet. A few notes for followups from UW devs or Ben...

- Why not record altitude? It's returned by the API - _this = this -- you probably want var _this = this. JS is not cautiously scoped, you have to say var before everything. - It should try to check lat & long for insane or non-numeric values (there's a validation library that could help) - If latitude is there, longitude may not be there, despite the assumption (thanks for documenting that assumption though) - if ( lat ) is not a strict enough check -- lat can be 0 at the equator. Look for lat === undefined or something - locationDiv should have a margin at the bottom, it's a bit visually crowded

#Comment by NeilK (talk | contribs)   21:45, 18 October 2011

Great work -- marking okay

However, it's not quite ready for production yet. A few notes for followups from UW devs or Ben...

- Why not record altitude? It's returned by the API

- _this = this -- you probably want var _this = this. JS is not cautiously scoped, you have to say var before everything.

- It should try to check lat & long for insane or non-numeric values (there's a validation library that could help)

- If latitude is there, longitude may not be there, despite the assumption (thanks for documenting that assumption though)

- if ( lat ) is not a strict enough check -- lat can be 0 at the equator. Look for lat === undefined or something

- locationDiv should have a margin at the bottom, it's a bit visually crowded

#Comment by NeilK (talk | contribs)   20:28, 26 October 2011

to clarify, the validation library I want is jquery.validate.js -- you can see examples of how it's used throughout UploadWizard, whenever ".rules" and ".messages" are invoked.

basically we need to:

- have a hidden <label> in the form, with a for= attribute for these fields, with the mwe-validator-error class, so it can be called in case of an error. Look at the other examples.

- add .rules to these elements with min(value) and max(value) for lat and long, and possibly add a 'required' which checks: if one is nonblank, then the other is required.

- add .messages for each rule (the error if the value isn't correct)

- obviously, add the entries in UploadWizard.i18n.php and UploadWizardHooks.php for the new messages

Status & tagging log