Merge pull request #41341 from nextcloud/fix/apporder-on-closures

fix: Allow to set app order on navigation entries added by closures

apps/theming/lib/Listener/BeforePreferenceListener.php

@@ -46,8 +46,10 @@ class BeforePreferenceListener implements IEventListener {
 		}
 
 		switch ($event->getAppId()) {
-			case Application::APP_ID: $this->handleThemingValues($event); break;
-			case 'core': $this->handleCoreValues($event); break;
+			case Application::APP_ID: $this->handleThemingValues($event);
+				break;
+			case 'core': $this->handleCoreValues($event);
+				break;
 		}
 	}
 
@@ -78,8 +80,8 @@ class BeforePreferenceListener implements IEventListener {
 
 		$value = json_decode($event->getConfigValue(), true, flags:JSON_THROW_ON_ERROR);
 		if (is_array(($value))) {
-			foreach ($value as $appName => $order) {
-				if (!$this->appManager->isEnabledForUser($appName) || !is_array($order) || empty($order) || !is_numeric($order[key($order)])) {
+			foreach ($value as $id => $info) {
+				if (!is_array($info) || empty($info) || !isset($info['app']) || !$this->appManager->isEnabledForUser($info['app']) || !is_numeric($info['order'] ?? '')) {
 					// Invalid config value, refuse the change
 					return;
 				}

apps/theming/src/components/AppOrderSelector.vue

@@ -37,8 +37,7 @@ export interface IApp {
 	icon: string // path to the icon svg
 	label: string // display name
 	default?: boolean // force app as default app
-	app: string
-	key: number
+	app?: string
 }
 
 export default defineComponent({

apps/theming/src/components/UserAppMenuSection.vue

@@ -58,13 +58,11 @@ interface INavigationEntry {
 	/** Whether this is the default app */
 	default?: boolean
 	/** App that registered this navigation entry (not necessarly the same as the id) */
-	app: string
-	/** The key used to identify this entry in the navigations entries */
-	key: number
+	app?: string
 }
 
 /** The app order user setting */
-type IAppOrder = Record<string, Record<number, number>>
+type IAppOrder = Record<string, { order: number, app?: string }>
 
 /** OCS responses */
 interface IOCSResponse<T> {
@@ -131,8 +129,8 @@ export default defineComponent({
 		 */
 		const updateAppOrder = (value: IApp[]) => {
 			const order: IAppOrder = {}
-			value.forEach(({ app, key }, index) => {
-				order[app] = { ...order[app], [key]: index }
+			value.forEach(({ app, id }, index) => {
+				order[id] = { order: index, app }
 			})
 
 			saveSetting('apporder', order)

lib/private/App/AppManager.php

@@ -838,9 +838,12 @@ class AppManager implements IAppManager {
 				/* Fallback on user defined apporder */
 				$customOrders = json_decode($this->config->getUserValue($user->getUID(), 'core', 'apporder', '[]'), true, flags:JSON_THROW_ON_ERROR);
 				if (!empty($customOrders)) {
-					$customOrders = array_map('min', $customOrders);
-					asort($customOrders);
-					$defaultApps = array_keys($customOrders);
+					// filter only entries with app key (when added using closures or NavigationManager::add the app is not guranteed to be set)
+					$customOrders = array_filter($customOrders, fn ($entry) => isset($entry['app']));
+					// sort apps by order
+					usort($customOrders, fn ($a, $b) => $a['order'] - $b['order']);
+					// set default apps to sorted apps
+					$defaultApps = array_map(fn ($entry) => $entry['app'], $customOrders);
 				}
 			}
 		}

lib/private/NavigationManager.php

@@ -65,6 +65,10 @@ class NavigationManager implements INavigationManager {
 	private $groupManager;
 	/** @var IConfig */
 	private $config;
+	/** The default app for the current user (cached for the `add` function) */
+	private ?string $defaultApp;
+	/** User defined app order (cached for the `add` function) */
+	private array $customAppOrder;
 
 	public function __construct(IAppManager $appManager,
 						 IURLGenerator $urlGenerator,
@@ -78,6 +82,8 @@ class NavigationManager implements INavigationManager {
 		$this->userSession = $userSession;
 		$this->groupManager = $groupManager;
 		$this->config = $config;
+
+		$this->defaultApp = null;
 	}
 
 	/**
@@ -102,6 +108,12 @@ class NavigationManager implements INavigationManager {
 
 		$id = $entry['id'];
 		$entry['unread'] = $this->unreadCounters[$id] ?? 0;
+		if ($entry['type'] === 'link') {
+			// This is the default app that will always be shown first
+			$entry['default'] = ($entry['app'] ?? false) === $this->defaultApp;
+			// Set order from user defined app order
+			$entry['order'] = $this->customAppOrder[$id]['order'] ?? $entry['order'] ?? 100;
+		}
 
 		$this->entries[$id] = $entry;
 	}
@@ -218,6 +230,12 @@ class NavigationManager implements INavigationManager {
 			]);
 		}
 
+		if ($this->appManager === 'null') {
+			return;
+		}
+
+		$this->defaultApp = $this->appManager->getDefaultAppForUser($this->userSession->getUser(), false);
+
 		if ($this->userSession->isLoggedIn()) {
 			// Profile
 			$this->add([
@@ -311,22 +329,15 @@ class NavigationManager implements INavigationManager {
 			}
 		}
 
-		if ($this->appManager === 'null') {
-			return;
-		}
-
 		if ($this->userSession->isLoggedIn()) {
 			$user = $this->userSession->getUser();
 			$apps = $this->appManager->getEnabledAppsForUser($user);
-			$customOrders = json_decode($this->config->getUserValue($user->getUID(), 'core', 'apporder', '[]'), true, flags:JSON_THROW_ON_ERROR);
+			$this->customAppOrder = json_decode($this->config->getUserValue($user->getUID(), 'core', 'apporder', '[]'), true, flags:JSON_THROW_ON_ERROR);
 		} else {
 			$apps = $this->appManager->getInstalledApps();
-			$customOrders = [];
+			$this->customAppOrder = [];
 		}
 
-		// The default app of the current user without fallbacks
-		$defaultApp = $this->appManager->getDefaultAppForUser($this->userSession->getUser(), false);
-
 		foreach ($apps as $app) {
 			if (!$this->userSession->isLoggedIn() && !$this->appManager->isEnabledForUser($app, $this->userSession->getUser())) {
 				continue;
@@ -352,7 +363,7 @@ class NavigationManager implements INavigationManager {
 				}
 				$l = $this->l10nFac->get($app);
 				$id = $nav['id'] ?? $app . ($key === 0 ? '' : $key);
-				$order = $customOrders[$app][$key] ?? $nav['order'] ?? 100;
+				$order = $nav['order'] ?? 100;
 				$type = $nav['type'];
 				$route = !empty($nav['route']) ? $this->urlGenerator->linkToRoute($nav['route']) : '';
 				$icon = $nav['icon'] ?? 'app.svg';
@@ -382,12 +393,8 @@ class NavigationManager implements INavigationManager {
 					// Localized name of the navigation entry
 					'name' => $l->t($nav['name']),
 				], $type === 'link' ? [
-					// This is the default app that will always be shown first
-					'default' => $defaultApp === $id,
 					// App that registered this navigation entry (not necessarly the same as the id)
 					'app' => $app,
-					// The key used to identify this entry in the navigations entries
-					'key' => $key,
 				] : []
 				));
 			}

lib/public/App/IAppManager.php

@@ -250,6 +250,8 @@ interface IAppManager {
 	 *
 	 * @param ?IUser $user User to query default app for
 	 * @param bool $withFallbacks Include fallback values if no default app was configured manually
+	 *                            Before falling back to predefined default apps,
+	 *                            the user defined app order is considered and the first app would be used as the fallback.
 	 *
 	 * @since 25.0.6
 	 * @since 28.0.0 Added optional $withFallbacks parameter

lib/public/INavigationManager.php

@@ -32,6 +32,10 @@
 
 namespace OCP;
 
+/**
+ * @psalm-type NavigationEntry = array{id: string, order: int, href: string, name: string, app?: string, icon?: string, classes?: string, type?: string}
+ */
+
 /**
  * Manages the ownCloud navigation
  * @since 6.0.0
@@ -58,9 +62,11 @@ interface INavigationManager {
 	/**
 	 * Creates a new navigation entry
 	 *
-	 * @param array|\Closure $entry Array containing: id, name, order, icon and href key
+	 * @param array array|\Closure $entry Array containing: id, name, order, icon and href key
+	 * 					If a menu entry (type = 'link') is added, you shall also set app to the app that added the entry.
 	 *					The use of a closure is preferred, because it will avoid
 	 * 					loading the routing of your app, unless required.
+	 * @psalm-param NavigationEntry|callable():NavigationEntry $entry
 	 * @return void
 	 * @since 6.0.0
 	 */

tests/lib/App/AppManagerTest.php

@@ -660,6 +660,17 @@ class AppManagerTest extends TestCase {
 				true,
 				'settings',
 			],
+			// system default app and user apporder
+			[
+				// system default is settings
+				'unexist,settings',
+				'',
+				// apporder says default app is files (order is lower)
+				'{"files_id":{"app":"files","order":1},"settings_id":{"app":"settings","order":2}}',
+				true,
+				// system default should override apporder
+				'settings'
+			],
 			// user-customized defaultapp
 			[
 				'',
@@ -680,7 +691,7 @@ class AppManagerTest extends TestCase {
 			[
 				'unexist,settings',
 				'files',
-				'{"settings":[1],"files":[2]}',
+				'{"settings_id":{"app":"settings","order":1},"files_id":{"app":"files","order":2}}',
 				true,
 				'files',
 			],
@@ -688,18 +699,34 @@ class AppManagerTest extends TestCase {
 			[
 				'',
 				'',
-				'{"settings":[1],"files":[2]}',
+				'{"settings_id":{"app":"settings","order":1},"files":{"app":"files","order":2}}',
 				true,
 				'settings',
 			],
+			// user-customized apporder fallback with missing app key (entries added by closures does not always have an app key set (Nextcloud 27 spreed app for example))
+			[
+				'',
+				'',
+				'{"spreed":{"order":1},"files":{"app":"files","order":2}}',
+				true,
+				'files',
+			],
 			// user-customized apporder, but called without fallback
 			[
 				'',
 				'',
-				'{"settings":[1],"files":[2]}',
+				'{"settings":{"app":"settings","order":1},"files":{"app":"files","order":2}}',
 				false,
 				'',
 			],
+			// user-customized apporder with an app that has multiple routes
+			[
+				'',
+				'',
+				'{"settings_id":{"app":"settings","order":1},"settings_id_2":{"app":"settings","order":3},"id_files":{"app":"files","order":2}}',
+				true,
+				'settings',
+			],
 		];
 	}
 

tests/lib/NavigationManagerTest.php

@@ -365,7 +365,6 @@ class NavigationManagerTest extends TestCase {
 						'unread' => 0,
 						'default' => true,
 						'app' => 'test',
-						'key' => 0,
 					]],
 					['logout' => $defaults['logout']]
 				),
@@ -416,7 +415,6 @@ class NavigationManagerTest extends TestCase {
 						'unread' => 0,
 						'default' => false,
 						'app' => 'test',
-						'key' => 0,
 					],
 						'test1' => [
 							'id' => 'test1',
@@ -430,7 +428,6 @@ class NavigationManagerTest extends TestCase {
 							'unread' => 0,
 							'default' => true, // because of order
 							'app' => 'test',
-							'key' => 1,
 						]],
 					['logout' => $defaults['logout']]
 				),
@@ -458,7 +455,6 @@ class NavigationManagerTest extends TestCase {
 						'unread' => 0,
 						'default' => true,
 						'app' => 'test',
-						'key' => 0,
 					]],
 					['logout' => $defaults['logout']]
 				),
@@ -514,7 +510,6 @@ class NavigationManagerTest extends TestCase {
 				'unread' => 0,
 				'default' => true,
 				'app' => 'test',
-				'key' => 0,
 			],
 		];
 		$navigation = ['navigations' => [
@@ -528,7 +523,7 @@ class NavigationManagerTest extends TestCase {
 				function (string $userId, string $appName, string $key, mixed $default = '') use ($testOrder) {
 					$this->assertEquals('user001', $userId);
 					if ($key === 'apporder') {
-						return json_encode(['test' => [$testOrder]]);
+						return json_encode(['test' => ['app' => 'test', 'order' => $testOrder]]);
 					}
 					return $default;
 				}