feat(AppManager): Provide getAppIcon function

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

apps/updatenotification/lib/Notification/AppUpdateNotifier.php

@@ -84,16 +84,7 @@ class AppUpdateNotifier implements INotifier {
 		// Prepare translation factory for requested language
 		$l = $this->l10nFactory->get(Application::APP_NAME, $languageCode);
 		
-		// See if we can find the app icon - if not fall back to default icon
-		$possibleIcons = [$appId . '-dark.svg', 'app-dark.svg', $appId . '.svg', 'app.svg'];
-		$icon = null;
-		foreach ($possibleIcons as $iconName) {
-			try {
-				$icon = $this->urlGenerator->imagePath($appId, $iconName);
-			} catch (\RuntimeException $e) {
-				// ignore
-			}
-		}
+		$icon = $this->appManager->getAppIcon($appId);
 		if ($icon === null) {
 			$icon = $this->urlGenerator->imagePath('core', 'default-app-icon');
 		}

lib/private/App/AppManager.php

@@ -56,6 +56,7 @@ use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\IGroup;
 use OCP\IGroupManager;
+use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Settings\IManager as ISettingsManager;
@@ -104,9 +105,24 @@ class AppManager implements IAppManager {
 		private ICacheFactory $memCacheFactory,
 		private IEventDispatcher $dispatcher,
 		private LoggerInterface $logger,
+		private IURLGenerator $urlGenerator,
 	) {
 	}
 
+	public function getAppIcon(string $appId): ?string {
+		$possibleIcons = [$appId . '.svg', 'app.svg', $appId . '-dark.svg', 'app-dark.svg'];
+		$icon = null;
+		foreach ($possibleIcons as $iconName) {
+			try {
+				$icon = $this->urlGenerator->imagePath($appId, $iconName);
+				break;
+			} catch (\RuntimeException $e) {
+				// ignore
+			}
+		}
+		return $icon;
+	}
+
 	/**
 	 * @return string[] $appId => $enabled
 	 */

lib/private/NavigationManager.php

@@ -372,15 +372,17 @@ class NavigationManager implements INavigationManager {
 				$order = $nav['order'] ?? 100;
 				$type = $nav['type'];
 				$route = !empty($nav['route']) ? $this->urlGenerator->linkToRoute($nav['route']) : '';
-				$icon = $nav['icon'] ?? 'app.svg';
-				foreach ([$icon, "$app.svg"] as $i) {
+				$icon = $nav['icon'] ?? null;
+				if ($icon !== null) {
 					try {
-						$icon = $this->urlGenerator->imagePath($app, $i);
-						break;
+						$icon = $this->urlGenerator->imagePath($app, $icon);
 					} catch (\RuntimeException $ex) {
-						// no icon? - ignore it then
+						// ignore
 					}
 				}
+				if ($icon === null) {
+					$icon = $this->appManager->getAppIcon($app);
+				}
 				if ($icon === null) {
 					$icon = $this->urlGenerator->imagePath('core', 'default-app-icon');
 				}

lib/private/Server.php

@@ -893,7 +893,8 @@ class Server extends ServerContainer implements IServerContainer {
 				$c->get(IGroupManager::class),
 				$c->get(ICacheFactory::class),
 				$c->get(IEventDispatcher::class),
-				$c->get(LoggerInterface::class)
+				$c->get(LoggerInterface::class),
+				$c->get(IURLGenerator::class),
 			);
 		});
 		/** @deprecated 19.0.0 */

lib/public/App/IAppManager.php

@@ -61,6 +61,15 @@ interface IAppManager {
 	 */
 	public function getAppVersion(string $appId, bool $useCache = true): string;
 
+	/**
+	 * Returns the app icon or null if none is found
+	 *
+	 * @param string $appId
+	 * @return string|null
+	 * @since 29.0.0
+	 */
+	public function getAppIcon(string $appId): string|null;
+
 	/**
 	 * Check if an app is enabled for user
 	 *

tests/lib/App/AppManagerTest.php

@@ -23,6 +23,7 @@ use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\IGroup;
 use OCP\IGroupManager;
+use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\IUserSession;
 use PHPUnit\Framework\MockObject\MockObject;
@@ -98,6 +99,8 @@ class AppManagerTest extends TestCase {
 	/** @var LoggerInterface|MockObject */
 	protected $logger;
 
+	protected IURLGenerator|MockObject $urlGenerator;
+
 	/** @var IAppManager */
 	protected $manager;
 
@@ -112,6 +115,7 @@ class AppManagerTest extends TestCase {
 		$this->cache = $this->createMock(ICache::class);
 		$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 		$this->cacheFactory->expects($this->any())
 			->method('createDistributed')
 			->with('settings')
@@ -123,10 +127,74 @@ class AppManagerTest extends TestCase {
 			$this->groupManager,
 			$this->cacheFactory,
 			$this->eventDispatcher,
-			$this->logger
+			$this->logger,
+			$this->urlGenerator,
 		);
 	}
 
+	/**
+	 * @dataProvider dataGetAppIcon
+	 */
+	public function testGetAppIcon($callback, string|null $expected) {
+		$this->urlGenerator->expects($this->atLeastOnce())
+			->method('imagePath')
+			->willReturnCallback($callback);
+
+		$this->assertEquals($expected, $this->manager->getAppIcon('test'));
+	}
+
+	public function dataGetAppIcon(): array {
+		$nothing = function ($appId) {
+			$this->assertEquals('test', $appId);
+			throw new \RuntimeException();
+		};
+
+		$createCallback = function ($workingIcons) {
+			return function ($appId, $icon) use ($workingIcons) {
+				$this->assertEquals('test', $appId);
+				if (in_array($icon, $workingIcons)) {
+					return '/path/' . $icon;
+				}
+				throw new \RuntimeException();
+			};
+		};
+
+		return [
+			'does not find anything' => [
+				$nothing,
+				null,
+			],
+			'only app.svg' => [
+				$createCallback(['app.svg']),
+				'/path/app.svg',
+			],
+			'only app-dark.svg' => [
+				$createCallback(['app-dark.svg']),
+				'/path/app-dark.svg',
+			],
+			'only appname -dark.svg' => [
+				$createCallback(['test-dark.svg']),
+				'/path/test-dark.svg',
+			],
+			'only appname.svg' => [
+				$createCallback(['test.svg']),
+				'/path/test.svg',
+			],
+			'priotize custom over default' => [
+				$createCallback(['app.svg', 'test.svg']),
+				'/path/test.svg',
+			],
+			'priotize default over dark' => [
+				$createCallback(['test-dark.svg', 'app-dark.svg', 'app.svg']),
+				'/path/app.svg',
+			],
+			'priotize custom over default' => [
+				$createCallback(['test.svg', 'test-dark.svg', 'app-dark.svg']),
+				'/path/test.svg',
+			],
+		];
+	}
+
 	public function testEnableApp() {
 		// making sure "files_trashbin" is disabled
 		if ($this->manager->isEnabledForUser('files_trashbin')) {
@@ -170,9 +238,16 @@ class AppManagerTest extends TestCase {
 		/** @var AppManager|MockObject $manager */
 		$manager = $this->getMockBuilder(AppManager::class)
 			->setConstructorArgs([
-				$this->userSession, $this->config, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher, $this->logger
+				$this->userSession,
+				$this->config,
+				$this->appConfig,
+				$this->groupManager,
+				$this->cacheFactory,
+				$this->eventDispatcher,
+				$this->logger,
+				$this->urlGenerator,
 			])
-			->setMethods([
+			->onlyMethods([
 				'getAppPath',
 			])
 			->getMock();
@@ -218,9 +293,16 @@ class AppManagerTest extends TestCase {
 		/** @var AppManager|MockObject $manager */
 		$manager = $this->getMockBuilder(AppManager::class)
 			->setConstructorArgs([
-				$this->userSession, $this->config, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher, $this->logger
+				$this->userSession,
+				$this->config,
+				$this->appConfig,
+				$this->groupManager,
+				$this->cacheFactory,
+				$this->eventDispatcher,
+				$this->logger,
+				$this->urlGenerator,
 			])
-			->setMethods([
+			->onlyMethods([
 				'getAppPath',
 				'getAppInfo',
 			])
@@ -274,9 +356,16 @@ class AppManagerTest extends TestCase {
 		/** @var AppManager|MockObject $manager */
 		$manager = $this->getMockBuilder(AppManager::class)
 			->setConstructorArgs([
-				$this->userSession, $this->config, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher, $this->logger
+				$this->userSession,
+				$this->config,
+				$this->appConfig,
+				$this->groupManager,
+				$this->cacheFactory,
+				$this->eventDispatcher,
+				$this->logger,
+				$this->urlGenerator,
 			])
-			->setMethods([
+			->onlyMethods([
 				'getAppPath',
 				'getAppInfo',
 			])
@@ -470,8 +559,17 @@ class AppManagerTest extends TestCase {
 	public function testGetAppsNeedingUpgrade() {
 		/** @var AppManager|MockObject $manager */
 		$manager = $this->getMockBuilder(AppManager::class)
-			->setConstructorArgs([$this->userSession, $this->config, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher, $this->logger])
-			->setMethods(['getAppInfo'])
+			->setConstructorArgs([
+				$this->userSession,
+				$this->config,
+				$this->appConfig,
+				$this->groupManager,
+				$this->cacheFactory,
+				$this->eventDispatcher,
+				$this->logger,
+				$this->urlGenerator,
+			])
+			->onlyMethods(['getAppInfo'])
 			->getMock();
 
 		$appInfos = [
@@ -521,8 +619,17 @@ class AppManagerTest extends TestCase {
 	public function testGetIncompatibleApps() {
 		/** @var AppManager|MockObject $manager */
 		$manager = $this->getMockBuilder(AppManager::class)
-			->setConstructorArgs([$this->userSession, $this->config, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher, $this->logger])
-			->setMethods(['getAppInfo'])
+			->setConstructorArgs([
+				$this->userSession,
+				$this->config,
+				$this->appConfig,
+				$this->groupManager,
+				$this->cacheFactory,
+				$this->eventDispatcher,
+				$this->logger,
+				$this->urlGenerator,
+			])
+			->onlyMethods(['getAppInfo'])
 			->getMock();
 
 		$appInfos = [

tests/lib/AppTest.php

@@ -14,6 +14,8 @@ use OC\App\InfoParser;
 use OC\AppConfig;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IAppConfig;
+use OCP\IURLGenerator;
+use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 
 /**
@@ -537,6 +539,7 @@ class AppTest extends \Test\TestCase {
 
 
 	private function setupAppConfigMock() {
+		/** @var AppConfig|MockObject */
 		$appConfig = $this->getMockBuilder(AppConfig::class)
 			->setMethods(['getValues'])
 			->setConstructorArgs([\OC::$server->getDatabaseConnection()])
@@ -561,7 +564,8 @@ class AppTest extends \Test\TestCase {
 			\OC::$server->getGroupManager(),
 			\OC::$server->getMemCacheFactory(),
 			\OC::$server->get(IEventDispatcher::class),
-			\OC::$server->get(LoggerInterface::class)
+			\OC::$server->get(LoggerInterface::class),
+			\OC::$server->get(IURLGenerator::class),
 		));
 	}
 

tests/lib/NavigationManagerTest.php

@@ -224,19 +224,19 @@ class NavigationManagerTest extends TestCase {
 		   ->method('isEnabledForUser')
 		   ->with('theming')
 		   ->willReturn(true);
-		$this->appManager->expects($this->once())->method('getAppInfo')->with('test')->willReturn($navigation);
-		/*
+		$this->appManager->expects($this->once())
+			->method('getAppInfo')
+			->with('test')
+			->willReturn($navigation);
+		$this->urlGenerator->expects($this->any())
+			->method('imagePath')
+			->willReturnCallback(function ($appName, $file) {
+				return "/apps/$appName/img/$file";
+			});
 		$this->appManager->expects($this->any())
-				   ->method('getAppInfo')
-				   ->will($this->returnValueMap([
-					   ['test', null, null, $navigation],
-					   ['theming', null, null, null],
-					]));
-		 */
+			->method('getAppIcon')
+			->willReturnCallback(fn (string $appName) => "/apps/$appName/img/app.svg");
 		$this->l10nFac->expects($this->any())->method('get')->willReturn($l);
-		$this->urlGenerator->expects($this->any())->method('imagePath')->willReturnCallback(function ($appName, $file) {
-			return "/apps/$appName/img/$file";
-		});
 		$this->urlGenerator->expects($this->any())->method('linkToRoute')->willReturnCallback(function ($route) {
 			if ($route === 'core.login.logout') {
 				return 'https://example.com/logout';
@@ -534,6 +534,7 @@ class NavigationManagerTest extends TestCase {
 		   ->with('theming')
 		   ->willReturn(true);
 		$this->appManager->expects($this->once())->method('getAppInfo')->with('test')->willReturn($navigation);
+		$this->appManager->expects($this->once())->method('getAppIcon')->with('test')->willReturn('/apps/test/img/app.svg');
 		$this->l10nFac->expects($this->any())->method('get')->willReturn($l);
 		$this->urlGenerator->expects($this->any())->method('imagePath')->willReturnCallback(function ($appName, $file) {
 			return "/apps/$appName/img/$file";