Phuedx has uploaded a new change for review.
https://gerrit.wikimedia.org/r/192788
Change subject: Add the MobileViewBannerImageRepository
......................................................................
Add the MobileViewBannerImageRepository
The pageimages API returns images that have one dimension - sometimes
height - equal to the target width. So use the mobileview API instead.
Also, both the pageimages and mobileview APIs return one image. Update
the BannerImage class and the repository classes to reflect this.
Change-Id: I0143e4b835311bf45fa3521d1335e23586043297
---
M includes/Resources.php
M javascripts/modules/bannerImage/BannerImage.js
A javascripts/modules/bannerImage/MobileViewBannerImageRepository.js
M javascripts/modules/bannerImage/PageImagesBannerImageRepository.js
M javascripts/modules/bannerImage/init.js
A tests/qunit/modules/bannerImage/test_MobileViewBannerImageRepository.js
M tests/qunit/modules/bannerImage/test_PageImagesBannerImageRepository.js
7 files changed, 220 insertions(+), 75 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/MobileFrontend
refs/changes/88/192788/1
diff --git a/includes/Resources.php b/includes/Resources.php
index 6215f80..3c4b55a 100644
--- a/includes/Resources.php
+++ b/includes/Resources.php
@@ -1245,6 +1245,7 @@
'scripts' => array(
'javascripts/modules/bannerImage/Image.js',
'javascripts/modules/bannerImage/PageImagesBannerImageRepository.js',
+
'javascripts/modules/bannerImage/MobileViewBannerImageRepository.js',
'javascripts/modules/bannerImage/BannerImage.js',
),
'styles' => array(
diff --git a/javascripts/modules/bannerImage/BannerImage.js
b/javascripts/modules/bannerImage/BannerImage.js
index 0acf277..aabfc42 100644
--- a/javascripts/modules/bannerImage/BannerImage.js
+++ b/javascripts/modules/bannerImage/BannerImage.js
@@ -5,51 +5,6 @@
ratio = ( browser.isWideScreen() ) ? 21 / 9 : 16 / 9;
/**
- * Tries to load one image from a list of images.
- *
- * The first image from the list is loaded. If it loads, then it is
returned, otherwise the next
- * image in the list is loaded. If the list of images is exhausted then
the loading has
- * completely failed and nothing is returned.
- *
- * @param {Array} images A list of images
- * @returns {jQuery.Promise}
- */
- function loadOneImage( images ) {
-
- /**
- * @ignore
- *
- * @param {Array} images
- * @param {Number} index
- * @param {jQuery.Deferred} deferred
- */
- function loadOneImageAcc( images, index, deferred ) {
- var image;
-
- if ( index >= images.length ) {
- deferred.reject();
-
- return;
- }
-
- image = images[ index ];
- image.load()
- .done( function () {
- deferred.resolve( image );
- } )
- .fail( function () {
- loadOneImageAcc( images, ++index,
deferred );
- } );
- }
-
- var deferred = $.Deferred();
-
- loadOneImageAcc( images, 0, deferred );
-
- return deferred.promise();
- }
-
- /**
* A banner image at the head of the page
* @class BannerImage
* @extends View
@@ -88,14 +43,11 @@
targetWidth = $( window ).width();
self.repository.getImages( targetWidth )
- .then( function ( images ) {
- loadOneImage( images )
- .done( function ( image ) {
- self.onImageLoaded(
image );
- } )
- .fail( function () {
- self.remove();
- } );
+ .then( function ( image ) {
+ self.onImageLoaded( image );
+ } )
+ .fail( function () {
+ self.remove();
} );
},
diff --git a/javascripts/modules/bannerImage/MobileViewBannerImageRepository.js
b/javascripts/modules/bannerImage/MobileViewBannerImageRepository.js
new file mode 100644
index 0000000..33ebb68
--- /dev/null
+++ b/javascripts/modules/bannerImage/MobileViewBannerImageRepository.js
@@ -0,0 +1,88 @@
+ ( function ( M, $ ) {
+
+ var Class = M.require( 'Class' ),
+ Image = M.require( 'modules/bannerImage/Image' ),
+ MobileViewBannerImageRepository;
+
+ /**
+ * Uses the PageImages API to get images that can be used as banner
images
+ * for a page.
+ *
+ * @class
+ */
+ MobileViewBannerImageRepository = Class.extend( {
+
+ /**
+ * @constructor
+ * @param {mw.Api} api A MediaWiki API client
+ * @param {String} title The title of the page
+ */
+ initialize: function ( api, title ) {
+ this.api = api;
+ this.title = title;
+ this.cache = {};
+ },
+
+ /**
+ * Gets images that can be used as banner images.
+ *
+ * @param {Number} targetWidth The target width of the image
+ * @return {$.Promise} A promise that resolves with an array of
Image objects if the
+ * MediaWiki API request was successful or rejects otherwise.
+ */
+ getImages: function ( targetWidth ) {
+ var deferred = $.Deferred(),
+ promise = deferred.promise(),
+ self = this;
+
+ if ( self.cache.hasOwnProperty( targetWidth ) ) {
+ deferred.resolve( self.cache[targetWidth] );
+
+ return promise;
+ }
+
+ self.api.get( {
+ action: 'mobileview',
+ prop: 'thumb',
+ page: self.title,
+ thumbwidth: targetWidth
+ } )
+ .then( function ( data ) {
+ var thumb,
+ image;
+
+ // Page doesn't exist.
+ if ( data.hasOwnProperty( 'error' ) ) {
+ deferred.reject( data.error );
+
+ return;
+ }
+
+ // Page exists but doesn't have thumbnail.
+ if ( !data.mobileview.hasOwnProperty( 'thumb' )
) {
+ // TODO: Should this reject with an
error?
+ deferred.reject();
+
+ return;
+ }
+
+ thumb = data.mobileview.thumb;
+ image = new Image(
+ thumb.url,
+ thumb.width,
+ thumb.height
+ );
+
+ self.cache[targetWidth] = image;
+
+ deferred.resolve( image );
+ } )
+ .fail( deferred.reject );
+
+ return promise;
+ }
+ } );
+
+ M.define( 'modules/bannerImage/MobileViewBannerImageRepository',
MobileViewBannerImageRepository );
+
+} ( mw.mobileFrontend, jQuery ) );
diff --git a/javascripts/modules/bannerImage/PageImagesBannerImageRepository.js
b/javascripts/modules/bannerImage/PageImagesBannerImageRepository.js
index 5dbd640..84f0cea 100644
--- a/javascripts/modules/bannerImage/PageImagesBannerImageRepository.js
+++ b/javascripts/modules/bannerImage/PageImagesBannerImageRepository.js
@@ -47,33 +47,39 @@
pithumbsize: targetWidth
} )
.then( function ( data ) {
- var images = [];
+ var image;
// Page doesn't exist.
if ( data.query.pages.hasOwnProperty( '-1' ) ) {
- deferred.resolve( images );
+ deferred.reject();
return;
}
+ // TODO: Do we have an IE8 Object.keys polyfill
available?
+ // If so, then get rid of this iterator.
$.each( data.query.pages, function ( key, value
) {
var thumbnail = value.thumbnail;
// Page exists but doesn't have
thumbnail.
if ( !thumbnail ) {
- return;
+ deferred.reject();
+
+ return false;
}
- images.push( new Image(
+ image = new Image(
thumbnail.source,
thumbnail.width,
thumbnail.height
- ) );
+ );
+
+ return false;
} );
- self.cache[targetWidth] = images;
+ self.cache[targetWidth] = image;
- deferred.resolve( images );
+ deferred.resolve( image );
} )
.fail( deferred.reject );
diff --git a/javascripts/modules/bannerImage/init.js
b/javascripts/modules/bannerImage/init.js
index bbfd68a..a481d56 100644
--- a/javascripts/modules/bannerImage/init.js
+++ b/javascripts/modules/bannerImage/init.js
@@ -1,7 +1,7 @@
( function ( M ) {
M.require( 'context' ).assertMode( [ 'alpha', 'beta' ] );
- var PageImagesBannerImageRepository = M.require(
'modules/bannerImage/PageImagesBannerImageRepository' ),
+ var MobileViewBannerImageRepository = M.require(
'modules/bannerImage/MobileViewBannerImageRepository' ),
BannerImage = M.require( 'modules/bannerImage/BannerImage' ),
page = M.getCurrentPage(),
repository,
@@ -12,7 +12,7 @@
!page.isMainPage() &&
page.getNamespaceId() === 0
) {
- repository = new PageImagesBannerImageRepository( new mw.Api(),
page.title );
+ repository = new MobileViewBannerImageRepository( new mw.Api(),
page.title );
bannerImage = new BannerImage( {
repository: repository
} );
diff --git
a/tests/qunit/modules/bannerImage/test_MobileViewBannerImageRepository.js
b/tests/qunit/modules/bannerImage/test_MobileViewBannerImageRepository.js
new file mode 100644
index 0000000..8ddb430
--- /dev/null
+++ b/tests/qunit/modules/bannerImage/test_MobileViewBannerImageRepository.js
@@ -0,0 +1,100 @@
+( function ( M, mw, $ ) {
+
+ var MobileViewBannerImageRepository = M.require(
'modules/bannerImage/MobileViewBannerImageRepository' ),
+ Image = M.require( 'modules/bannerImage/Image' );
+
+ QUnit.module( 'MobileFrontend
modules/bannerImage/MobileViewBannerImageRepository', {
+ setup: function () {
+ var api = new mw.Api();
+ this.getDeferred = $.Deferred();
+ this.getSpy = this.stub( api, 'get' ).returns(
this.getDeferred.promise() );
+ this.title = 'Samurai';
+ this.repository = new MobileViewBannerImageRepository(
api, this.title );
+ }
+ } );
+
+ QUnit.test( 'it should try to get the image at the target width', 1,
function ( assert ) {
+ this.repository.getImages( 1234 );
+
+ assert.deepEqual( this.getSpy.lastCall.args[0], {
+ action: 'mobileview',
+ prop: 'thumb',
+ page: this.title,
+ thumbwidth: 1234
+ } );
+ } );
+
+ QUnit.test( 'it should reject if the page doesn\'t exist', 1, function
( assert ) {
+ var error = {
+ code: 'missingtitle',
+ info: 'The page you requested doesn\'t exist',
+ '*': 'See https://en.wikipedia.org/w/api.php for API
usage'
+ };
+
+ this.getDeferred.resolve( {
+ error: error
+ } );
+
+ this.repository.getImages( 1234 ).fail( function ( actualError
) {
+ assert.deepEqual( error, actualError );
+ } );
+ } );
+
+ QUnit.test( 'it should reject if there\'s an error getting the image',
1, function ( assert ) {
+ var error = 'ERR0R!!!';
+
+ this.getDeferred.reject( error );
+
+ this.repository.getImages( 1234 ).fail( function ( actualError
) {
+ assert.deepEqual( error, actualError );
+ } );
+ } );
+
+ QUnit.test( 'it should return the image if there is one', 1, function (
assert ) {
+ this.getDeferred.resolve( {
+ mobileview: {
+ thumb: {
+ url: 'http://foo/bar/baz',
+ width: 1,
+ height: 1
+ },
+ sections: []
+ }
+ } );
+
+ this.repository.getImages( 1234 ).then( function ( image ) {
+ assert.deepEqual( image, new Image(
'http://foo/bar/baz', 1, 1 ) );
+ } );
+ } );
+
+ QUnit.test( 'it should reject if there isn\'t a thumbnail', 1, function
( assert ) {
+ this.getDeferred.resolve( {
+ mobileview: {
+ sections: []
+ }
+ } );
+
+ this.repository.getImages( 1234 ).fail( function () {
+ assert.strictEqual( true, true );
+ } );
+ } );
+
+ QUnit.test( 'it shouldn\'t try to get the image at the target width
more than once', 1, function ( assert ) {
+ this.getDeferred.resolve( {
+ mobileview: {
+ thumb: {
+ url: 'http://foo/bar/baz',
+ width: 1,
+ height: 1
+ },
+ sections: []
+ }
+ } );
+
+ this.repository.getImages( 1234 );
+ this.repository.getImages( 1234 );
+
+ assert.strictEqual( this.getSpy.callCount, 1 );
+ } );
+
+} ( mw.mobileFrontend, mw, jQuery ) );
diff --git
a/tests/qunit/modules/bannerImage/test_PageImagesBannerImageRepository.js
b/tests/qunit/modules/bannerImage/test_PageImagesBannerImageRepository.js
index 53d7de4..0a81b88 100644
--- a/tests/qunit/modules/bannerImage/test_PageImagesBannerImageRepository.js
+++ b/tests/qunit/modules/bannerImage/test_PageImagesBannerImageRepository.js
@@ -23,7 +23,7 @@
} );
} );
- QUnit.test( 'it should resolve with an empty list if the page doesn\'t
exist', 1, function ( assert ) {
+ QUnit.test( 'it should reject if the page doesn\'t exist', 1, function
( assert ) {
this.getDeferred.resolve( {
query: {
pages: {
@@ -32,12 +32,12 @@
}
} );
- this.repository.getImages( 1234 ).then( function ( images ) {
- assert.deepEqual( images, [] );
+ this.repository.getImages( 1234 ).fail( function () {
+ assert.strictEqual( true, true );
} );
} );
- QUnit.test( 'it should reject if there\'s an error getting the images',
1, function ( assert ) {
+ QUnit.test( 'it should reject if there\'s an error getting the image',
1, function ( assert ) {
var error = 'ERR0R!!!';
this.getDeferred.reject( error );
@@ -47,7 +47,7 @@
} );
} );
- QUnit.test( 'it should return a list of images if there are any', 1,
function ( assert ) {
+ QUnit.test( 'it should return an image if there is one', 1, function (
assert ) {
this.getDeferred.resolve( {
query: {
pages: {
@@ -62,14 +62,12 @@
}
} );
- this.repository.getImages( 1234 ).then( function ( images ) {
- assert.deepEqual( images, [
- new Image( 'http://foo/bar/baz', 1, 1 )
- ] );
+ this.repository.getImages( 1234 ).then( function ( image ) {
+ assert.deepEqual( image, new Image(
'http://foo/bar/baz', 1, 1 ) );
} );
} );
- QUnit.test( 'it shouldn\'t return an image if there isn\'t a
thumbnail', 1, function ( assert ) {
+ QUnit.test( 'it shouldn reject image if there isn\'t a thumbnail', 1,
function ( assert ) {
this.getDeferred.resolve( {
query: {
pages: {
@@ -78,12 +76,12 @@
}
} );
- this.repository.getImages( 1234 ).then( function ( images ) {
- assert.deepEqual( images, [] );
+ this.repository.getImages( 1234 ).fail( function () {
+ assert.strictEqual( true, true );
} );
} );
- QUnit.test( 'it shouldn\'t try to get images at the target width more
than once', 1, function ( assert ) {
+ QUnit.test( 'it shouldn\'t try to get an image at the target width more
than once', 1, function ( assert ) {
this.getDeferred.resolve( {
query: {
pages: {
--
To view, visit https://gerrit.wikimedia.org/r/192788
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0143e4b835311bf45fa3521d1335e23586043297
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/MobileFrontend
Gerrit-Branch: master
Gerrit-Owner: Phuedx <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
