Echo: A few adjustments based on feedback from Ryan Kaldari and Benny Situ.

Change-Id: If430c02793a1843253ccc18512075befc99a3eb6

Hooks.php

@@ -30,6 +30,8 @@ class EchoHooks {
 	 */
 	public static function getDefaultNotifiedUsers( $event, &$users ) {
 		switch( $event->getType() ) {
+			// Everyone deserves to know when something happens
+			//  on their user talk page
 			case 'edit-user-talk':
 				if ( !$event->getTitle() || !$event->getTitle()->getNamespace() == NS_USER_TALK ) {
 					break;
@@ -37,13 +39,13 @@ class EchoHooks {
 
 				$username = $event->getTitle()->getText();
 				$user = User::newFromName( $username );
-				if ($user->getId()) {
-					$user = User::newFromName($username);
+				if ( $user && $user->getId() ) {
 					$users[$user->getId()] = $user;
 				}
 			break;
 			case 'add-comment':
 			case 'add-talkpage-topic':
+				// Handled by EchoDiscussionParser
 				$extraData = $event->getExtra();
 
 				if ( !isset( $extraData['revid'] ) || !$extraData['revid'] ) {
@@ -212,6 +214,7 @@ class EchoHooks {
 	 */
 	static function onPersonalUrls( &$personal_urls, &$title ) {
 		global $wgUser, $wgLang, $wgOut;
+		// Add a "My notifications" item to personal URLs
 
 		if ( $wgUser->isAnon() ) {
 			return true;
@@ -246,6 +249,8 @@ class EchoHooks {
 	public static function makeGlobalVariablesScript( &$vars, $outputPage ) {
 		$user = $outputPage->getUser();
 
+		// Provide info for the Overlay
+
 		if ( ! $user->isAnon() ) {
 			$vars['wgEchoOverlayConfiguration'] = array(
 				'timestamp' => wfTimestamp( TS_UNIX, wfTimestampNow() ),

api/ApiEchoNotifications.php

@@ -17,7 +17,7 @@ class ApiEchoNotifications extends ApiQueryBase {
 		$prop = $params['prop'];
 
 		if ( in_array('list', $prop) ) {
-			$r = $this->getNotifications( $user, $params['unread'], $params['format'] );
+			$r = $this->getNotifications( $user, $params['unread'], $params['format'], $params['limit'] );
 		} else {
 			$r = array();
 		}
@@ -38,9 +38,10 @@ class ApiEchoNotifications extends ApiQueryBase {
 	 * @param $user User to get notifications for
 	 * @param $unread Boolean: True to get only unread notifications.
 	 * @param $format false to not format any notifications, or an output format name.
+	 * @param $limit The maximum number of notifications to return.
 	 * @return array
 	 */
-	public function getNotifications( $user, $unread = false, $format = false ) {
+	public function getNotifications( $user, $unread = false, $format = false, $limit = 20 ) {
 		$dbr = wfGetDB( DB_SLAVE );
 
 		$output = array();
@@ -60,7 +61,7 @@ class ApiEchoNotifications extends ApiQueryBase {
 			__METHOD__,
 			array(
 				'ORDER BY' => 'notification_timestamp DESC',
-				'LIMIT' => 50,
+				'LIMIT' => $limit,
 			),
 			array(
 				'echo_event' => array('LEFT JOIN', 'notification_event=event_id'),
@@ -145,6 +146,12 @@ class ApiEchoNotifications extends ApiQueryBase {
 						'html',
 					),
 			),
+			'limit' => array(
+				ApiBase::PARAM_TYPE => 'integer',
+				ApiBase::PARAM_DFLT => 20,
+				ApiBase::PARAM_MAX => 50,
+				ApiBase::PARAM_MIN => 1,
+			),
 			'index' => false,
 		);
 	}
@@ -156,6 +163,7 @@ class ApiEchoNotifications extends ApiQueryBase {
 			'unread' => 'Request only unread notifications',
 			'format' => 'If specified, notifications will be returned formatted this way.',
 			'index' => 'If specified, a list of notification IDs, in order, will be returned.',
+			'limit' => 'The maximum number of notifications to return.'
 		);
 	}
 

controller/NotificationController.php

@@ -23,7 +23,8 @@ class EchoNotificationController {
 				'notification_user' => $user->getId(),
 				'notification_read_timestamp' => null,
 			),
-			__METHOD__);
+			__METHOD__
+		);
 
 		$wgMemc->set($memcKey, $count, 86400);
 

includes/DiscussionParser.php

@@ -55,11 +55,11 @@ abstract class EchoDiscussionParser {
 		}
 
 		if ( !$createdEvents && $title->getNamespace() == NS_USER_TALK ) {
-			$user = User::newFromName( $revision->getText() );
-			if ( $user && $user->getID() ) {
+			$notifyUser = User::newFromName( $title->getText() );
+			if ( $notifyUser && $notifyUser->getID() ) {
 				$event = EchoEvent::create( array(
 					'type' => 'edit-user-talk',
-					'title' => $article->getTitle(),
+					'title' => $title,
 					'extra' => array('revid' => $revision->getID()),
 					'agent' => $user,
 				) );
@@ -289,7 +289,7 @@ abstract class EchoDiscussionParser {
 		$text = trim($text);
 
 		$matches = array();
-		preg_match_all( '/'.self::$headerRegex.'/um', $text, &$matches );
+		preg_match_all( '/'.self::$headerRegex.'/um', $text, $matches );
 
 		return count( $matches[0] );
 	}
@@ -305,7 +305,7 @@ abstract class EchoDiscussionParser {
 
 		$matches = array();
 
-		if ( !preg_match( '/'.self::$headerRegex.'/um', $text, &$matches ) ) {
+		if ( !preg_match( '/'.self::$headerRegex.'/um', $text, $matches ) ) {
 			return false;
 		}
 
@@ -352,7 +352,7 @@ abstract class EchoDiscussionParser {
 		// Now if there is only one list item, strip that too
 		$listRegex = '/^\s*(?:[\:#*]\s*)*[#*]/m';
 		$matches = array();
-		if ( preg_match_all( $listRegex, $text, &$matches ) ) {
+		if ( preg_match_all( $listRegex, $text, $matches ) ) {
 			if ( count($matches) == 1 ) {
 				$text = preg_replace( $listRegex, '', $text );
 			}
@@ -707,7 +707,7 @@ abstract class EchoDiscussionParser {
 		if ( ! preg_match(
 			'/^[^\|\]\#]+/u',
 			$userPart,
-			&$userMatches
+			$userMatches
 		) ) {
 			// user link is invalid
 			// print "I\tUser link invalid\t$userPart\n";
@@ -774,7 +774,7 @@ abstract class EchoDiscussionParser {
 		$output = $exemplarTimestamp;
 		$tzRegex = '/\s*\(\w+\)\s*$/';
 		$tzMatches = array();
-		preg_match( $tzRegex, $output, &$tzMatches );
+		preg_match( $tzRegex, $output, $tzMatches );
 		$output = preg_replace( $tzRegex, '', $output );
 		$output = preg_quote( $output, '/' );
 		$output = preg_replace( '/[^\d\W]+/u', '[^\d\W]+', $output );

model/Event.php

@@ -59,7 +59,7 @@ class EchoEvent {
 		$obj = new EchoEvent;
 		static $validFields = array( 'type', 'variant', 'agent', 'title', 'extra' );
 
-		if ( empty($info['type']) ) {
+		if ( empty( $info['type'] ) ) {
 			throw new MWException( "'type' parameter is mandatory" );
 		}
 

model/Notification.php

@@ -1,10 +1,11 @@
 <?php
 
 class EchoNotification {
+	protected $id = false;
 	protected $user = false;
 	protected $event = false;
 	protected $timestamp = false;
-	protected $readTimestamp = false;
+	protected $readTimestamp = null;
 
 	/**
 	 * Do not use this constructor.
@@ -24,9 +25,7 @@ class EchoNotification {
 		$obj = new EchoNotification;
 		static $validFields = array( 'event', 'user' );
 
-		$obj->id = false;
 		$obj->timestamp = wfTimestampNow();
-		$obj->readTimestamp = null;
 
 		foreach( $validFields as $field ) {
 			if ( isset($info[$field]) ) {
@@ -61,7 +60,7 @@ class EchoNotification {
 			'notification_event' => $this->event->getId(),
 			'notification_user' => $this->user->getId(),
 			'notification_timestamp' => $dbw->timestamp( $this->timestamp ),
-			'notification_read_timestamp' => null,
+			'notification_read_timestamp' => $this->readTimestamp,
 		);
 
 		$dbw->insert( 'echo_notification', $row, __METHOD__ );

model/Subscription.php

@@ -14,7 +14,7 @@ class EchoSubscription {
 	 * a user to an event (on a page if applicable).
 	 *
 	 * @param $user User object for the user whose subscription we're talking about.
-	 * @param $event String identifier for the event type of interest
+	 * @param $event String identifier for the event type of interest. Max length is 63 chars.
 	 * @param $title Title|null Optional Title of interest for events, if applicable.
 	 */
 	public function __construct( $user, $event, $title = null) {
@@ -26,7 +26,7 @@ class EchoSubscription {
 			throw new MWException("Invalid Title parameter");
 		}
 
-		if (!$event || is_object($event) || strlen($event) > 63 ) {
+		if ( !$event || !is_string($event) || strlen($event) > 63 ) {
 			throw new MWException("Invalid event parameter");
 		}
 

modules/base/ext.echo.base.css

@@ -1,6 +1,6 @@
 .mw-echo-timestamp {
 	color: gray;
-	font-size: 8px;
+	font-size: 9px;
 }
 
 .mw-echo-unread {

modules/overlay/ext.echo.overlay.css

@@ -9,6 +9,7 @@
 	padding: 15px;
 	padding-left: 0;
 	color: #252525;
+	z-index: 10;
 
 	-moz-border-radius: 5px;
 	-webkit-border-radius: 5px;
@@ -59,3 +60,7 @@
 #pt-notifications .mw-badge {
 	margin-left: 3px;
 }
+
+.mw-echo-overlay-none {
+	margin-left: 20px;
+}

modules/overlay/ext.echo.overlay.js

@@ -1,29 +1,19 @@
 ( function($,mw) {
-	$( function() {
-		mw.echo.overlay = {
-			'updateCount' : function(newCount) {
-				$('#pt-notifications a')
-					.text( mw.msg('echo-link') )
-					.badge( newCount, { 'type' : 'inline' } );
+	mw.echo.overlay = {
+		'updateCount' : function(newCount) {
+			$('#pt-notifications a')
+				.text( mw.msg('echo-link') )
+				.badge( newCount, { 'type' : 'inline' } );
 
-				mw.echo.overlay.notification_count = newCount;
-				
-				console.log('Updated new notification count to '+newCount);
-			},
-			'configuration' : mw.config.get('wgEchoOverlayConfiguration')
-		};
+			mw.echo.overlay.notification_count = newCount;
+		},
 
-		mw.echo.overlay.notification_count = mw.echo.overlay.configuration['notification-count'];
-		mw.echo.overlay.updateCount( mw.echo.overlay.notification_count );
+		'configuration' : mw.config.get('wgEchoOverlayConfiguration'),
 
-		var $link = $('#pt-notifications a');
-		if ( ! $link.length ) {
-			return;
-		}
-
-		function buildOverlay(callback) {
+		'buildOverlay' : function(callback) {
 			var $overlay = $('<div></div>')
 				.addClass('mw-echo-overlay');
+			var $link = $('#pt-notifications a');
 
 			$overlay.append(
 				$('<div/>')
@@ -44,7 +34,7 @@
 					var $ul = $('<ul></ul>').appendTo($overlay);
 
 					$.each( notifications.index, function(index, id) {
-						data = notifications[id];
+						var data = notifications[id];
 						var $li = $('<li></li>')
 							.data('details', data)
 							.data('id', id)
@@ -61,6 +51,7 @@
 						$ul.remove();
 						$overlay.append(
 							$('<div></div>')
+								.addClass( 'mw-echo-overlay-none' )
 								.text(mw.msg('echo-none'))
 						);
 					}
@@ -94,6 +85,17 @@
 				}
 			} );
 		}
+	};
+
+	mw.echo.overlay.notification_count = mw.echo.overlay.configuration['notification-count'];
+
+	$( function() {
+		mw.echo.overlay.updateCount( mw.echo.overlay.notification_count );
+
+		var $link = $('#pt-notifications a');
+		if ( ! $link.length ) {
+			return;
+		}
 
 		$link.click( function(e) {
 			e.preventDefault();
@@ -115,7 +117,7 @@
 				return;
 			}
 			
-			$overlay = buildOverlay(
+			$overlay = mw.echo.overlay.buildOverlay(
 				function($overlay) {
 					$overlay
 						.hide()