jenkins-bot has submitted this change and it was merged.
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
and remove the PageImagesBannerImageRepository.
Also, both the mobileview API returns 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
D javascripts/modules/bannerImage/PageImagesBannerImageRepository.js
M javascripts/modules/bannerImage/init.js
A tests/qunit/modules/bannerImage/test_MobileViewBannerImageRepository.js
D tests/qunit/modules/bannerImage/test_PageImagesBannerImageRepository.js
7 files changed, 196 insertions(+), 250 deletions(-)
Approvals:
Florianschmidtwelzow: Looks good to me, approved
Jdlrobson: Looks good to me, but someone else must approve
Bmansurov: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/Resources.php b/includes/Resources.php
index 52047ea..1eb7cd4 100644
--- a/includes/Resources.php
+++ b/includes/Resources.php
@@ -1289,7 +1289,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..9e2538d 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
@@ -87,15 +42,12 @@
var self = this,
targetWidth = $( window ).width();
- self.repository.getImages( targetWidth )
- .then( function ( images ) {
- loadOneImage( images )
- .done( function ( image ) {
- self.onImageLoaded(
image );
- } )
- .fail( function () {
- self.remove();
- } );
+ self.repository.getImage( targetWidth )
+ .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..d88d74a
--- /dev/null
+++ b/javascripts/modules/bannerImage/MobileViewBannerImageRepository.js
@@ -0,0 +1,87 @@
+ ( function ( M, $ ) {
+
+ var Class = M.require( 'Class' ),
+ Image = M.require( 'modules/bannerImage/Image' ),
+ MobileViewBannerImageRepository;
+
+ /**
+ * Uses the mobileview 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 an image that can be used as a banner image.
+ *
+ * @param {Number} targetWidth The target width of the image
+ * @return {$.Promise} A promise that resolves with an Image
object if the MediaWiki API
+ * request was successful or rejects otherwise.
+ */
+ getImage: 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
deleted file mode 100644
index 5dbd640..0000000
--- a/javascripts/modules/bannerImage/PageImagesBannerImageRepository.js
+++ /dev/null
@@ -1,86 +0,0 @@
- ( function ( M, $ ) {
-
- var Class = M.require( 'Class' ),
- Image = M.require( 'modules/bannerImage/Image' ),
- PageImagesBannerImageRepository;
-
- /**
- * Uses the PageImages API to get images that can be used as banner
images
- * for a page.
- *
- * @class
- */
- PageImagesBannerImageRepository = 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( {
- prop: 'pageimages',
- titles: self.title,
- pithumbsize: targetWidth
- } )
- .then( function ( data ) {
- var images = [];
-
- // Page doesn't exist.
- if ( data.query.pages.hasOwnProperty( '-1' ) ) {
- deferred.resolve( images );
-
- return;
- }
-
- $.each( data.query.pages, function ( key, value
) {
- var thumbnail = value.thumbnail;
-
- // Page exists but doesn't have
thumbnail.
- if ( !thumbnail ) {
- return;
- }
-
- images.push( new Image(
- thumbnail.source,
- thumbnail.width,
- thumbnail.height
- ) );
- } );
-
- self.cache[targetWidth] = images;
-
- deferred.resolve( images );
- } )
- .fail( deferred.reject );
-
- return promise;
- }
- } );
-
- M.define( 'modules/bannerImage/PageImagesBannerImageRepository',
PageImagesBannerImageRepository );
-
-} ( mw.mobileFrontend, jQuery ) );
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..ccb9f48
--- /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.getImage( 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.getImage( 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.getImage( 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.getImage( 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.getImage( 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.getImage( 1234 );
+ this.repository.getImage( 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
deleted file mode 100644
index 53d7de4..0000000
--- a/tests/qunit/modules/bannerImage/test_PageImagesBannerImageRepository.js
+++ /dev/null
@@ -1,107 +0,0 @@
-( function ( M, mw, $ ) {
-
- var PageImagesBannerImageRepository = M.require(
'modules/bannerImage/PageImagesBannerImageRepository' ),
- Image = M.require( 'modules/bannerImage/Image' );
-
- QUnit.module( 'MobileFrontend
modules/bannerImage/PageImagesBannerImageRepository', {
- 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 PageImagesBannerImageRepository(
api, this.title );
- }
- } );
-
- QUnit.test( 'it should try to get the images at the target width', 1,
function ( assert ) {
- this.repository.getImages( 1234 );
-
- assert.deepEqual( this.getSpy.lastCall.args[0], {
- prop: 'pageimages',
- titles: this.title,
- pithumbsize: 1234
- } );
- } );
-
- QUnit.test( 'it should resolve with an empty list if the page doesn\'t
exist', 1, function ( assert ) {
- this.getDeferred.resolve( {
- query: {
- pages: {
- '-1': {}
- }
- }
- } );
-
- this.repository.getImages( 1234 ).then( function ( images ) {
- assert.deepEqual( images, [] );
- } );
- } );
-
- QUnit.test( 'it should reject if there\'s an error getting the images',
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 a list of images if there are any', 1,
function ( assert ) {
- this.getDeferred.resolve( {
- query: {
- pages: {
- 1234: {
- thumbnail: {
- source:
'http://foo/bar/baz',
- width: 1,
- height: 1
- }
- }
- }
- }
- } );
-
- this.repository.getImages( 1234 ).then( function ( images ) {
- assert.deepEqual( images, [
- 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 ) {
- this.getDeferred.resolve( {
- query: {
- pages: {
- 1234: {}
- }
- }
- } );
-
- this.repository.getImages( 1234 ).then( function ( images ) {
- assert.deepEqual( images, [] );
- } );
- } );
-
- QUnit.test( 'it shouldn\'t try to get images at the target width more
than once', 1, function ( assert ) {
- this.getDeferred.resolve( {
- query: {
- pages: {
- 1234: {
- thumbnail: {
- source:
'http://foo/bar/baz',
- width: 1,
- height: 1
- }
- }
- }
- }
- } );
-
- this.repository.getImages( 1234 );
- this.repository.getImages( 1234 );
-
- assert.strictEqual( this.getSpy.callCount, 1 );
- } );
-
-} ( mw.mobileFrontend, mw, jQuery ) );
--
To view, visit https://gerrit.wikimedia.org/r/192788
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I0143e4b835311bf45fa3521d1335e23586043297
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/extensions/MobileFrontend
Gerrit-Branch: master
Gerrit-Owner: Phuedx <[email protected]>
Gerrit-Reviewer: Bmansurov <[email protected]>
Gerrit-Reviewer: Florianschmidtwelzow <[email protected]>
Gerrit-Reviewer: Jdlrobson <[email protected]>
Gerrit-Reviewer: Phuedx <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
