From 9d9fff1871523f17d42bf0ec7e2e0df9aff17acd Mon Sep 17 00:00:00 2001 From: Gilles Dubuc Date: Thu, 27 Feb 2014 01:40:39 +0100 Subject: [PATCH] Bugfixes and improvements for the progress bar - the bar now starts at 5% for a visual indication that something is going on - the bar animates (fast) to 100%, instead of disappearing immediately - the animation logic has been fixed to avoid seeing the bar go backwards - added sanity check in all the callbacks to make sure that we don't apply any changes to an image we are not looking at anymore (including progress updates) Bug: 58055 Change-Id: I765a61c16513e9330a412c5ec96387623ae7dbc7 Mingle: https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/146 Mingle: https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/242 --- resources/mmv/mmv.js | 46 ++++++++-- resources/mmv/mmv.lightboxinterface.js | 15 +-- resources/mmv/ui/mmv.ui.metadataPanel.js | 25 ++++- tests/qunit/mmv/mmv.test.js | 91 ++++++++++++++++++- .../qunit/mmv/ui/mmv.ui.metadataPanel.test.js | 32 +++++-- 5 files changed, 172 insertions(+), 37 deletions(-) diff --git a/resources/mmv/mmv.js b/resources/mmv/mmv.js index 366f1cf5f..3ed4661e4 100755 --- a/resources/mmv/mmv.js +++ b/resources/mmv/mmv.js @@ -245,28 +245,62 @@ start = $.now(); - imagePromise = this.fetchThumbnail( - image.filePageTitle, imageWidths.real - ).progress( function ( thumbnailInfoResponse, imageResponse ) { - if ( viewer.ui && viewer.ui.panel ) { + // Reset the progress bar, it could be at any state if we're calling loadImage + // while another image is already loading + viewer.ui.panel.percent( 0 ); + + imagePromise = this.fetchThumbnail( image.filePageTitle, 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.percent( 5 ); + } + + imagePromise.progress( function ( thumbnailInfoResponse, imageResponse ) { + if ( viewer.lightbox.currentIndex !== image.index ) { + return; + } + + if ( viewer.ui + && viewer.ui.panel + && imageResponse.length === 2 + && imageResponse[ 1 ] > 5 ) { viewer.ui.panel.percent( imageResponse[ 1 ] ); } - } ).done( function ( thumbnail, image ) { - viewer.displayRealThumbnail( thumbnail, image, imageWidths, $.now() - start ); + } ).done( function ( thumbnail, imageElement ) { + if ( viewer.lightbox.currentIndex !== image.index ) { + return; + } + + viewer.displayRealThumbnail( thumbnail, imageElement, imageWidths, $.now() - start ); } ); this.imageInfoProvider.get( image.filePageTitle ).done( function ( imageInfo ) { + if ( viewer.lightbox.currentIndex !== image.index ) { + return; + } + viewer.displayPlaceholderThumbnail( imageInfo, image, $initialImage, imageWidths ); } ); metadataPromise = this.fetchSizeIndependentLightboxInfo( image.filePageTitle ).done( function ( imageInfo, repoInfo, localUsage, globalUsage, userInfo ) { + if ( viewer.lightbox.currentIndex !== image.index ) { + return; + } + viewer.lightbox.iface.panel.setImageInfo(image, imageInfo, repoInfo, localUsage, globalUsage, userInfo ); } ); $.when( imagePromise, metadataPromise ).then( function() { + if ( viewer.lightbox.currentIndex !== image.index ) { + return; + } + viewer.stopListeningToScroll(); viewer.animateMetadataDivOnce() // We need to wait until the animation is finished before we listen to scroll diff --git a/resources/mmv/mmv.lightboxinterface.js b/resources/mmv/mmv.lightboxinterface.js index 794b20bea..cdffe073d 100644 --- a/resources/mmv/mmv.lightboxinterface.js +++ b/resources/mmv/mmv.lightboxinterface.js @@ -54,11 +54,6 @@ addToPost = [], lbinterface = this; - // Staging area for image resizes - this.$staging = $( '
' ) - .addClass( 'mlb-staging-area' ); - $( document.body ).append( this.$staging ); - // SVG filter, needed to achieve blur in Firefox this.$filter = $( '' ); @@ -267,7 +262,7 @@ this.$image = $imageElement; - this.autoResizeImage(); + this.$imageDiv.html( this.$image ); // Capture listener so we can remove it later, otherwise // we are going to leak listeners ! @@ -319,14 +314,6 @@ this.handleEvent( 'mmv-fade-stopped', function ( e ) { ui.fadeStopped( e ); } ); }; - /** - * FIXME refactor and document - */ - LIP.autoResizeImage = function () { - this.$staging.append( this.$image ); - this.$imageDiv.append( this.$image ); - }; - /** * Changes what image is being displayed. * @param {HTMLImageElement} imageEle diff --git a/resources/mmv/ui/mmv.ui.metadataPanel.js b/resources/mmv/ui/mmv.ui.metadataPanel.js index b65e4fed6..b90792032 100644 --- a/resources/mmv/ui/mmv.ui.metadataPanel.js +++ b/resources/mmv/ui/mmv.ui.metadataPanel.js @@ -738,12 +738,27 @@ * @param {number} percent */ MPP.percent = function ( percent ) { - if ( percent < 100 ) { - this.$percent.animate( { width : percent + '%' } ); - this.$progress.removeClass( 'empty' ); - } else { - this.$percent.css( { width : 0 } ); + var panel = this; + + if ( percent === 0 ) { + // When a 0% update comes in, we jump without animation to 0 and we hide the bar this.$progress.addClass( 'empty' ); + this.$percent.stop().css( { width : 0 } ); + } else if ( percent === 100 ) { + // When a 100% update comes in, we make sure that the bar is visible, we animate + // fast to 100 and we hide the bar when the animation is done + this.$progress.removeClass( 'empty' ); + this.$percent.stop().animate( { width : percent + '%' }, 50, 'swing', + function () { + // Reset the position for good measure + panel.$percent.stop().css( { width : 0 } ); + panel.$progress.addClass( 'empty' ); + } ); + } else { + // When any other % update comes in, we make sure the bar is visible + // and we animate to the right position + this.$progress.removeClass( 'empty' ); + this.$percent.stop().animate( { width : percent + '%' } ); } }; diff --git a/tests/qunit/mmv/mmv.test.js b/tests/qunit/mmv/mmv.test.js index 229640660..e837eb3ee 100644 --- a/tests/qunit/mmv/mmv.test.js +++ b/tests/qunit/mmv/mmv.test.js @@ -153,12 +153,13 @@ window.location.hash = ''; } ); - QUnit.test( 'Progress', 1, function ( assert ) { + QUnit.test( 'Progress', 3, function ( assert ) { var imageDeferred = $.Deferred(), viewer = new mw.mmv.MultimediaViewer(), oldImageGet = mw.mmv.provider.Image.prototype.get, oldImageInfoGet = mw.mmv.provider.ImageInfo.prototype.get, - oldThumbnailInfoGet = mw.mmv.provider.ThumbnailInfo.prototype.get; + oldThumbnailInfoGet = mw.mmv.provider.ThumbnailInfo.prototype.get, + i = 0; viewer.thumbs = []; viewer.displayPlaceholderThumbnail = $.noop; @@ -172,8 +173,18 @@ getCurrentImageWidths : function () { return { real : 0 }; }, panel : { setImageInfo : $.noop, percent : function ( percent ) { - assert.strictEqual( percent, 45, - 'Percentage correctly funneled to panel UI' ); + if ( i === 0 ) { + assert.strictEqual( percent, 0, + 'Percentage correctly reset by loadImage' ); + } else if ( i === 1 ) { + assert.strictEqual( percent, 5, + 'Percentage correctly animated to 5 by loadImage' ); + } else { + assert.strictEqual( percent, 45, + 'Percentage correctly funneled to panel UI' ); + } + + i++; } } }, open : $.noop }; @@ -376,4 +387,76 @@ assert.ok( !$image.hasClass( 'blurred' ), 'Placeholder is not blurred' ); assert.ok( !viewer.blurredThumbnailShown, 'Placeholder state is correct' ); } ); + + QUnit.test( 'New image loaded while another one is loading', 1, function ( assert ) { + var imageDeferred, + ligthboxInfoDeferred, + viewer = new mw.mmv.MultimediaViewer(), + oldImageGet = mw.mmv.provider.Image.prototype.get, + oldImageInfoGet = mw.mmv.provider.ImageInfo.prototype.get, + oldThumbnailInfoGet = mw.mmv.provider.ThumbnailInfo.prototype.get, + firstImageDeferred = $.Deferred(), + secondImageDeferred = $.Deferred(), + firstLigthboxInfoDeferred = $.Deferred(), + secondLigthboxInfoDeferred = $.Deferred(); + + viewer.preloadFullscreenThumbnail = $.noop; + viewer.fetchSizeIndependentLightboxInfo = function () { return ligthboxInfoDeferred.promise(); }; + viewer.lightbox = { iface : { + setupForLoad : $.noop, + showImage : $.noop, + getCurrentImageWidths : function () { return { real : 0 }; }, + panel : { setImageInfo : function () { + assert.ok( false, 'Metadata of the first image should not be shown' ); + }, + percent : function ( response, percent ) { + if ( percent === 45 ) { + assert.ok( false, 'Progress of the first image should not be shown' ); + } + } }, + empty: $.noop + }, + open : $.noop }; + viewer.eachPrealoadableLightboxIndex = $.noop; + viewer.animateMetadataDivOnce = function () { + assert.ok( false, 'Metadata of the first image should not be animated' ); + return $.Deferred().reject(); + }; + + mw.mmv.provider.Image.prototype.get = function() { return imageDeferred.promise(); }; + mw.mmv.provider.ImageInfo.prototype.get = function() { return $.Deferred().reject(); }; + mw.mmv.provider.ThumbnailInfo.prototype.get = function() { return $.Deferred().resolve( {} ); }; + + imageDeferred = firstImageDeferred; + ligthboxInfoDeferred = firstLigthboxInfoDeferred; + viewer.loadImage( { filePageTitle : new mw.Title( 'File:Foo.jpg' ), index : 0 }, new Image() ); + + imageDeferred = secondImageDeferred; + ligthboxInfoDeferred = secondLigthboxInfoDeferred; + viewer.loadImage( { filePageTitle : new mw.Title( 'File:Bar.jpg' ), index : 1 }, new Image() ); + + viewer.displayRealThumbnail = function () { + assert.ok( false, 'The first image being done loading should have no effect'); + }; + + firstImageDeferred.notify( undefined, 45 ); + firstImageDeferred.resolve(); + firstLigthboxInfoDeferred.resolve(); + + viewer.lightbox.iface.panel.setImageInfo = $.noop; + viewer.animateMetadataDivOnce = function() { return $.Deferred().reject(); }; + + viewer.displayRealThumbnail = function () { + assert.ok( true, 'The second image being done loading should result in the image being shown'); + }; + + secondImageDeferred.resolve(); + secondLigthboxInfoDeferred.resolve(); + + viewer.close(); + + mw.mmv.provider.Image.prototype.get = oldImageGet; + mw.mmv.provider.ImageInfo.prototype.get = oldImageInfoGet; + mw.mmv.provider.ThumbnailInfo.prototype.get = oldThumbnailInfoGet; + } ); }( mediaWiki, jQuery ) ); diff --git a/tests/qunit/mmv/ui/mmv.ui.metadataPanel.test.js b/tests/qunit/mmv/ui/mmv.ui.metadataPanel.test.js index e8ae8d029..a6e38e9eb 100644 --- a/tests/qunit/mmv/ui/mmv.ui.metadataPanel.test.js +++ b/tests/qunit/mmv/ui/mmv.ui.metadataPanel.test.js @@ -195,28 +195,44 @@ assert.strictEqual( result, date1, 'Invalid date is correctly ignored' ); } ); - QUnit.test( 'Progress bar', 8, function ( assert ) { + QUnit.test( 'Progress bar', 10, function ( assert ) { var $qf = $( '#qunit-fixture' ), panel = new mw.mmv.ui.MetadataPanel( $qf, $( '
' ).appendTo( $qf ) ), oldAnimate = $.fn.animate; - $.fn.animate = function ( target ) { - $( this ).css( target ); - }; - assert.ok( panel.$progress.hasClass( 'empty' ), 'Progress bar is hidden' ); assert.strictEqual( panel.$percent.width(), 0, 'Progress bar\'s indicator is at 0' ); - panel.percent( 0 ); + $.fn.animate = function ( target ) { + $( this ).css( target ); - assert.ok( !panel.$progress.hasClass( 'empty' ), 'Progress bar is visible' ); - assert.strictEqual( panel.$percent.width(), 0, 'Progress bar\'s indicator is at 0' ); + assert.strictEqual( target.width, '50%', 'Animation should go to 50%' ); + }; panel.percent( 50 ); assert.ok( !panel.$progress.hasClass( 'empty' ), 'Progress bar is visible' ); assert.strictEqual( panel.$percent.width(), $qf.width() / 2, 'Progress bar\'s indicator is at half' ); + $.fn.animate = function () { + assert.ok( false, 'Going to 0% should not animate' ); + }; + + panel.percent( 0 ); + + assert.ok( panel.$progress.hasClass( 'empty' ), 'Progress bar is hidden' ); + assert.strictEqual( panel.$percent.width(), 0, 'Progress bar\'s indicator is at 0' ); + + $.fn.animate = function ( target, duration, transition, callback ) { + $( this ).css( target ); + + assert.strictEqual( target.width, '100%', 'Animation should go to 100%' ); + + if ( callback !== undefined ) { + callback(); + } + }; + panel.percent( 100 ); assert.ok( panel.$progress.hasClass( 'empty' ), 'Progress bar is hidden' );