From 404bcd39895c0889f3f5ffbe9b660b2fd761d576 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gerg=C5=91=20Tisza?= <tgr.huwiki@gmail.com>
Date: Tue, 8 Apr 2014 19:36:35 +0000
Subject: [PATCH] Make categories behave

The way categories were handled made it impossible to add anything
after them (they always ended up as the last child of the parent
container). This commit fixes that, and also moves the repo link
behind them (as required by #270).

Change-Id: I7c561c43897054e60028bd524d8ad5ea85f39e36
---
 resources/mmv/ui/mmv.ui.categories.js        | 16 ++--
 resources/mmv/ui/mmv.ui.metadataPanel.js     | 85 +++++++++++---------
 tests/qunit/mmv/ui/mmv.ui.categories.test.js | 36 +++++----
 3 files changed, 75 insertions(+), 62 deletions(-)

diff --git a/resources/mmv/ui/mmv.ui.categories.js b/resources/mmv/ui/mmv.ui.categories.js
index decc87734..c35476057 100755
--- a/resources/mmv/ui/mmv.ui.categories.js
+++ b/resources/mmv/ui/mmv.ui.categories.js
@@ -26,10 +26,12 @@
 	function Categories( $container ) {
 		mw.mmv.ui.Element.call( this, $container );
 
-		this.$categoryTpl = $( '<span>')
+		this.$categoryTpl = $( '<span>').addClass( 'mw-mmv-tag' )
 			.append( $( '<span>').addClass( 'comma-container' )
 				.text( mw.message( 'comma-separator' ) ) )
 			.append( '<a>' );
+
+		this.$categories = $( '<span>' ).appendTo( this.$container );
 	}
 
 	oo.inheritClass( Categories, mw.mmv.ui.Element );
@@ -42,7 +44,7 @@
 	Categories.prototype.set = function ( articlePath, categories ) {
 		var i, cat, href, $category;
 
-		this.empty();
+		this.$categories.empty();
 
 		if ( !categories || !categories.length ) {
 			return;
@@ -57,11 +59,6 @@
 		for ( i = 0; i < categories.length; i++ ) {
 			cat = categories[i];
 
-			if ( !this.$categories ) {
-				this.$categories = $( '<li>' ).addClass( 'mw-mmv-image-category' )
-					.appendTo( this.$container );
-			}
-
 			href = articlePath.replace( '$1', 'Category:' + cat );
 
 			$category = this.$categoryTpl
@@ -83,10 +80,7 @@
 	 * @inheritdoc
 	 */
 	Categories.prototype.empty = function () {
-		if ( this.$categories ) {
-			this.$categories.remove();
-			this.$categories = undefined;
-		}
+		this.$categories.empty();
 	};
 
 	mw.mmv.ui.Categories = Categories;
diff --git a/resources/mmv/ui/mmv.ui.metadataPanel.js b/resources/mmv/ui/mmv.ui.metadataPanel.js
index 2c1714d7e..356c5a676 100644
--- a/resources/mmv/ui/mmv.ui.metadataPanel.js
+++ b/resources/mmv/ui/mmv.ui.metadataPanel.js
@@ -251,13 +251,13 @@
 			.addClass( 'mw-mmv-image-links' )
 			.appendTo( this.$imageLinkDiv );
 
-		this.initializeRepoLink();
 		this.initializeUploader();
 		this.initializeDatetime();
 		this.initializeLocation();
+		this.initializeCategories();
+		this.initializeRepoLink();
 
 		this.fileReuse = new mw.mmv.ui.reuse.Dialog( this.$container, this.buttons.buttons.$reuse );
-		this.categories = new mw.mmv.ui.Categories( this.$imageLinks );
 
 		this.fileUsage = new mw.mmv.ui.FileUsage(
 			$( '<div>' ).appendTo( this.$imageMetadataRight )
@@ -265,41 +265,6 @@
 		this.fileUsage.init();
 	};
 
-	/**
-	 * Initializes the link to the file page on the (maybe remote) repository.
-	 */
-	MPP.initializeRepoLink = function () {
-		this.$repoLi = $( '<li>' )
-			.addClass( 'mw-mmv-repo-li empty' )
-			.appendTo( this.$imageLinks );
-
-		this.$repo = $( '<a>' )
-			.addClass( 'mw-mmv-repo' )
-			.prop( 'href', '#' )
-			.click( function ( e ) {
-				var $link = $( this ),
-					redirect;
-
-				if ( e.altKey || e.shiftKey || e.ctrlKey || e.metaKey || e.button === 1 ) {
-					// They are likely opening the link in a new window or tab
-					mw.mmv.logger.log( 'site-link-click' );
-					return;
-				}
-
-				// If it's a plain click, we need to wait for the logging to
-				// be done before navigating to the desired page
-				e.preventDefault();
-
-				redirect = function () {
-					window.location.href = $link.prop( 'href' );
-				};
-
-				// We want to redirect anyway, whether logging worked or not
-				mw.mmv.logger.log( 'site-link-click' ).then( redirect, redirect );
-			} )
-			.appendTo( this.$repoLi );
-	};
-
 	/**
 	 * Initializes the upload date/time element.
 	 */
@@ -340,6 +305,52 @@
 			.appendTo( this.$locationLi );
 	};
 
+	/**
+	 * Initializes the list of categories of the image
+	 */
+	MPP.initializeCategories = function () {
+		this.categories = new mw.mmv.ui.Categories(
+			$( '<li>' )
+				.addClass( 'mw-mmv-image-category' )
+				.appendTo( this.$imageLinks )
+		);
+	};
+
+	/**
+	 * Initializes the link to the file page on the (maybe remote) repository.
+	 */
+	MPP.initializeRepoLink = function () {
+		this.$repoLi = $( '<li>' )
+			.addClass( 'mw-mmv-repo-li empty' )
+			.appendTo( this.$imageLinks );
+
+		this.$repo = $( '<a>' )
+			.addClass( 'mw-mmv-repo' )
+			.prop( 'href', '#' )
+			.click( function ( e ) {
+				var $link = $( this ),
+					redirect;
+
+				if ( e.altKey || e.shiftKey || e.ctrlKey || e.metaKey || e.button === 1 ) {
+					// They are likely opening the link in a new window or tab
+					mw.mmv.logger.log( 'site-link-click' );
+					return;
+				}
+
+				// If it's a plain click, we need to wait for the logging to
+				// be done before navigating to the desired page
+				e.preventDefault();
+
+				redirect = function () {
+					window.location.href = $link.prop( 'href' );
+				};
+
+				// We want to redirect anyway, whether logging worked or not
+				mw.mmv.logger.log( 'site-link-click' ).then( redirect, redirect );
+			} )
+			.appendTo( this.$repoLi );
+	};
+
 	/**
 	 * Initializes two about links at the bottom of the panel.
 	 */
diff --git a/tests/qunit/mmv/ui/mmv.ui.categories.test.js b/tests/qunit/mmv/ui/mmv.ui.categories.test.js
index 8539ea1fd..49da119e4 100755
--- a/tests/qunit/mmv/ui/mmv.ui.categories.test.js
+++ b/tests/qunit/mmv/ui/mmv.ui.categories.test.js
@@ -2,7 +2,8 @@
 	QUnit.module( 'mmv.ui.categories', QUnit.newMwEnvironment() );
 
 	QUnit.test( 'Sanity test, object creation and UI construction', 2, function ( assert ) {
-		var categories = new mw.mmv.ui.Categories( $( '<ul>' ).appendTo( $( '#qunit-fixture' ) ) );
+		var $list = $( '<li>' ).appendTo( $( '#qunit-fixture' ) ).wrap( '<ul>' ),
+			categories = new mw.mmv.ui.Categories( $list );
 
 		assert.ok( categories, 'Image categories UI element is created' );
 		assert.strictEqual( categories.$categoryTpl.length, 1, 'Image category template is created' );
@@ -11,13 +12,16 @@
 	QUnit.test( 'Setting data in different combinations works well', 17, function ( assert ) {
 		var i,
 			j = 0,
-			categories = new mw.mmv.ui.Categories( $( '<ul>' ).appendTo( $( '#qunit-fixture' ) ) ),
+			$list = $( '<li>' ).appendTo( $( '#qunit-fixture' ) ).wrap( '<ul>' ),
+			categories = new mw.mmv.ui.Categories( $list ),
 			categoryNames = [ 'Foo', undefined, null, 'Bar', 'Baz', '', 'Quux' ],
-			categoryName;
+			categoryName,
+			$category;
 
 		categories.set( 'http://example.net/wiki/$1', categoryNames );
 
-		assert.strictEqual( $( '.mw-mmv-image-category > span' ).length, 4, 'Only valid categories have an element created for them' );
+		assert.strictEqual( categories.$container.find( '.mw-mmv-tag' ).length, 4,
+			'Only valid categories have an element created for them' );
 
 		for ( i = 0; i < categoryNames.length; i ++ ) {
 			categoryName = categoryNames[ i ];
@@ -25,30 +29,34 @@
 			if ( !categoryName || !categoryName.length ) {
 				continue;
 			}
+			$category = categories.$container.find( '.mw-mmv-tag' ).eq( j );
 
-			assert.strictEqual( $( '.mw-mmv-image-category > span' ).eq( j ).find( '.comma-container' ).length, j > 0 ? 1 : 0, 'Comma is properly set inside of them' );
-			assert.strictEqual( $( '.mw-mmv-image-category > span' ).eq( j ).find( 'a' ).text(), categoryName, 'Category elements have correct text inside of them' );
-			assert.strictEqual( $( '.mw-mmv-image-category > span' ).eq( j ).find( 'a' ).prop( 'href' ), 'http://example.net/wiki/Category:' + categoryName, 'Category links are set to the right target' );
+			assert.strictEqual( $category.find( '.comma-container' ).length, j > 0 ? 1 : 0,
+				'Comma is properly set inside of them' );
+			assert.strictEqual( $category.find( 'a' ).text(), categoryName,
+				'Category elements have correct text inside of them' );
+			assert.strictEqual( $category.find( 'a' ).prop( 'href' ),
+				'http://example.net/wiki/Category:' + categoryName,
+				'Category links are set to the right target' );
 
 			j++;
 		}
 
 		for ( i = 0; i < 3; i ++ ) {
-			assert.ok( !$( '.mw-mmv-image-category > span' ).eq( i ).hasClass( 'extra' ), 'Categories before the fourth are not marked as extra' );
+			assert.ok( !categories.$container.find( '.mw-mmv-tag' ).eq( i ).hasClass( 'extra' ),
+				'Categories before the fourth are not marked as extra' );
 		}
 
-		assert.ok( $( '.mw-mmv-image-category > span' ).eq( 3 ).hasClass( 'extra' ), 'Categories after the third are marked as extra' );
+		assert.ok( categories.$container.find( '.mw-mmv-tag' ).eq( 3 ).hasClass( 'extra' ),
+			'Categories after the third are marked as extra' );
 	} );
 
-	QUnit.test( 'Emptying data works as expected', 4, function ( assert ) {
-		var $list = $( '<ul>' ).appendTo( $( '#qunit-fixture' ) ),
+	QUnit.test( 'Emptying data works as expected', 1, function ( assert ) {
+		var $list = $( '<li>' ).appendTo( $( '#qunit-fixture' ) ).wrap( '<ul>' ),
 			categories = new mw.mmv.ui.Categories( $list );
 
 		categories.set( 'http://example.net/wiki/$1', [ 'Foo', 'Bar', 'Baz', 'Quux' ] );
 		categories.empty();
-		assert.strictEqual( $( '.mw-mmv-image-category > span' ).length, 0, 'All elements are removed from the DOM' );
 		assert.strictEqual( $list.text(), '', 'Text is emptied correctly' );
-		assert.strictEqual( $list.find( 'li' ).length, 0, 'List elements are all removed' );
-		assert.strictEqual( categories.$categories, undefined, 'Category UI element is removed from object' );
 	} );
 }( mediaWiki, jQuery ) );