jenkins-bot has submitted this change and it was merged. Change subject: Make progress bar less erratic ......................................................................
Make progress bar less erratic Solves the bug, and makes the code slightly cleaner, but it still does not inspire confidence (e.g. use of viewer flags by a bunch of callbacks that can run for a background image). Also, the tests seem underspecified. I'll follow up with some more refactoring. Change-Id: I2557abcec173691ffce21185bf1a939f1644ba8c Mingle: https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/489 --- M resources/mmv/mmv.js M resources/mmv/ui/mmv.ui.canvas.js M tests/qunit/mmv/mmv.test.js 3 files changed, 117 insertions(+), 52 deletions(-) Approvals: Gilles: Looks good to me, approved jenkins-bot: Verified diff --git a/resources/mmv/mmv.js b/resources/mmv/mmv.js index ed64043..8457789 100755 --- a/resources/mmv/mmv.js +++ b/resources/mmv/mmv.js @@ -239,9 +239,7 @@ metadataPromise, start, viewer = this, - $initialImage = $( initialImage ), - fileWidth = image.originalWidth, - fileHeight = image.originalHeight; + $initialImage = $( initialImage ); this.currentIndex = image.index; @@ -273,48 +271,13 @@ start = $.now(); - // Reset the progress bar, it could be at any state if we're calling loadImage - // while another image is already loading - viewer.ui.panel.progressBar.percent( 0 ); - imagePromise = this.fetchThumbnailForLightboxImage( image, imageWidths.real ); - // Check that the image hasn't already been loaded - if ( imagePromise.state() === 'pending' ) { - // Animate it to 5 to give a sense to something is happening, even if we're stuck - // waiting for server-side processing, such as thumbnail (re)generation - viewer.ui.panel.progressBar.percent( 5 ); - } + viewer.displayPlaceholderThumbnail( image, $initialImage, imageWidths ); - if ( fileWidth > 0 && fileHeight > 0 ) { - viewer.displayPlaceholderThumbnail( { width : fileWidth , height : fileHeight }, - $initialImage, - imageWidths ); - } else { - this.imageInfoProvider.get( image.filePageTitle ).done( function ( imageInfo ) { - if ( viewer.currentIndex !== image.index ) { - return; - } + this.setupProgressBar( image, imagePromise ); - viewer.displayPlaceholderThumbnail( imageInfo, $initialImage, imageWidths ); - } ); - } - - imagePromise.progress( function ( thumbnailInfoResponse, imageResponse ) { - if ( viewer.currentIndex !== image.index ) { - return; - } - - if ( viewer.ui - && viewer.ui.panel - && imageResponse.length === 2 - && imageResponse[ 1 ] > 5 ) { - viewer.ui.panel.progressBar.percent( imageResponse[ 1 ] ); - } - } ).done( function ( thumbnail, imageElement ) { - // Fallback in case the browser doesn't have fancy progress updates - viewer.ui.panel.progressBar.percent( 100 ); - + imagePromise.done( function ( thumbnail, imageElement ) { if ( viewer.currentIndex !== image.index ) { return; } @@ -406,18 +369,97 @@ /** * Display the blurred thumbnail from the page - * @param {mw.mmv.model.Image} imageInfo + * @param {mw.mmv.LightboxImage} image * @param {jQuery} $initialImage The thumbnail from the page * @param {mw.mmv.model.ThumbnailWidth} imageWidths + * @param {boolean} [recursion=false] for internal use, never set this */ - MMVP.displayPlaceholderThumbnail = function ( imageInfo, $initialImage, imageWidths ) { + MMVP.displayPlaceholderThumbnail = function ( image, $initialImage, imageWidths, recursion ) { + var viewer = this, + size = { width : image.originalWidth, height : image.originalHeight }; + // If the actual image has already been displayed, there's no point showing the blurry one if ( this.realThumbnailShown ) { return; } - this.blurredThumbnailShown = this.ui.canvas.maybeDisplayPlaceholder( - imageInfo, $initialImage, imageWidths ); + // Width/height of the original image are added to the HTML by MediaViewer via a PHP hook, + // and can be missing in exotic circumstances, e. g. when the extension has only been + // enabled recently and the HTML cache has not cleared yet. If that is the case, we need + // to fetch the size from the API first. + if ( !size.width || !size.height ) { + if ( recursion ) { + // this should not be possible, but an infinite recursion is nasty + // business, so we make a sanity check + throw 'MediaViewer internal error: displayPlaceholderThumbnail recursion'; + } + this.imageInfoProvider.get( image.filePageTitle ).done( function ( imageInfo ) { + // Make sure the user has not navigated away while we were waiting for the size + if ( viewer.currentIndex === image.index ) { + image.originalWidth = imageInfo.width; + image.originalHeight = imageInfo.height; + viewer.displayPlaceholderThumbnail( image, $initialImage, imageWidths, true ); + } + } ); + } else { + this.blurredThumbnailShown = this.ui.canvas.maybeDisplayPlaceholder( + size, $initialImage, imageWidths ); + } + }; + + /** + * Displays a progress bar for the image loading, if necessary, and sets up handling of + * all the related callbacks. + * FIXME would be nice to pass a simple promise which only returns a single number + * and does not fire when the image is not visible + * @param {mw.mmv.LightboxImage} image + * @param {jQuery.Promise.<mw.mmv.model.Thumbnail, HTMLImageElement>} imagePromise + */ + MMVP.setupProgressBar = function ( image, imagePromise ) { + var viewer = this; + + // Reset the progress bar, it could be at any state if we're calling loadImage + // while another image is already loading + // FIXME we should probably jump to the current progress instead + viewer.ui.panel.progressBar.percent( 0 ); + + if ( imagePromise.state() !== 'pending' ) { + // image has already loaded (or failed to load) - nothing to do + return; + } + + // FIXME this is all wrong, we might be navigating back to a half-loaded image + + // Animate progress bar to 5 to give a sense to something is happening, even if we're + // stuck waiting for server-side processing, such as thumbnail (re)generation + viewer.ui.panel.progressBar.percent( 5 ); + + imagePromise.progress( function ( thumbnailInfoResponse, imageResponse ) { + // FIXME this should be explained in a comment + var progress = imageResponse[1]; + + if ( viewer.currentIndex !== image.index ) { + return; + } + + // We started from 5, don't move backwards + if ( progress > 5 ) { + viewer.ui.panel.progressBar.percent( progress ); + } + } ).done( function () { + if ( viewer.currentIndex !== image.index ) { + return; + } + + // Fallback in case the browser doesn't have fancy progress updates + viewer.ui.panel.progressBar.percent( 100 ); + } ).fail( function () { + if ( viewer.currentIndex !== image.index ) { + return; + } + // Hide progress bar on error + viewer.ui.panel.progressBar.percent( 0 ); + } ); }; /** @@ -592,6 +634,7 @@ * Loads size-dependent components of a lightbox - the thumbnail model and the image itself. * @param {mw.mmv.LightboxImage} image * @param {number} width the width of the requested thumbnail + * @returns {jQuery.Promise.<mw.mmv.model.Thumbnail, HTMLImageElement>} */ MMVP.fetchThumbnailForLightboxImage = function ( image, width ) { return this.fetchThumbnail( diff --git a/resources/mmv/ui/mmv.ui.canvas.js b/resources/mmv/ui/mmv.ui.canvas.js index e08f5ec..c691b8d 100644 --- a/resources/mmv/ui/mmv.ui.canvas.js +++ b/resources/mmv/ui/mmv.ui.canvas.js @@ -177,23 +177,23 @@ * We set SVG files to the maximum screen size available. * Assumes set function called before. * - * @param {mw.mmv.model.Image} imageInfo + * @param {{width: number, height: number}} size * @param {jQuery} $imagePlaceholder Image placeholder to be displayed while the real image loads. * @param {mw.mmv.model.ThumbnailWidth} imageWidths * @returns {boolean} Whether the image was blured or not */ - C.maybeDisplayPlaceholder = function ( imageInfo, $imagePlaceholder, imageWidths ) { + C.maybeDisplayPlaceholder = function ( size, $imagePlaceholder, imageWidths ) { var targetWidth, targetHeight, blowupFactor, blurredThumbnailShown = false; // Assume natural thumbnail size¸ - targetWidth = imageInfo.width; - targetHeight = imageInfo.height; + targetWidth = size.width; + targetHeight = size.height; // If the image is bigger than the screen we need to resize it - if ( imageInfo.width > imageWidths.cssWidth ) { // This assumes imageInfo.width in CSS units + if ( size.width > imageWidths.cssWidth ) { // This assumes imageInfo.width in CSS units targetWidth = imageWidths.cssWidth; targetHeight = imageWidths.cssHeight; } diff --git a/tests/qunit/mmv/mmv.test.js b/tests/qunit/mmv/mmv.test.js index 57e9f1a..d211bb8 100644 --- a/tests/qunit/mmv/mmv.test.js +++ b/tests/qunit/mmv/mmv.test.js @@ -174,8 +174,30 @@ viewer.ui = { canvas : { maybeDisplayPlaceholder : function() { return true; } } }; + viewer.imageInfoProvider.get = this.sandbox.stub(); - viewer.displayPlaceholderThumbnail( undefined, undefined, undefined); + viewer.displayPlaceholderThumbnail( { originalWidth: 100, originalHeight: 100 }, undefined, undefined); + + assert.ok( viewer.blurredThumbnailShown, 'Placeholder state is correct' ); + assert.ok( !viewer.realThumbnailShown, 'Real thumbnail state is correct' ); + + viewer.displayRealThumbnail(); + + assert.ok( viewer.realThumbnailShown, 'Real thumbnail state is correct' ); + assert.ok( viewer.blurredThumbnailShown, 'Placeholder state is correct' ); + } ); + + QUnit.test( 'Placeholder first, then real thumbnail - missing size', 4, function ( assert ) { + var viewer = new mw.mmv.MultimediaViewer(); + + viewer.currentIndex = 1; + viewer.setImage = $.noop; + viewer.ui = { canvas : { + maybeDisplayPlaceholder : function() { return true; } + } }; + viewer.imageInfoProvider.get = this.sandbox.stub().returns( $.Deferred().resolve( {width: 100, height: 100 } ) ); + + viewer.displayPlaceholderThumbnail( { index: 1 }, undefined, undefined); assert.ok( viewer.blurredThumbnailShown, 'Placeholder state is correct' ); assert.ok( !viewer.realThumbnailShown, 'Real thumbnail state is correct' ); @@ -199,7 +221,7 @@ assert.ok( viewer.realThumbnailShown, 'Real thumbnail state is correct' ); assert.ok( !viewer.blurredThumbnailShown, 'Placeholder state is correct' ); - viewer.displayPlaceholderThumbnail( undefined, undefined, undefined); + viewer.displayPlaceholderThumbnail( {}, undefined, undefined); assert.ok( viewer.realThumbnailShown, 'Real thumbnail state is correct' ); assert.ok( !viewer.blurredThumbnailShown, 'Placeholder state is correct' ); -- To view, visit https://gerrit.wikimedia.org/r/129564 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I2557abcec173691ffce21185bf1a939f1644ba8c Gerrit-PatchSet: 4 Gerrit-Project: mediawiki/extensions/MultimediaViewer Gerrit-Branch: master Gerrit-Owner: Gergő Tisza <gti...@wikimedia.org> Gerrit-Reviewer: Gilles <gdu...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
