r76424 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r76423‎ | r76424 | r76425 >
Date:02:41, 10 November 2010
Author:neilk
Status:ok (Comments)
Tags:
Comment:
improved internationalization logic and error messages when a localized tutorial not available
Modified paths:
  • /trunk/extensions/UploadWizard/SpecialUploadWizard.php (modified) (history)
  • /trunk/extensions/UploadWizard/UploadWizard.i18n.php (modified) (history)

Diff [purge]

Index: trunk/extensions/UploadWizard/UploadWizard.i18n.php
@@ -32,7 +32,9 @@
3333 'mwe-upwiz-step-details' => 'Describe',
3434 'mwe-upwiz-step-thanks' => 'Use',
3535 'mwe-upwiz-api-error-code' => 'API error code: $1',
36 - 'mwe-upwiz-tutorial-error' => 'A tutorial about what images you can upload to this wiki should appear here. Please contact the system administrator.',
 36+ 'mwe-upwiz-tutorial-error-localized-file-missing' => 'Sorry, we could not find a tutorial in your language. The English one is shown instead.',
 37+ 'mwe-upwiz-tutorial-error-file-missing' => 'Sorry, we could not find any files for the tutorial that is supposed to go here. Please contact the system administrators.',
 38+ 'mwe-upwiz-tutorial-error-cannot-transform' => 'Sorry, we could not get a scaled image of the tutorial to fit this screen. This may be a temporary problem with Wikimedia Commons; try again later.',
