Allow multiple navigation links from info.xml

Signed-off-by: Joas Schilling <coding@schilljs.com>

apps/files/appinfo/info.xml

@@ -55,10 +55,12 @@
 		<command>OCA\Files\Command\ScanAppData</command>
 	</commands>
 
-	<navigation>
-		<name>Files</name>
-		<route>files.view.index</route>
-		<order>0</order>
-	</navigation>
+	<navigations>
+		<navigation>
+			<name>Files</name>
+			<route>files.view.index</route>
+			<order>0</order>
+		</navigation>
+	</navigations>
 
 </info>

apps/files/lib/App.php

@@ -48,7 +48,7 @@ class App {
 				\OC::$server->getGroupManager(),
 				\OC::$server->getConfig()
 			);
-			self::$navigationManager->noDefaultLinks();
+			self::$navigationManager->clear(false);
 		}
 		return self::$navigationManager;
 	}

lib/private/NavigationManager.php

@@ -118,24 +118,13 @@ class NavigationManager implements INavigationManager {
 		});
 	}
 
-	/**
-	 * Do not load the default links
-	 * This is just a hack for the files app
-	 * @internal
-	 */
-	public function noDefaultLinks() {
-		$this->entries = [];
-		$this->closureEntries = [];
-		$this->init = true;
-	}
-
 	/**
 	 * removes all the entries
 	 */
