Gergő Tisza has uploaded a new change for review. https://gerrit.wikimedia.org/r/125932
Change subject: Replace current URL generation logic with routing classes ...................................................................... Replace current URL generation logic with routing classes * deduplicates URL generating/parsing code * gets rid of spaces in URLs * fixes error for file names with / in them (in case they exist; current MediaWiki seems to disallow such names anyway) Change-Id: I5aad43f6af1b99523c597c39befcc9db1ecab83a Mingle: https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/371 --- M MultimediaViewer.php M MultimediaViewerHooks.php M resources/mmv/mmv.EmbedFileFormatter.js M resources/mmv/mmv.bootstrap.js M resources/mmv/mmv.js M resources/mmv/routing/mmv.routing.Router.js M resources/mmv/ui/mmv.ui.reuse.share.js M tests/qunit/mmv/mmv.bootstrap.test.js M tests/qunit/mmv/mmv.test.js M tests/qunit/mmv/routing/mmv.routing.Router.test.js M tests/qunit/mmv/ui/mmv.ui.reuse.share.test.js 11 files changed, 65 insertions(+), 35 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/MultimediaViewer refs/changes/32/125932/1 diff --git a/MultimediaViewer.php b/MultimediaViewer.php index 0726e6e..84944e6 100644 --- a/MultimediaViewer.php +++ b/MultimediaViewer.php @@ -471,6 +471,7 @@ 'dependencies' => array( 'mmv.base', + 'mmv.routing', 'oojs', 'mmv.HtmlUtils', ), @@ -686,6 +687,7 @@ 'mmv.model.TaskQueue', 'mmv.lightboxinterface', 'mmv.provider', + 'mmv.routing', 'jquery.fullscreen', 'jquery.hidpi', 'jquery.scrollTo', diff --git a/MultimediaViewerHooks.php b/MultimediaViewerHooks.php index afa8676..52ccf29 100644 --- a/MultimediaViewerHooks.php +++ b/MultimediaViewerHooks.php @@ -226,7 +226,6 @@ 'mmv.ui.reuse.share', 'mmv.ui.reuse.embed', 'mmv.ui.reuse.download', - 'mmv.routing', ), 'localBasePath' => __DIR__, 'remoteExtPath' => 'MultimediaViewer', diff --git a/resources/mmv/mmv.EmbedFileFormatter.js b/resources/mmv/mmv.EmbedFileFormatter.js index 93766e3..d449f41 100644 --- a/resources/mmv/mmv.EmbedFileFormatter.js +++ b/resources/mmv/mmv.EmbedFileFormatter.js @@ -26,6 +26,13 @@ function EmbedFileFormatter() { /** @property {mw.mmv.HtmlUtils} htmlUtils - */ this.htmlUtils = new mw.mmv.HtmlUtils(); + + /** + * FIXME should use the same instance as the main app; we need a dependency + * injection framework for that + * @property {mediaWiki.mmv.routing.Router} + */ + this.router = new mw.mmv.routing.Router(); } EFFP = EmbedFileFormatter.prototype; @@ -167,13 +174,12 @@ }; /** - * Generare a link which we will be using for sharing stuff. - * FIXME this should be handled by mmv.js to be DRY - * + * Generate a link which we will be using for sharing stuff. * @param {mw.mmv.model.EmbedFileInfo} info */ EFFP.getLinkUrl = function ( info ) { - return info.imageInfo.descriptionUrl + '#mediaviewer/' + info.imageInfo.title.getMainText(); + var route = new mw.mmv.routing.ThumbnailRoute( info.imageInfo.title ); + return this.router.createHashForUrl( route, info.imageInfo.descriptionUrl ); }; mw.mmv.EmbedFileFormatter = EmbedFileFormatter; diff --git a/resources/mmv/mmv.bootstrap.js b/resources/mmv/mmv.bootstrap.js index 162843e..40cc3c0 100755 --- a/resources/mmv/mmv.bootstrap.js +++ b/resources/mmv/mmv.bootstrap.js @@ -227,7 +227,7 @@ } this.loadViewer().then( function ( viewer ) { - viewer.loadImageByTitle( title.getPrefixedText(), true ); + viewer.loadImageByTitle( title, true ); } ); e.preventDefault(); diff --git a/resources/mmv/mmv.js b/resources/mmv/mmv.js index cb56815..ad82a59 100755 --- a/resources/mmv/mmv.js +++ b/resources/mmv/mmv.js @@ -80,9 +80,14 @@ /** * Image index on page. - * @type {number} + * @property {number} */ this.currentIndex = 0; + + /** + * @property {mediaWiki.mmv.routing.Router} + */ + this.router = new mw.mmv.routing.Router(); /** * UI object used to display the pictures in the page. @@ -318,7 +323,7 @@ /** * Loads an image by its title - * @param {string} title + * @param {mw.Title} title * @param {boolean} updateHash Viewer should update the location hash when true */ MMVP.loadImageByTitle = function ( title, updateHash ) { @@ -331,7 +336,7 @@ this.comingFromHashChange = !updateHash; $.each( this.thumbs, function ( idx, thumb ) { - if ( thumb.title.getPrefixedText() === title ) { + if ( thumb.title.getPrefixedText() === title.getPrefixedText() ) { viewer.loadImage( thumb.image, thumb.$thumb.clone()[ 0 ], true ); return false; } @@ -629,11 +634,15 @@ * Handles a hash change coming from the browser */ MMVP.hash = function () { - var hash = decodeURIComponent( window.location.hash ), - linkState = hash.split( '/' ); + // Firefox percent-decodes location.hash: https://bugzilla.mozilla.org/show_bug.cgi?id=483304 + // which would cause inconsistent behavior for files which have % or / characters in their names. + // Using location.href is safe. + var hash = window.location.href.split( '#' )[1] || '', + route = this.router.parseHash( hash ); - if ( linkState[0] === '#mediaviewer' ) { - this.loadImageByTitle( linkState[ 1 ] ); + if ( route instanceof mw.mmv.routing.ThumbnailRoute ) { + // FIXME handle image position + this.loadImageByTitle( route.fileTitle ); } else if ( this.isOpen ) { // This allows us to avoid the mmv.hash event that normally happens on close comingFromHashChange = true; @@ -648,8 +657,11 @@ }; MMVP.setHash = function() { + var route, hashFragment; if ( !this.comingFromHashChange ) { - var hashFragment = '#mediaviewer/' + this.currentImageFilename; + // FIXME add image position + route = new mw.mmv.routing.ThumbnailRoute( this.currentImageFileTitle ); + hashFragment = '#' + this.router.createHash( route ); $( document ).trigger( $.Event( 'mmv.hash', { hash : hashFragment } ) ); } }; diff --git a/resources/mmv/routing/mmv.routing.Router.js b/resources/mmv/routing/mmv.routing.Router.js index c60118f..69c416c 100644 --- a/resources/mmv/routing/mmv.routing.Router.js +++ b/resources/mmv/routing/mmv.routing.Router.js @@ -21,11 +21,8 @@ /** * Converts between routes and their URL hash representations such as `mediaviewer/File:Foo`. * @constructor - * @param location */ - function Router( location ) { - this.location = location; - } + function Router() {} RP = Router.prototype; RP.applicationPrefix = 'mediaviewer'; @@ -34,7 +31,9 @@ * Takes an URL hash and returns a route (or null if it could not be parsed). * Returns null for URL hashes which were not created by MediaViewer; you should use * #isMediaViewerHash() if you want to differentiate such hashes. - * The hash can contain the starting `#` but does not have to. + * The hash can contain the starting `#` but does not have to; it should be in raw (percent- + * encoded) form. Note that the percent-encoding behavior of location.hash is not consistent + * between browsers; location.href can be used instead. * @param {string} hash * @return {mw.mmv.routing.Route|null} */ @@ -63,7 +62,8 @@ /** * Takes a route and returns a string representation which can be used in the URL fragment. - * The string does not contain the starting `#`. + * The string does not contain the starting `#`, and it is encoded and guaranteed to be a + * valid URL. * @param {mw.mmv.routing.Route} route * @return {string} */ @@ -94,17 +94,6 @@ */ RP.createHashForUrl = function ( route, url ) { return url.replace( /#.*/, '' ) + '#' + this.createHash( route ); - }; - - /** - * Like #createHash(), but appends the hash to the current URL - * @param {mw.mmv.routing.Route} route - * @param {boolean} [absolute] create an absolute URL (defaults to false) - * @return {string} - */ - RP.createHashForCurrentPage = function ( route, absolute ) { - var currentUrl = absolute ? location.href : location.pathname + location.search; - return this.createHashForUrl( route, currentUrl ); }; /** diff --git a/resources/mmv/ui/mmv.ui.reuse.share.js b/resources/mmv/ui/mmv.ui.reuse.share.js index 5e8e8b3..859e194 100644 --- a/resources/mmv/ui/mmv.ui.reuse.share.js +++ b/resources/mmv/ui/mmv.ui.reuse.share.js @@ -28,6 +28,13 @@ function Share( $container ) { Share['super'].call( this, $container ); + /** + * FIXME should use the same instance as the main app; we need a dependency + * injection framework for that + * @property {mediaWiki.mmv.routing.Router} + */ + this.router = new mw.mmv.routing.Router(); + this.init(); } oo.inheritClass( Share, mw.mmv.ui.reuse.Tab ); @@ -75,8 +82,9 @@ * @param {mw.mmv.model.Image} image */ SP.set = function ( image ) { - // FIXME this should be handled by mmv.js to be DRY - var url = image.descriptionUrl + '#mediaviewer/' + image.title.getMainText(); + var route = new mw.mmv.routing.ThumbnailRoute( image.title ), + url = this.router.createHashForUrl( route, image.descriptionUrl ); + this.pageInput.setValue( url ); this.select(); diff --git a/tests/qunit/mmv/mmv.bootstrap.test.js b/tests/qunit/mmv/mmv.bootstrap.test.js index f13930e..817d474 100644 --- a/tests/qunit/mmv/mmv.bootstrap.test.js +++ b/tests/qunit/mmv/mmv.bootstrap.test.js @@ -183,7 +183,7 @@ $link = $div.find( 'a.image' ); viewer.loadImageByTitle = function ( loadedTitle ) { - assert.strictEqual( loadedTitle, 'File:Foo.jpg', 'Titles are identical' ); + assert.strictEqual( loadedTitle.getPrefixedDb(), 'File:Foo.jpg', 'Titles are identical' ); }; // Create a new bootstrap object to trigger the DOM scan, etc. diff --git a/tests/qunit/mmv/mmv.test.js b/tests/qunit/mmv/mmv.test.js index aef1ac6..276873e 100644 --- a/tests/qunit/mmv/mmv.test.js +++ b/tests/qunit/mmv/mmv.test.js @@ -78,7 +78,7 @@ $( '#qunit-fixture' ).append( '<a class="image"><img src="' + imageSrc + '"></a>' ); viewer.loadImageByTitle = function( title ) { - assert.strictEqual( title, 'File:' + imageSrc, 'The title matches' ); + assert.strictEqual( title.getPrefixedText(), 'File:' + imageSrc, 'The title matches' ); }; // Open a valid mmv hash link and check that the right image is requested. diff --git a/tests/qunit/mmv/routing/mmv.routing.Router.test.js b/tests/qunit/mmv/routing/mmv.routing.Router.test.js index e7e60e0..a98fa75 100644 --- a/tests/qunit/mmv/routing/mmv.routing.Router.test.js +++ b/tests/qunit/mmv/routing/mmv.routing.Router.test.js @@ -147,4 +147,16 @@ assert.strictEqual( route.fileTitle.getPrefixedDb(), 'File:Foo_bar.png', 'Old urls (with space) are handled' ); } ); + + QUnit.test( 'createHashForUrl()', 2, function ( assert ) { + var url, + route = new mw.mmv.routing.MainFileRoute(), + router = new mw.mmv.routing.Router(); + + url = router.createHashForUrl( route, 'http://example.com/' ); + assert.strictEqual( url, 'http://example.com/#mediaviewer', 'Url generation works' ); + + url = router.createHashForUrl( route, 'http://example.com/#foo' ); + assert.strictEqual( url, 'http://example.com/#mediaviewer', 'Urls with fragments are handled' ); + } ); }( mediaWiki ) ); diff --git a/tests/qunit/mmv/ui/mmv.ui.reuse.share.test.js b/tests/qunit/mmv/ui/mmv.ui.reuse.share.test.js index 181291a..9558a29 100644 --- a/tests/qunit/mmv/ui/mmv.ui.reuse.share.test.js +++ b/tests/qunit/mmv/ui/mmv.ui.reuse.share.test.js @@ -36,6 +36,7 @@ image = { // fake mw.mmv.model.Image title: new mw.Title( 'File:Foobar.jpg' ), url: 'https://upload.wikimedia.org/wikipedia/commons/3/3a/Foobar.jpg', + descriptionUrl: '//commons.wikimedia.org/wiki/File:Foobar.jpg' }; assert.notStrictEqual( ! share.pageInput.getValue(), '', 'pageInput is empty.' ); @@ -58,6 +59,7 @@ image = { title: new mw.Title( 'File:Foobar.jpg' ), url: 'https://upload.wikimedia.org/wikipedia/commons/3/3a/Foobar.jpg', + descriptionUrl: '//commons.wikimedia.org/wiki/File:Foobar.jpg' }; share.set( image ); -- To view, visit https://gerrit.wikimedia.org/r/125932 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5aad43f6af1b99523c597c39befcc9db1ecab83a Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/MultimediaViewer Gerrit-Branch: master Gerrit-Owner: Gergő Tisza <[email protected]> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