3739 'mwe-upwiz-help-desk' => 'Help Desk',
3840 'mwe-upwiz-intro' => "Welcome to Wikimedia Commons, a repository of images, sounds and movies that anyone can freely download and use.
3941 Add to humanity's knowledge by uploading files that could be used for an educational purpose.",
Index: trunk/extensions/UploadWizard/SpecialUploadWizard.php
@@ -53,7 +53,7 @@
5454 * @param subpage, e.g. the "foo" in Special:UploadWizard/foo.
5555 */
5656 public function execute( $subPage ) {
57 - global $wgLang, $wgUser, $wgOut, $wgLanguageCode, $wgExtensionAssetsPath,
 57+ global $wgLang, $wgUser, $wgOut, $wgExtensionAssetsPath,
5858 $wgUploadWizardDebug, $wgUploadWizardDisableResourceLoader;
5959
6060 // side effects: if we can't upload, will print error page to wgOut
@@ -81,7 +81,7 @@
8282 $wgOut->addModules( 'ext.uploadWizard' );
8383 } else {
8484 $basepath = "$wgExtensionAssetsPath/UploadWizard";
85 - $dependencyLoader = new UploadWizardDependencyLoader( $wgLanguageCode );
 85+ $dependencyLoader = new UploadWizardDependencyLoader( $wgLang->getCode() );
8686 if ( $wgUploadWizardDebug ) {
8787 // each file as an individual script or style
8888 $dependencyLoader->outputHtmlDebug( $wgOut, $basepath );
@@ -191,22 +191,35 @@
192192 * @return {String} html that will display the tutorial.
193193 */
194194 function getTutorialHtml() {
195 - global $wgLanguageCode;
 195+ global $wgLang;
196196
197 - // assume failure (this will be replaced if we're successful)
198 - $tutorialHtml = '<p class="errorbox">' . wfMsg( 'mwe-upwiz-tutorial-error' ) . '</p>';
 197+ $error = null;
 198+ $errorHtml = '';
 199+ $tutorialHtml = '';
199200
200201 // get a valid language code, even if the global is wrong
201 - $langCode = $wgLanguageCode;
 202+ if ( $wgLang ) {
 203+ $langCode = $wgLang->getCode();
 204+ }
202205 if ( !isset( $langCode) or $langCode === '' ) {
203206 $langCode = 'en';
204207 }
205208
206 - $tutorialName = str_replace( '$1', $langCode, self::TUTORIAL_NAME_TEMPLATE );
207 - $tutorialTitle = Title::newFromText( $tutorialName, NS_FILE );
 209+ $tutorialFile = false;
 210+ // getTutorialFile returns false if it can't find the right file
 211+ if ( ! $tutorialFile = self::getTutorialFile( $langCode ) ) {
 212+ $error = 'localized-file-missing';
 213+ if ( $langCode !== 'en' ) {
 214+ $tutorialFile = self::getTutorialFile( 'en' );
 215+ }
 216+ }
208217
209 - // wfFindFile() returns a File object, or false
210 - if ( $tutorialFile = wfFindFile( $tutorialTitle ) ) {
 218+ // at this point, we have one of the following situations:
 219+ // $errors is empty, and tutorialFile is the right one for this language
 220+ // $errors notes we couldn't find the tutorialFile for your language, and $tutorialFile is the english one
 221+ // $errors notes we couldn't find the tutorialFile for your language, and $tutorialFile is still false
 222+
 223+ if ( $tutorialFile ) {
211224 // XXX TODO if the client can handle SVG, we could also just send it the unscaled thumb, client-scaled into a DIV or something.
212225 // if ( client can handle SVG ) {
213226 // $tutorialThumbnailImage->getUnscaledThumb();
@@ -214,34 +227,62 @@
215228 // put it into a div of appropriate dimensions.
216229
217230 // n.b. File::transform() returns false if failed, MediaTransformOutput otherwise
218 - if ( $tutorialThumbnailImage = $tutorialFile->transform( array( 'width' => self::TUTORIAL_WIDTH_PX ) ) ) {
219 - // here we use the not-yet-forgotten HTML imagemap to add a clickable area to the tutorial image.
220 - // we could do more special effects with hovers and images and such, not to mention SVG scripting,
221 - // but we aren't sure what we want yet...
222 - $img = Html::element( 'img', array(
223 - 'src' => $tutorialThumbnailImage->getUrl(),
224 - 'width' => $tutorialThumbnailImage->getWidth(),
225 - 'height' => $tutorialThumbnailImage->getHeight(),
226 - 'usemap' => '#' . self::TUTORIAL_IMAGEMAP_ID
227 - ) );
228 - $areaAltText = wfMsg( 'mwe-upwiz-help-desk' );
229 - $area = Html::element( 'area', array(
230 - 'shape' => 'rect',
231 - 'coords' => self::TUTORIAL_HELPDESK_BUTTON_COORDS,
232 - 'href' => self::TUTORIAL_HELPDESK_URL,
233 - 'alt' => $areaAltText,
234 - 'title' => $areaAltText
235 - ) );
236 - $map = Html::rawElement( 'map', array( 'id' => self::TUTORIAL_IMAGEMAP_ID, 'name' => self::TUTORIAL_IMAGEMAP_ID ), $area );
237 - $tutorialHtml = $map . $img;
 231+ if ( $thumbnailImage = $tutorialFile->transform( array( 'width' => self::TUTORIAL_WIDTH_PX ) ) ) {
 232+ $tutorialHtml = self::getTutorialImageHtml( $thumbnailImage );
 233+ } else {
 234+ $errors = 'cannot-transform';
238235 }
 236+ } else {
 237+ $error = 'file-missing';
239238 }
 239+
 240+ if ( isset( $error ) ) {
 241+ $errorHtml = Html::element( 'p', array( 'class' => 'errorbox', 'style' => 'float: none;' ), wfMsg( 'mwe-upwiz-tutorial-error-' . $error ) );
 242+ }
 243+
 244+ return $errorHtml . $tutorialHtml;
240245
241 - return $tutorialHtml;
242 -
243246 }
244247
245248 /**
 249+ * Get tutorial file for a particular language, or false if not available.
 250+ * @param {String} $langCode: language Code
 251+ * @return {File|false}
 252+ */
 253+ function getTutorialFile( $langCode ) {
 254+ $tutorialName = str_replace( '$1', $langCode, self::TUTORIAL_NAME_TEMPLATE );
 255+ $tutorialTitle = Title::newFromText( $tutorialName, NS_FILE );
 256+ return wfFindFile( $tutorialTitle );
 257+ }
 258+
 259+ /**
 260+ * Constructs HTML for the tutorial (laboriously), including an imagemap for the clickable "Help desk" button.
 261+ * @param {ThumbnailImage} $thumb
 262+ * @return {String} HTML representing the image, with clickable helpdesk button
 263+ */
 264+ function getTutorialImageHtml( $thumb ) {
 265+ // here we use the not-yet-forgotten HTML imagemap to add a clickable area to the tutorial image.
 266+ // we could do more special effects with hovers and images and such, not to mention SVG scripting,
 267+ // but we aren't sure what we want yet...
 268+ $img = Html::element( 'img', array(
 269+ 'src' => $thumb->getUrl(),
 270+ 'width' => $thumb->getWidth(),
 271+ 'height' => $thumb->getHeight(),
 272+ 'usemap' => '#' . self::TUTORIAL_IMAGEMAP_ID
 273+ ) );
 274+ $areaAltText = wfMsg( 'mwe-upwiz-help-desk' );
 275+ $area = Html::element( 'area', array(
 276+ 'shape' => 'rect',
 277+ 'coords' => self::TUTORIAL_HELPDESK_BUTTON_COORDS,
 278+ 'href' => self::TUTORIAL_HELPDESK_URL,
 279+ 'alt' => $areaAltText,
 280+ 'title' => $areaAltText
 281+ ) );
 282+ $map = Html::rawElement( 'map', array( 'id' => self::TUTORIAL_IMAGEMAP_ID, 'name' => self::TUTORIAL_IMAGEMAP_ID ), $area );
 283+ return $map . $img;
 284+ }
 285+
 286+ /**
246287 * Return the basic HTML structure for the entire page
247288 * Will be enhanced by the javascript to actually do stuff
248289 * @return {String} html

Comments

#Comment by Catrope (talk | contribs)   22:13, 13 November 2010
+		if ( $wgLang ) {
+			$langCode = $wgLang->getCode();
+		}

$wgLang is never null, and there's lots of code around using $wgLang unchecked, which is totally safe.

+		if ( isset( $error ) ) {

Don't use isset() when what you really want is check for null. The variable you're checking is explicitly and unconditionally set (to null). Write what you mean.

#Comment by NeilK (talk | contribs)   01:30, 16 November 2010

fixed in r76753

Status & tagging log