Skip to content

fix: generate favourite icon without imagick svg support #55132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SebastianKrupinski wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: generate favourite icon without imagick svg support #55132

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69057a5
fix: generate favourite icon without imagick svg support
SebastianKrupinski Sep 15, 2025
d2f2f62
fixup! fix: generate favourite icon without imagick svg support
SebastianKrupinski Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions apps/theming/lib/Controller/IconController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use OCP\AppFramework\Http\NotFoundResponse;
use OCP\AppFramework\Http\Response;
use OCP\Files\NotFoundException;
use OCP\IConfig;
use OCP\IRequest;

class IconController extends Controller {
Expand All @@ -30,6 +31,7 @@ class IconController extends Controller {
public function __construct(
$appName,
IRequest $request,
private IConfig $config,
private ThemingDefaults $themingDefaults,
private IconBuilder $iconBuilder,
private ImageManager $imageManager,
Expand Down Expand Up @@ -79,7 +81,7 @@ public function getThemedIcon(string $app, string $image): Response {
* Return a 32x32 favicon as png
*
* @param string $app ID of the app
* @return DataDisplayResponse<Http::STATUS_OK, array{Content-Type: 'image/x-icon'}>|FileDisplayResponse<Http::STATUS_OK, array{Content-Type: 'image/x-icon'}>|NotFoundResponse<Http::STATUS_NOT_FOUND, array{}>
* @return DataDisplayResponse<Http::STATUS_OK, array{Content-Type: 'image/png'}>|FileDisplayResponse<Http::STATUS_OK, array{Content-Type: 'image/x-icon'}>|NotFoundResponse<Http::STATUS_NOT_FOUND, array{}>
* @throws \Exception
*
* 200: Favicon returned
Expand All @@ -95,12 +97,14 @@ public function getFavicon(string $app = 'core'): Response {

$response = null;
$iconFile = null;
// retrieve instance favorite icon
Copy link
Contributor

@kesselb kesselb Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code already shows that we retrieve the icon. A comment would be more useful if it explained the reasoning or any non-obvious behavior.

Copy link
Contributor Author

@SebastianKrupinski SebastianKrupinski Dec 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does explain what we are doing? We are retrieving the user instance configured favorite icon.

Copy link
Contributor

@kesselb kesselb Dec 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// retrieve instance favorite icon
// retrieve instance favicon

Copy link
Contributor Author

@SebastianKrupinski SebastianKrupinski Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

try {
$iconFile = $this->imageManager->getImage('favicon', false);
$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
} catch (NotFoundException $e) {
}
if ($iconFile === null && $this->imageManager->shouldReplaceIcons()) {
// retrieve or generate app specific favorite icon
Copy link
Contributor

@kesselb kesselb Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// retrieve or generate app specific favorite icon
// retrieve or generate app specific favicon

Copy link
Contributor

@kesselb kesselb Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest removing this comment unless there's additional context we should know. Right now it's just echoing the code.

Copy link
Contributor Author

@SebastianKrupinski SebastianKrupinski Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep these in, they break up the code into specific logical section and tell us which icon is being worked with. User Instance icon, App specific icon or Generic core icon.

if (($this->imageManager->canConvert('PNG') || $this->imageManager->canConvert('SVG')) && $this->imageManager->canConvert('ICO')) {
$color = $this->themingDefaults->getColorPrimary();
try {
$iconFile = $this->imageManager->getCachedImage('favIcon-' . $app . $color);
Expand All @@ -113,9 +117,10 @@ public function getFavicon(string $app = 'core'): Response {
}
$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
}
// fallback to core favorite icon
Copy link
Contributor

@kesselb kesselb Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// fallback to core favorite icon
// fallback to core favicon

Copy link
Contributor Author

@SebastianKrupinski SebastianKrupinski Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

if ($response === null) {
$fallbackLogo = \OC::$SERVERROOT . '/core/img/favicon.png';
$response = new DataDisplayResponse($this->fileAccessHelper->file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
$response = new DataDisplayResponse($this->fileAccessHelper->file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/png']);
}
$response->cacheFor(86400);
return $response;
Expand All @@ -125,7 +130,7 @@ public function getFavicon(string $app = 'core'): Response {
* Return a 512x512 icon for touch devices
*
* @param string $app ID of the app
* @return DataDisplayResponse<Http::STATUS_OK, array{Content-Type: 'image/png'}>|FileDisplayResponse<Http::STATUS_OK, array{Content-Type: 'image/x-icon'|'image/png'}>|NotFoundResponse<Http::STATUS_NOT_FOUND, array{}>
* @return DataDisplayResponse<Http::STATUS_OK, array{Content-Type: 'image/png'}>|FileDisplayResponse<Http::STATUS_OK, array{Content-Type: string}>|NotFoundResponse<Http::STATUS_NOT_FOUND, array{}>
* @throws \Exception
*
* 200: Touch icon returned
Expand All @@ -140,12 +145,14 @@ public function getTouchIcon(string $app = 'core'): Response {
}

$response = null;
// retrieve instance favorite icon
try {
$iconFile = $this->imageManager->getImage('favicon');
$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => $iconFile->getMimeType()]);
} catch (NotFoundException $e) {
}
if ($this->imageManager->shouldReplaceIcons()) {
// retrieve or generate app specific touch icon
if ($this->imageManager->canConvert('PNG')) {
$color = $this->themingDefaults->getColorPrimary();
try {
$iconFile = $this->imageManager->getCachedImage('touchIcon-' . $app . $color);
Expand All @@ -158,6 +165,7 @@ public function getTouchIcon(string $app = 'core'): Response {
}
$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/png']);
}
// fallback to core touch icon
if ($response === null) {
$fallbackLogo = \OC::$SERVERROOT . '/core/img/favicon-touch.png';
$response = new DataDisplayResponse($this->fileAccessHelper->file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/png']);
Expand Down
8 changes: 2 additions & 6 deletions apps/theming/lib/Controller/ThemingController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,7 @@ public function undoAll(): DataResponse {
#[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)]
public function getImage(string $key, bool $useSvg = true) {
try {
$useSvg = $useSvg && $this->imageManager->canConvert('SVG');
$file = $this->imageManager->getImage($key, $useSvg);
} catch (NotFoundException $e) {
return new NotFoundResponse();
Expand All @@ -355,13 +356,8 @@ public function getImage(string $key, bool $useSvg = true) {
$csp->allowInlineStyle();
$response->setContentSecurityPolicy($csp);
$response->cacheFor(3600);
$response->addHeader('Content-Type', $this->config->getAppValue($this->appName, $key . 'Mime', ''));
$response->addHeader('Content-Type', $file->getMimeType());
$response->addHeader('Content-Disposition', 'attachment; filename="' . $key . '"');
if (!$useSvg) {
$response->addHeader('Content-Type', 'image/png');
} else {
$response->addHeader('Content-Type', $this->config->getAppValue($this->appName, $key . 'Mime', ''));
}
return $response;
}

Expand Down
168 changes: 97 additions & 71 deletions apps/theming/lib/IconBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
namespace OCA\Theming;

use Imagick;
use ImagickDraw;
use ImagickPixel;
use OCP\Files\SimpleFS\ISimpleFile;

Expand All @@ -30,17 +31,18 @@ public function __construct(
* @return string|false image blob
*/
public function getFavicon($app) {
if (!$this->imageManager->shouldReplaceIcons()) {
if (!$this->imageManager->canConvert('PNG')) {
return false;
}
try {
$favicon = new Imagick();
$favicon->setFormat('ico');
$icon = $this->renderAppIcon($app, 128);
if ($icon === false) {
return false;
}
$icon->setImageFormat('png32');
$icon->setImageFormat('PNG32');

$favicon = new Imagick();
$favicon->setFormat('ICO');

$clone = clone $icon;
$clone->scaleImage(16, 0);
Expand Down Expand Up @@ -96,7 +98,9 @@ public function getTouchIcon($app) {
* @return Imagick|false
*/
public function renderAppIcon($app, $size) {
$appIcon = $this->util->getAppIcon($app);
$supportSvg = $this->imageManager->canConvert('SVG');
// retrieve app icon
$appIcon = $this->util->getAppIcon($app, $supportSvg);
if ($appIcon instanceof ISimpleFile) {
$appIconContent = $appIcon->getContent();
$mime = $appIcon->getMimeType();
Expand All @@ -111,79 +115,101 @@ public function renderAppIcon($app, $size) {
return false;
}

$color = $this->themingDefaults->getColorPrimary();
$appIconFile = null;
$appIconIsSvg = ($mime === 'image/svg+xml' || substr($appIconContent, 0, 4) === '<svg');

// generate background image with rounded corners
$cornerRadius = 0.2 * $size;
$background = '<?xml version="1.0" encoding="UTF-8"?>'
. '<svg xmlns="http://www.w3.org/2000/svg" version="1.1" xmlns:cc="http://creativecommons.org/ns#" width="' . $size . '" height="' . $size . '" xmlns:xlink="http://www.w3.org/1999/xlink">'
. '<rect x="0" y="0" rx="' . $cornerRadius . '" ry="' . $cornerRadius . '" width="' . $size . '" height="' . $size . '" style="fill:' . $color . ';" />'
. '</svg>';
// resize svg magic as this seems broken in Imagemagick
if ($mime === 'image/svg+xml' || substr($appIconContent, 0, 4) === '<svg') {
if (substr($appIconContent, 0, 5) !== '<?xml') {
$svg = '<?xml version="1.0"?>' . $appIconContent;
} else {
$svg = $appIconContent;
}
$tmp = new Imagick();
$tmp->setBackgroundColor(new ImagickPixel('transparent'));
$tmp->setResolution(72, 72);
$tmp->readImageBlob($svg);
$x = $tmp->getImageWidth();
$y = $tmp->getImageHeight();
$tmp->destroy();

// convert svg to resized image
// if source image is svg but svg not supported, abort
if ($appIconIsSvg && !$supportSvg) {
return false;
}

try {
// construct original image object
$appIconFile = new Imagick();
$resX = (int)(72 * $size / $x);
$resY = (int)(72 * $size / $y);
$appIconFile->setResolution($resX, $resY);
$appIconFile->setBackgroundColor(new ImagickPixel('transparent'));
$appIconFile->readImageBlob($svg);

/**
* invert app icons for bright primary colors
* the default nextcloud logo will not be inverted to black
*/
if ($this->util->isBrightColor($color)
&& !$appIcon instanceof ISimpleFile
&& $app !== 'core'
) {
$appIconFile->negateImage(false);

if ($appIconIsSvg) {
// handle SVG images
// ensure proper XML declaration
if (substr($appIconContent, 0, 5) !== '<?xml') {
$svg = '<?xml version="1.0"?>' . $appIconContent;
} else {
$svg = $appIconContent;
}
// get dimensions for resolution calculation
$tmp = new Imagick();
$tmp->setBackgroundColor(new ImagickPixel('transparent'));
$tmp->setResolution(72, 72);
$tmp->readImageBlob($svg);
$x = $tmp->getImageWidth();
$y = $tmp->getImageHeight();
$tmp->destroy();
// set resolution for proper scaling
$resX = (int)(72 * $size / $x);
$resY = (int)(72 * $size / $y);
$appIconFile->setResolution($resX, $resY);
$appIconFile->readImageBlob($svg);
} else {
// handle non-SVG images
$appIconFile->readImageBlob($appIconContent);
}
} else {
$appIconFile = new Imagick();
$appIconFile->setBackgroundColor(new ImagickPixel('transparent'));
$appIconFile->readImageBlob($appIconContent);
} catch (\ImagickException $e) {
return false;
}
// offset for icon positioning
$padding = 0.15;
$border_w = (int)($appIconFile->getImageWidth() * $padding);
$border_h = (int)($appIconFile->getImageHeight() * $padding);
$innerWidth = ($appIconFile->getImageWidth() - $border_w * 2);
$innerHeight = ($appIconFile->getImageHeight() - $border_h * 2);
$appIconFile->adaptiveResizeImage($innerWidth, $innerHeight);
// center icon
$offset_w = (int)($size / 2 - $innerWidth / 2);
$offset_h = (int)($size / 2 - $innerHeight / 2);

$finalIconFile = new Imagick();
$finalIconFile->setBackgroundColor(new ImagickPixel('transparent'));
$finalIconFile->readImageBlob($background);
$finalIconFile->setImageVirtualPixelMethod(Imagick::VIRTUALPIXELMETHOD_TRANSPARENT);
$finalIconFile->setImageArtifact('compose:args', '1,0,-0.5,0.5');
$finalIconFile->compositeImage($appIconFile, Imagick::COMPOSITE_ATOP, $offset_w, $offset_h);
$finalIconFile->setImageFormat('png24');
if (defined('Imagick::INTERPOLATE_BICUBIC') === true) {
$filter = Imagick::INTERPOLATE_BICUBIC;
} else {
$filter = Imagick::FILTER_LANCZOS;
// calculate final image size and position
$padding = 0.85;
$original_w = $appIconFile->getImageWidth();
$original_h = $appIconFile->getImageHeight();
$contentSize = (int)floor($size * $padding);
$scale = min($contentSize / $original_w, $contentSize / $original_h);
$new_w = max(1, (int)floor($original_w * $scale));
$new_h = max(1, (int)floor($original_h * $scale));
$offset_w = (int)floor(($size - $new_w) / 2);
$offset_h = (int)floor(($size - $new_h) / 2);
$cornerRadius = 0.2 * $size;
$color = $this->themingDefaults->getColorPrimary();
// resize original image
$appIconFile->resizeImage($new_w, $new_h, Imagick::FILTER_LANCZOS, 1);
/**
* invert app icons for bright primary colors
* the default nextcloud logo will not be inverted to black
*/
if ($this->util->isBrightColor($color)
&& !$appIcon instanceof ISimpleFile
&& $app !== 'core'
) {
$appIconFile->negateImage(false);
}
// construct final image object
try {
// image background
$finalIconFile = new Imagick();
$finalIconFile->setBackgroundColor(new ImagickPixel('transparent'));
// icon background
$finalIconFile->newImage($size, $size, new ImagickPixel('transparent'));
$draw = new ImagickDraw();
$draw->setFillColor($color);
$draw->roundRectangle(0, 0, $size - 1, $size - 1, $cornerRadius, $cornerRadius);
$finalIconFile->drawImage($draw);
$draw->destroy();
// overlay icon
$finalIconFile->setImageVirtualPixelMethod(Imagick::VIRTUALPIXELMETHOD_TRANSPARENT);
$finalIconFile->setImageArtifact('compose:args', '1,0,-0.5,0.5');
$finalIconFile->compositeImage($appIconFile, Imagick::COMPOSITE_ATOP, $offset_w, $offset_h);
$finalIconFile->setImageFormat('PNG32');
if (defined('Imagick::INTERPOLATE_BICUBIC') === true) {
$filter = Imagick::INTERPOLATE_BICUBIC;
} else {
$filter = Imagick::FILTER_LANCZOS;
}
$finalIconFile->resizeImage($size, $size, $filter, 1, false);

return $finalIconFile;
} finally {
unset($appIconFile);
}
$finalIconFile->resizeImage($size, $size, $filter, 1, false);

$appIconFile->destroy();
return $finalIconFile;
return false;
Copy link
Contributor

@kesselb kesselb Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you forgot the catch block for the exception?

Copy link
Contributor Author

@SebastianKrupinski SebastianKrupinski Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this is just a default return, this should never get triggered but I did not want to change the function signature. The original code did not throw a error, so just matching that.

Copy link
Contributor

@kesselb kesselb Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try-catch-finally block does not catch any exceptions. That means if an error occurs, the variable is unset and the exception still bubbles up. If no exception occurs, the function returns earlier. This makes the final return block unreachable. It is not documented that the functions throw and the previous try-catch block does catch the ImagickException, so I wanted to check whether the catch block is missing.

}

/**
Expand Down
Loading
Loading