Skip to content
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.

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

Svg color api #9984

Merged
merged 19 commits into from
Jul 19, 2018
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
10 changes: 7 additions & 3 deletions apps/accessibility/css/themedark.scss
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,16 @@ $color-border-dark: lighten($color-main-background, 14%);
}
}
}
.federation-menu .icon-federation-menu {
filter: invert(100%);
}
.bubble,
.app-navigation-entry-menu,
.popovermenu,
#header .menu {
border: 1px solid var(--color-border);
}

// since svg icons are inverted, revert to white for the header
.header-right > * {
[class^='icon-'], [class*=' icon-'] {
filter: invert(100%);
Copy link
Member

Choose a reason for hiding this comment

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

Can we get rid of this invert rule as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, header needs to use the proper urls
Or we need to exclude the variable invert for the header icons :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'd say we should exclude those. Their color anyway depends on the theming app color so we should keep that and not invert it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum, since we're changing the global variables, I'm not sure we can ignore them :/

Copy link
Member

Choose a reason for hiding this comment

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

How about just setting the icon for the header with $color-primary-text instead of $color-white? Then they should not be inverted, if I get the code right.

Copy link
Member

Choose a reason for hiding this comment

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

So we would have the regular .icon-contacts with $color-white and a header .icon-contacts with $color-primary-text. Yes this will be some duplication, but the icons where this is used are quite limited, so it would be ok i think.

Copy link
Member Author

Choose a reason for hiding this comment

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

@juliushaertl Of course!!! Great thinking! 👍 🤗

}
}
25 changes: 23 additions & 2 deletions apps/accessibility/lib/Controller/AccessibilityController.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
use OCP\IURLGenerator;
use OCP\IUserManager;
use OCP\IUserSession;
use OC\Template\IconsCacher;

