Page MenuHomePhabricator

Technical: Separate notifications from personal menu
Closed, ResolvedPublic

Description

The personal menu and notifications are rendered separately in the existing Timeless and Minerva. This will soon be the case in Vector as well (T266536). This creates a need for architectural changes at the skin level.

Acceptance criteria

Changes in core

  • It's proposed that a new menu placeholder is added to core for notification menu items that is modifiable by SkinTemplateNavigation::Universal
  • SkinMustache will read from this menu item and prepend to personal tools for skins so that they are not impacted by this change.
  • SkinMustache will be versioned so that skins using SkinMustache can update themselves safely to include the new menu.

changes in Echo

  • Echo will be updated to use the new menu. This should involve using SkinTemplateNavigation::Universal to add to the new menu. It may require some CSS/HTML ID changes as menu IDs are tied to the menu they belong to

changes in Skin

  • Vector will be updated to read from the new menu
  • Modern (which uses SkinMustache) will be updated to read from the new menu
  • Compatibility code in core will be dropped.

Developer notes

We'd need some kind of versioning for this in SkinMustache to make the transition as smooth as possible. The existence of SkinTemplate skins would have to be handled separately.

diff --git a/includes/skins/SkinMustache.php b/includes/skins/SkinMustache.php
index 1888d21a6c..303112b0bd 100644
--- a/includes/skins/SkinMustache.php
+++ b/includes/skins/SkinMustache.php
@@ -24,11 +24,26 @@ use Wikimedia\WrappedStringList;
  * @since 1.35
  */
 class SkinMustache extends SkinTemplate {
+	/**
+	 * @var integer current skin version
+	 */
+	private $skinVersion = 1;
+
 	/**
 	 * @var TemplateParser|null
 	 */
 	private $templateParser = null;
 
+
+	/**
+	 * @inheritdoc
+	 */
+	public function __construct( $options = null ) {
+		parent::__construct( $options );
+		// used for backwards compatibility skin changes.
+		$this->skinVersion = $options['version'] ?? 1;
+	}
+
 	/**
 	 * @since 1.36
 	 * @stable for overriding
@@ -223,13 +238,27 @@ class SkinMustache extends SkinTemplate {
 				}
 			}
 		}
+
+		$personalMenuData = self::getPersonalToolsForMakeListItem(
+			$this->buildPersonalUrls()
+		);
+
+		// Merge notifications and personal menu for old SkinMustache skins
+		$notifications = $contentNavigation['notifications'] ?? [];
+
+		if ( $this->skinVersion === 1 && count( $notifications ) > 0 ) {
+			// Make backwards compatibility changes for skins using 1.35 SkinMustache spec here.
+
+			$personalMenuData = wfArrayInsertAfter( $personalMenuData, $notifications, 'userpage' );
+			unset( $contentNavigation['notifications'] );
+		}
+
 		foreach ( $contentNavigation as $name => $items ) {
 			$portlets['data-' . $name] = $this->getPortletData( $name, $items );
 		}
+
 		$portlets['data-personal'] = $this->getPortletData( 'personal',
-			self::getPersonalToolsForMakeListItem(
-				$this->buildPersonalUrls()
-			)
+			$personalMenuData
 		);
 		return [
 			'data-portlets' => $portlets,
diff --git a/includes/skins/SkinTemplate.php b/includes/skins/SkinTemplate.php
index 62a0d514d7..b79f43cc8b 100644
--- a/includes/skins/SkinTemplate.php
+++ b/includes/skins/SkinTemplate.php
@@ -359,8 +359,11 @@ class SkinTemplate extends Skin {
 		$tpl->set( 'language_urls', $this->getLanguages() ?: false );
 
 		# Personal toolbar
-		$tpl->set( 'personal_urls', $this->buildPersonalUrls() );
 		$content_navigation = $this->buildContentNavigationUrls();
+		$personalMenuData = $this->buildPersonalUrls();
+		$tpl->set( 'personal_urls', $personalMenuData );
+		// these are included in personal tools.
+		unset( $content_navigation['notifications'] );
 		$content_actions = $this->buildContentActionUrls( $content_navigation );
 		$tpl->set( 'content_navigation', $content_navigation );
 		$tpl->set( 'content_actions', $content_actions );
@@ -434,6 +437,13 @@ class SkinTemplate extends Skin {
 			);
 		}
 
+
+		wfArrayInsertAfter( $personalTools, $notifications, 'userpage' );
+
+		$content_navigation = $this->buildContentNavigationUrls();
+		var_dump( $content_navigation['notifications'] );die;
+		wfArrayInsertAfter( $personalTools, $content_navigation['notifications'], 'userpage' );
+
 		foreach ( $personalTools as $key => $item ) {
 			$html .= $this->makeListItem( $key, $item, $options );
 		}
@@ -820,6 +830,7 @@ class SkinTemplate extends Skin {
 		$permissionManager = MediaWikiServices::getInstance()->getPermissionManager();
 
 		$content_navigation = [
+			'notifications' => [],
 			'namespaces' => [],
 			'views' => [],
 			'actions' => [],

Event Timeline

Restricted Application added subscribers: Masumrezarock100, Aklapper. · View Herald Transcript

I took a stab at this, and ran into the following issues that I'd like to get some input on before I'll upload a patch:

  • Currently, the id's of the echo badges are pt-notifications-alert and pt-notifications-notice. This is expected by skins and by on-wiki stylings. SkinTemplate::buildContentNavigationUrls unconditionally prefixes that with ca-, breaking existing usages of those ids. buildContentNavigationUrls can be changes, but to what? Reserving pt- for URLs in notifications seems wrong.
  • Vector currently calls buildPersonalUrls and getPersonalToolsForMakeListItem directly (so does Mirage, but that's my problem), meaning it misses out on backwards compatibility for placing echo's badges in personal tools. Either buildPersonalUrls is changed to add the notifications from the content navigation (requiring changes to SkinTemplate) or Vector should be pre-adjusted for this.
  • Skin::VERSION_MAJOR does already exist. Adding a separate versioning system to SkinMustache is somewhat confusing.

Skin::VERSION_MAJOR does already exist. Adding a separate versioning system to SkinMustache is somewhat confusing.

Feel free to use or amend the existing versioning. I think the idea here would be to have different behaviour based on the skin version being used. Splitting out the notifications would bump the version number and Echo would need to check if its bumped or not.

urrently, the id's of the echo badges are pt-notifications-alert and pt-notifications-notice. This is expected by skins and by on-wiki stylings. SkinTemplate::buildContentNavigationUrls unconditionally prefixes that with ca-, breaking existing usages of those ids.

I ran into this as well. One suggestion I had was https://gerrit.wikimedia.org/r/c/mediawiki/core/+/648341 - does that solve your issue?

Either buildPersonalUrls is changed to add the notifications from the content navigation (requiring changes to SkinTemplate) or Vector should be pre-adjusted for this.

Changes to SkinTemplate sound necessary, yes.

Skin::VERSION_MAJOR does already exist. Adding a separate versioning system to SkinMustache is somewhat confusing.

Feel free to use or amend the existing versioning. I think the idea here would be to have different behaviour based on the skin version being used. Splitting out the notifications would bump the version number and Echo would need to check if its bumped or not.

Skin::VERSION_MAJOR and it's getter are static, which makes them difficult to initialize from the skin options + they share state between instances. I'm hesitant to allow it to be dynamically adjusted, especially because I'm not sure if it is picked up through late static binding when checking the version through Echo, which doesn't know the class it's dealing with.

urrently, the id's of the echo badges are pt-notifications-alert and pt-notifications-notice. This is expected by skins and by on-wiki stylings. SkinTemplate::buildContentNavigationUrls unconditionally prefixes that with ca-, breaking existing usages of those ids.

I ran into this as well. One suggestion I had was https://gerrit.wikimedia.org/r/c/mediawiki/core/+/648341 - does that solve your issue?

That was a solution I considered. I can use that.

Either buildPersonalUrls is changed to add the notifications from the content navigation (requiring changes to SkinTemplate) or Vector should be pre-adjusted for this.

Changes to SkinTemplate sound necessary, yes.

I'll go with that then.

Change 654651 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/core@master] Skin: Add notifications content navigation

https://gerrit.wikimedia.org/r/654651

Change 654652 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/extensions/Echo@master] Provide notifications icons through content navigation

https://gerrit.wikimedia.org/r/654652

Change 654651 merged by jenkins-bot:
[mediawiki/core@master] Skin: Add notifications and user-menu content navigation

https://gerrit.wikimedia.org/r/654651

Change 654652 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Provide notifications icons through content navigation

https://gerrit.wikimedia.org/r/654652

T266536 takes care of the follow up in Vector.