-	public function clear() {
+	public function clear($loadDefaultLinks = true) {
 		$this->entries = [];
 		$this->closureEntries = [];
-		$this->init = false;
+		$this->init = !$loadDefaultLinks;
 	}
 
 	/**
@@ -242,45 +231,46 @@ class NavigationManager implements INavigationManager {
 		foreach ($this->appManager->getInstalledApps() as $app) {
 			// load plugins and collections from info.xml
 			$info = $this->appManager->getAppInfo($app);
-			if (!isset($info['navigation'])) {
-				continue;
-			}
-			$nav = $info['navigation'];
-			if (!isset($nav['name'])) {
-				continue;
-			}
-			if (!isset($nav['route'])) {
-				continue;
-			}
-			$role = isset($nav['@attributes']['role']) ? $nav['@attributes']['role'] : 'all';
-			if ($role === 'admin' && !$this->isAdmin()) {
+			if (empty($info['navigations'])) {
 				continue;
 			}
-			$l = $this->l10nFac->get($app);
-			$order = isset($nav['order']) ? $nav['order'] : 100;
-			$type = isset($nav['type']) ? $nav['type'] : 'link';
-			$route = $this->urlGenerator->linkToRoute($nav['route']);
-			$icon = isset($nav['icon']) ? $nav['icon'] : 'app.svg';
-			foreach ([$icon, "$app.svg"] as $i) {
-				try {
-					$icon = $this->urlGenerator->imagePath($app, $i);
-					break;
-				} catch (\RuntimeException $ex) {
-					// no icon? - ignore it then
+			foreach ($info['navigations'] as $nav) {
+				if (!isset($nav['name'])) {
+					continue;
+				}
+				if (!isset($nav['route'])) {
+					continue;
+				}
+				$role = isset($nav['@attributes']['role']) ? $nav['@attributes']['role'] : 'all';
+				if ($role === 'admin' && !$this->isAdmin()) {
+					continue;
+				}
+				$l = $this->l10nFac->get($app);
+				$order = isset($nav['order']) ? $nav['order'] : 100;
+				$type = isset($nav['type']) ? $nav['type'] : 'link';
+				$route = $this->urlGenerator->linkToRoute($nav['route']);
+				$icon = isset($nav['icon']) ? $nav['icon'] : 'app.svg';
+				foreach ([$icon, "$app.svg"] as $i) {
+					try {
+						$icon = $this->urlGenerator->imagePath($app, $i);
+						break;
+					} catch (\RuntimeException $ex) {
+						// no icon? - ignore it then
+					}
+				}
+				if ($icon === null) {
+					$icon = $this->urlGenerator->imagePath('core', 'default-app-icon');
 				}
-			}
-			if ($icon === null) {
-				$icon = $this->urlGenerator->imagePath('core', 'default-app-icon');
-			}
 
-			$this->add([
-				'id' => $app,
-				'order' => $order,
-				'href' => $route,
-				'icon' => $icon,
-				'type' => $type,
-				'name' => $l->t($nav['name']),
-			]);
+				$this->add([
+					'id' => $app,
+					'order' => $order,
+					'href' => $route,
+					'icon' => $icon,
+					'type' => $type,
+					'name' => $l->t($nav['name']),
+				]);
+			}
 		}
 	}
 

tests/lib/NavigationManagerTest.php

@@ -14,7 +14,7 @@ namespace Test;
 
 use OC\App\AppManager;
 use OC\NavigationManager;
-use OCP\App\IAppManager;
+use OCP\IConfig;
 use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\IURLGenerator;
@@ -23,13 +23,41 @@ use OCP\IUserSession;
 use OCP\L10N\IFactory;
 
 class NavigationManagerTest extends TestCase {
+	/** @var AppManager|\PHPUnit_Framework_MockObject_MockObject */
+	protected $appManager;
+	/** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */
+	protected $urlGenerator;
+	/** @var IFactory|\PHPUnit_Framework_MockObject_MockObject */
+	protected $l10nFac;
+	/** @var IUserSession|\PHPUnit_Framework_MockObject_MockObject */
+	protected $userSession;
+	/** @var IGroupManager|\PHPUnit_Framework_MockObject_MockObject */
+	protected $groupManager;
+	/** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */
+	protected $config;
+
 	/** @var \OC\NavigationManager */
 	protected $navigationManager;
 
 	protected function setUp() {
 		parent::setUp();
 
-		$this->navigationManager = new NavigationManager();
+		$this->appManager = $this->createMock(AppManager::class);
+		$this->urlGenerator = $this->createMock(IURLGenerator::class);
+		$this->l10nFac = $this->createMock(IFactory::class);
+		$this->userSession = $this->createMock(IUserSession::class);
+		$this->groupManager = $this->createMock(IGroupManager::class);
+		$this->config = $this->createMock(IConfig::class);
+		$this->navigationManager = new NavigationManager(
+			$this->appManager,
+			$this->urlGenerator,
+			$this->l10nFac,
+			$this->userSession,
+			$this->groupManager,
+			$this->config
+		);
+
+		$this->navigationManager->clear(false);
 	}
 
 	public function addArrayData() {
@@ -41,6 +69,7 @@ class NavigationManagerTest extends TestCase {
 					'order'	=> 1,
 					'icon'	=> 'optional',
 					'href'	=> 'url',
+					'type'	=> 'settings',
 				],
 				[
 					'id'		=> 'entry id',
@@ -49,6 +78,7 @@ class NavigationManagerTest extends TestCase {
 					'icon'		=> 'optional',
 					'href'		=> 'url',
 					'active'	=> false,
+					'type'		=> 'settings',
 				],
 			],
 			[
@@ -67,6 +97,7 @@ class NavigationManagerTest extends TestCase {
 					'icon'		=> '',
 					'href'		=> 'url',
 					'active'	=> false,
+					'type'		=> 'link',
 				],
 			],
 		];
@@ -79,15 +110,15 @@ class NavigationManagerTest extends TestCase {
 	 * @param array $expectedEntry
 	 */
 	public function testAddArray(array $entry, array $expectedEntry) {
-		$this->assertEmpty($this->navigationManager->getAll(), 'Expected no navigation entry exists');
+		$this->assertEmpty($this->navigationManager->getAll('all'), 'Expected no navigation entry exists');
 		$this->navigationManager->add($entry);
 
-		$navigationEntries = $this->navigationManager->getAll();
-		$this->assertEquals(1, sizeof($navigationEntries), 'Expected that 1 navigation entry exists');
+		$navigationEntries = $this->navigationManager->getAll('all');
+		$this->assertCount(1, $navigationEntries, 'Expected that 1 navigation entry exists');
 		$this->assertEquals($expectedEntry, $navigationEntries[0]);
 
-		$this->navigationManager->clear();
-		$this->assertEmpty($this->navigationManager->getAll(), 'Expected no navigation entry exists after clear()');
+		$this->navigationManager->clear(false);
+		$this->assertEmpty($this->navigationManager->getAll('all'), 'Expected no navigation entry exists after clear()');
 	}
 
 	/**
@@ -109,18 +140,18 @@ class NavigationManagerTest extends TestCase {
 
 		$this->assertEquals(0, $testAddClosureNumberOfCalls, 'Expected that the closure is not called by add()');
 
-		$navigationEntries = $this->navigationManager->getAll();
+		$navigationEntries = $this->navigationManager->getAll('all');
 		$this->assertEquals(1, $testAddClosureNumberOfCalls, 'Expected that the closure is called by getAll()');
-		$this->assertEquals(1, sizeof($navigationEntries), 'Expected that 1 navigation entry exists');
+		$this->assertCount(1, $navigationEntries, 'Expected that 1 navigation entry exists');
 		$this->assertEquals($expectedEntry, $navigationEntries[0]);
 
-		$navigationEntries = $this->navigationManager->getAll();
+		$navigationEntries = $this->navigationManager->getAll('all');
 		$this->assertEquals(1, $testAddClosureNumberOfCalls, 'Expected that the closure is only called once for getAll()');
-		$this->assertEquals(1, sizeof($navigationEntries), 'Expected that 1 navigation entry exists');
+		$this->assertCount(1, $navigationEntries, 'Expected that 1 navigation entry exists');
 		$this->assertEquals($expectedEntry, $navigationEntries[0]);
 
-		$this->navigationManager->clear();
-		$this->assertEmpty($this->navigationManager->getAll(), 'Expected no navigation entry exists after clear()');
+		$this->navigationManager->clear(false);
+		$this->assertEmpty($this->navigationManager->getAll('all'), 'Expected no navigation entry exists after clear()');
 	}
 
 	public function testAddArrayClearGetAll() {
@@ -134,7 +165,7 @@ class NavigationManagerTest extends TestCase {
 
 		$this->assertEmpty($this->navigationManager->getAll(), 'Expected no navigation entry exists');
 		$this->navigationManager->add($entry);
-		$this->navigationManager->clear();
+		$this->navigationManager->clear(false);
 		$this->assertEmpty($this->navigationManager->getAll(), 'Expected no navigation entry exists after clear()');
 	}
 
@@ -160,7 +191,7 @@ class NavigationManagerTest extends TestCase {
 		});
 
 		$this->assertEquals(0, $testAddClosureNumberOfCalls, 'Expected that the closure is not called by add()');
-		$this->navigationManager->clear();
+		$this->navigationManager->clear(false);
 		$this->assertEquals(0, $testAddClosureNumberOfCalls, 'Expected that the closure is not called by clear()');
 		$this->assertEmpty($this->navigationManager->getAll(), 'Expected no navigation entry exists after clear()');
 		$this->assertEquals(0, $testAddClosureNumberOfCalls, 'Expected that the closure is not called by getAll()');
@@ -169,35 +200,29 @@ class NavigationManagerTest extends TestCase {
 	/**
 	 * @dataProvider providesNavigationConfig
 	 */
-	public function testWithAppManager($expected, $config, $isAdmin = false) {
+	public function testWithAppManager($expected, $navigation, $isAdmin = false) {
 
-		$appManager = $this->createMock(AppManager::class);
-		$urlGenerator = $this->createMock(IURLGenerator::class);
-		$l10nFac = $this->createMock(IFactory::class);
-		$userSession = $this->createMock(IUserSession::class);
-		$groupManager = $this->createMock(IGroupManager::class);
 		$l = $this->createMock(IL10N::class);
 		$l->expects($this->any())->method('t')->willReturnCallback(function($text, $parameters = []) {
 			return vsprintf($text, $parameters);
 		});
 
-		$appManager->expects($this->once())->method('getInstalledApps')->willReturn(['test']);
-		$appManager->expects($this->once())->method('getAppInfo')->with('test')->willReturn($config);
-		$l10nFac->expects($this->exactly(count($expected)))->method('get')->with('test')->willReturn($l);
-		$urlGenerator->expects($this->any())->method('imagePath')->willReturnCallback(function($appName, $file) {
+		$this->appManager->expects($this->once())->method('getInstalledApps')->willReturn(['test']);
+		$this->appManager->expects($this->once())->method('getAppInfo')->with('test')->willReturn($navigation);
+		$this->l10nFac->expects($this->exactly(count($expected) + 1))->method('get')->willReturn($l);
+		$this->urlGenerator->expects($this->any())->method('imagePath')->willReturnCallback(function($appName, $file) {
 			return "/apps/$appName/img/$file";
 		});
-		$urlGenerator->expects($this->exactly(count($expected)))->method('linkToRoute')->willReturnCallback(function($route) {
+		$this->urlGenerator->expects($this->exactly(count($expected)))->method('linkToRoute')->willReturnCallback(function() {
 			return "/apps/test/";
 		});
 		$user = $this->createMock(IUser::class);
 		$user->expects($this->any())->method('getUID')->willReturn('user001');
-		$userSession->expects($this->any())->method('getUser')->willReturn($user);
-		$groupManager->expects($this->any())->method('isAdmin')->willReturn($isAdmin);
-
-		$navigationManager = new NavigationManager($appManager, $urlGenerator, $l10nFac, $userSession, $groupManager);
+		$this->userSession->expects($this->any())->method('getUser')->willReturn($user);
+		$this->groupManager->expects($this->any())->method('isAdmin')->willReturn($isAdmin);
 
-		$entries = $navigationManager->getAll();
+		$this->navigationManager->clear();
+		$entries = $this->navigationManager->getAll('all');
 		$this->assertEquals($expected, $entries);
 	}
 
@@ -209,18 +234,29 @@ class NavigationManagerTest extends TestCase {
 				'href' => '/apps/test/',
 				'icon' => '/apps/test/img/app.svg',
 				'name' => 'Test',
-				'active' => false
-			]], ['navigation' => ['route' => 'test.page.index', 'name' => 'Test']]],
+				'active' => false,
+				'type' => 'link',
+			]], ['navigations' => [['route' => 'test.page.index', 'name' => 'Test']]]],
+			'minimalistic-settings' => [[[
+				'id' => 'test',
+				'order' => 100,
+				'href' => '/apps/test/',
+				'icon' => '/apps/test/img/app.svg',
+				'name' => 'Test',
+				'active' => false,
+				'type' => 'settings',
+			]], ['navigations' => [['route' => 'test.page.index', 'name' => 'Test', 'type' => 'settings']]]],
 			'no admin' => [[[
 				'id' => 'test',
 				'order' => 100,
 				'href' => '/apps/test/',
 				'icon' => '/apps/test/img/app.svg',
 				'name' => 'Test',
-				'active' => false
-			]], ['navigation' => ['@attributes' => ['role' => 'admin'], 'route' => 'test.page.index', 'name' => 'Test']], true],
-			'no name' => [[], ['navigation' => ['@attributes' => ['role' => 'admin'], 'route' => 'test.page.index']], true],
-			'admin' => [[], ['navigation' => ['@attributes' => ['role' => 'admin'], 'route' => 'test.page.index', 'name' => 'Test']]]
+				'active' => false,
+				'type' => 'link',
+			]], ['navigations' => [['@attributes' => ['role' => 'admin'], 'route' => 'test.page.index', 'name' => 'Test']]], true],
+			'no name' => [[], ['navigations' => [['@attributes' => ['role' => 'admin'], 'route' => 'test.page.index']]], true],
+			'admin' => [[], ['navigations' => [['@attributes' => ['role' => 'admin'], 'route' => 'test.page.index', 'name' => 'Test']]]]
 		];
 	}
 }