class AccessibilityController extends Controller {

Expand Down Expand Up @@ -66,6 +67,9 @@ class AccessibilityController extends Controller {
/** @var IAppManager */
private $appManager;

/** @var IconsCacher */
protected $iconsCacher;

/**
* Account constructor.
*
Expand All @@ -87,7 +91,8 @@ public function __construct(string $appName,
IURLGenerator $urlGenerator,
ITimeFactory $timeFactory,
IUserSession $userSession,
IAppManager $appManager) {
IAppManager $appManager,
IconsCacher $iconsCacher) {
parent::__construct($appName, $request);
$this->appName = $appName;
$this->config = $config;
Expand All @@ -97,6 +102,7 @@ public function __construct(string $appName,
$this->timeFactory = $timeFactory;
$this->userSession = $userSession;
$this->appManager = $appManager;
$this->iconsCacher = $iconsCacher;

$this->serverRoot = \OC::$SERVERROOT;
$this->appRoot = $this->appManager->getAppPath($this->appName);
Expand Down Expand Up @@ -147,7 +153,12 @@ public function getCss(): DataDisplayResponse {

// Rebase all urls
$appWebRoot = substr($this->appRoot, strlen($this->serverRoot) - strlen(\OC::$WEBROOT));
$css = $this->rebaseUrls($css, $appWebRoot . '/css');
$css = $this->rebaseUrls($css, $appWebRoot . '/css');

if (in_array('themedark', $userValues) && $this->iconsCacher->getCachedCSS() && $this->iconsCacher->getCachedCSS()->getSize() > 0) {
$iconsCss = $this->invertSvgIconsColor($this->iconsCacher->getCachedCSS()->getContent());
$css = $css . $iconsCss;
}

$response = new DataDisplayResponse($css, Http::STATUS_OK, ['Content-Type' => 'text/css']);

Expand Down Expand Up @@ -199,4 +210,14 @@ private function rebaseUrls(string $css, string $webDir): string {

return preg_replace($re, $subst, $css);
}

/**
* Remove all matches from the $rule regex
*
* @param string $css string to parse
* @return string
*/
private function invertSvgIconsColor(string $css) {
return str_replace(['/000', '/fff', '/***'], ['/***', '/000', '/fff'], $css);
}
}
2 changes: 1 addition & 1 deletion apps/dav/lib/CalDAV/Activity/Filter/Calendar.php
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public function getPriority() {
* @since 11.0.0
*/
public function getIcon() {
return $this->url->getAbsoluteURL($this->url->imagePath('core', 'places/calendar-dark.svg'));
return $this->url->getAbsoluteURL($this->url->imagePath('core', 'places/calendar.svg'));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ protected function setUp() {
public function testGetIcon() {
$this->url->expects($this->once())
->method('imagePath')
->with('core', 'places/calendar-dark.svg')
->with('core', 'places/calendar.svg')
->willReturn('path-to-icon');

$this->url->expects($this->once())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@

/* icons for sidebar */
.nav-icon-basic-encryption-module {
background-image: url('../img/app.svg?v=1');
@include icon-color('app', 'encryption', $color-black);
}
21 changes: 10 additions & 11 deletions apps/files/css/files.scss
Original file line number Diff line number Diff line change
Expand Up @@ -84,30 +84,29 @@

/* icons for sidebar */
.nav-icon-files {
background-image: url('../img/folder.svg?v=1');
@include icon-color('folder', 'files', $color-black);
}
.nav-icon-recent {
background-image: url('../img/recent.svg?v=1');
@include icon-color('recent', 'files', $color-black);
}
.nav-icon-favorites {
background-image: url('../img/star.svg?v=1');
@include icon-color('star-dark', 'files', $color-black, 2, true);
}
.nav-icon-sharingin,
.nav-icon-sharingout,
.nav-icon-shareoverview {
background-image: url('../img/share.svg?v=1');
.nav-icon-sharingout {
@include icon-color('share', 'files', $color-black);
}
.nav-icon-sharinglinks {
background-image: url('../img/public.svg?v=1');
@include icon-color('public', 'files', $color-black);
}
.nav-icon-extstoragemounts {
background-image: url('../img/external.svg?v=1');
@include icon-color('external', 'files', $color-black);
}
.nav-icon-trashbin {
background-image: url('../img/delete.svg?v=1');
@include icon-color('delete', 'files', $color-black);
}
.nav-icon-deletedshares {
background-image: url('../img/unshare.svg?v=1');
@include icon-color('unshare', 'files', $color-black);
}

#app-navigation .nav-files a.nav-icon-files {
Expand Down Expand Up @@ -668,7 +667,7 @@ table.dragshadow td.size {
background-image: none;
}
& .icon-starred {
background-image: url('../../../core/img/actions/starred.svg?v=1');
@include icon-color('star-dark', 'actions', 'FC0', 1, true);
}
}

Expand Down
2 changes: 1 addition & 1 deletion apps/files/lib/Activity/Filter/FileChanges.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public function getPriority() {
* @since 11.0.0
*/
public function getIcon() {
return $this->url->getAbsoluteURL($this->url->imagePath('core', 'places/files-dark.svg'));
return $this->url->getAbsoluteURL($this->url->imagePath('core', 'places/files.svg'));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion apps/files_external/css/settings.scss
Original file line number Diff line number Diff line change
Expand Up @@ -122,5 +122,5 @@ td.mountPoint, td.backend { width:160px; }
}

.nav-icon-external-storage {
background-image: url('../img/app-dark.svg?v=1');
@include icon-color('app-dark', 'files_external', $color-black);
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
}

.nav-icon-systemtagsfilter {
background-image: url('../img/tag.svg');
@include icon-color('tag', 'systemtags', $color-black);
}

#app-sidebar .mainFileInfoView .tag-label {
Expand Down
2 changes: 1 addition & 1 deletion autotest-js.sh
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ for SCSSFILE in core/css/*.scss
do
FILE=$(basename $SCSSFILE)
FILENAME="${FILE%.*}"
printf "@import 'variables.scss'; @import '${FILE}';" | ./build/bin/node-sass --include-path core/css/ > tests/css/${FILE}.css
printf "\$webroot:''; @import 'functions.scss'; @import 'variables.scss'; @import '${FILE}';" | ./build/bin/node-sass --include-path core/css/ > tests/css/${FILE}.css
done

KARMA="$PREFIX/node_modules/karma/bin/karma"
Expand Down
4 changes: 2 additions & 2 deletions core/Controller/JsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ public function __construct($appName, IRequest $request, Factory $appDataFactory
* @PublicPage
* @NoCSRFRequired
*
* @param string $fileName css filename with extension
* @param string $appName css folder name
* @param string $fileName js filename with extension
* @param string $appName js folder name
* @return FileDisplayResponse|NotFoundResponse
*/
public function getJs(string $fileName, string $appName): Response {
Expand Down
136 changes: 136 additions & 0 deletions core/Controller/SvgController.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
<?php
declare (strict_types = 1);
/**
* @copyright Copyright (c) 2018, John Molakvoæ (skjnldsv@protonmail.com)
*
* @author John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OC\Core\Controller;

use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\DataDisplayResponse;
use OCP\AppFramework\Http\NotFoundResponse;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\Files\NotFoundException;
use OCP\IRequest;

class SvgController extends Controller {

/** @var string */
protected $serverRoot;

/** @var ITimeFactory */
protected $timeFactory;

public function __construct(string $appName,
IRequest $request,
ITimeFactory $timeFactory) {
parent::__construct($appName, $request);

$this->serverRoot = \OC::$SERVERROOT;
$this->timeFactory = $timeFactory;
}

/**
* @NoAdminRequired
* @NoCSRFRequired
*
* Generate svg from filename with the requested color
*
* @param string $folder
* @param string $fileName
* @param string $color
* @return DataDisplayResponse|NotFoundException
Copy link
Member

Choose a reason for hiding this comment

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

You don't return an exception you throw it ;)

*/
public function getSvgFromCore(string $folder, string $fileName, string $color = 'ffffff') {
Copy link
Member

Choose a reason for hiding this comment

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

You can also declare return types ;)

$path = $this->serverRoot . "/core/img/$folder/$fileName.svg";
return $this->getSvg($path, $color, $fileName);
}

/**
* @NoAdminRequired
* @NoCSRFRequired
*
* Generate svg from filename with the requested color
*
* @param string $app
* @param string $fileName
* @param string $color
* @return DataDisplayResponse|NotFoundException
Copy link
Member

Choose a reason for hiding this comment

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

NotFoundResponse it seems

*/
public function getSvgFromApp(string $app, string $fileName, string $color = 'ffffff') {

if ($app === 'settings') {
$path = $this->serverRoot . "/settings/img/$fileName.svg";
return $this->getSvg($path, $color, $fileName);
}

$appPath = \OC_App::getAppWebPath($app);
Copy link
Member

Choose a reason for hiding this comment

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

Can't the appmanager do this?

if (!$appPath) {
return new NotFoundResponse();
}
$path = $this->serverRoot . $appPath ."/img/$fileName.svg";
return $this->getSvg($path, $color, $fileName);
}


/**
* Generate svg from filename with the requested color
*
* @param string $path
* @param string $color
* @return DataDisplayResponse|NotFoundException
*/
private function getSvg(string $path, string $color, string $fileName) {
if (!file_exists($path)) {
return new NotFoundResponse();
}

$svg = file_get_contents($path);

if (is_null($svg)) {
return new NotFoundResponse();
}

// add fill (fill is not present on black elements)
$fillRe = '/<((circle|rect|path)((?!fill)[a-z0-9 =".\-#():;])+)\/>/mi';

$svg = preg_replace($fillRe, '<$1 fill="#' . $color . '"/>', $svg);

// replace any fill or stroke colors
$svg = preg_replace('/stroke="#([a-z0-9]{3,6})"/mi', 'stroke="#' . $color . '"', $svg);
$svg = preg_replace('/fill="#([a-z0-9]{3,6})"/mi', 'fill="#' . $color . '"', $svg);

$response = new DataDisplayResponse($svg, Http::STATUS_OK, ['Content-Type' => 'image/svg+xml']);

// Set cache control
$ttl = 31536000;
$response->cacheFor($ttl);
$response->addHeader('Content-Disposition', 'inline; filename="' . $fileName . '.svg"');
$expires = new \DateTime();
$expires->setTimestamp($this->timeFactory->getTime());
$expires->add(new \DateInterval('PT' . $ttl . 'S'));
$response->addHeader('Expires', $expires->format(\DateTime::RFC1123));
$response->addHeader('Pragma', 'cache');

return $response;
}
}
2 changes: 1 addition & 1 deletion core/css/apps.scss
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,7 @@ kbd {
padding: 0;
margin: 0;
background-color: var(--color-main-background);
background-image: url('../img/actions/settings-dark.svg?v=1');
@include icon-color('settings-dark', 'actions', $color-black, 1, true);
background-position: 14px center;
background-repeat: no-repeat;
box-shadow: none;
Expand Down
Loading