Add dynamic icon creation #840

juliushaertl · 2016-08-11T21:49:11Z

So this basically addresses #218 and #378. With this change the theming app offers two new endpoints for getting themed icons.
Return a image with logo and colored background:
http://localhost:8080/index.php/apps/theming/favicon/appname
Return a svg icon replacing the default color with the themed one:
http://localhost:8080/index.php/apps/theming/img/appname/filepath

Results:

use theming logo instead of nextcloud if available
touch homescreen icons
search icons.css for classes like icon-folder to replace the icon with a themed one
Add caching
check if there are directory traversal issues
Unit tests
code cleanup

mention-bot · 2016-08-11T21:49:13Z

@juliushaertl, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bartv2, @rullzer and @LukasReschke to be potential reviewers

rullzer · 2016-08-14T16:37:39Z

Mmmm I guess we kind of have to at some point. But we should add a big cache and cache buster (changed only when the theme is updated!) to make sure this hits the server as few times as possible. Because we will setup a lot of stuff to server an icon.

awesome initiative @juliushaertl!

juliushaertl · 2016-08-15T09:45:48Z

I'm still trying to figure out how caching could be implemented. I think having the files on the file system makes the most sense.

For now I think I'll just store them into the data/ folder, like it is currently happening with the logo and background image, but this will become a huge mess due to number of icons.
Therefore #136 looks promising.

rullzer · 2016-08-15T10:01:43Z

@juliushaertl no the point is that right now all icons are serverd by the webserver. No PHP is involved etc.

We can easily add proper caching headers. Just let me know if you need help.

jancborchardt · 2016-08-26T08:37:32Z

Awesome @juliushaertl! :) Only thing is that the favicon logo is black whereas the normal logo is white. The favicon should be the same as the chosen logo. :)

juliushaertl · 2016-08-26T10:11:40Z

@jancborchardt ah right, I'll remove that.

I should have some time later today to finish this.

juliushaertl · 2016-08-29T15:05:37Z

OK, i'll need some more time, as testing the Imagemagick stuff will be a bit harder. But I'm on it. 😄

juliushaertl · 2016-08-30T10:13:12Z

Finally ready for review! @nextcloud/theming

@rullzer It would be great if you could have a look if the caching is fine. (apps/theming/lib/Controller/IconController.php)

It would also be good if someone could have a look if there are any problems with directory traversal. The route Icon#getThemedIcon accepts paths within the image parameter to allow getting images inside a subdirectory ( like mimetypes/folder.svg to get /core/img/mimetypes/folder.svg).
It seems like those are already prevented, but it would be good to have someone
check that as well. @nextcloud/security

MorrisJobke · 2016-08-30T12:51:41Z

Tested and works in Firefox. Opera, Safari, Chrome, Edge, IE11 and IE10 🙌 👍

rullzer · 2016-08-31T13:47:59Z

apps/theming/lib/Controller/IconController.php

+		$response->cacheFor(86400);
+		$response->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
+		$response->addHeader('Pragma', 'cache');
+		return $response;


You don't store the colored svg anywhere?
If you have a themed instance with 50K users you will still have to do this for 50K users.

Also if you have a file that you server we could use #1121. you could set the ETag. That way if the file did not change the request is fast.

We can easily add proper caching headers.

OK, maybe I misinterpreded your previous answer. I already thought that storing the generated icons in the filesystem would make sense. Do you have any hint how I could implement that?

We can't store it in the user-folders as these are also required for public/login page. I guess we should not just put them inside the data folder. That is why I thought that #136 might be important. Or is it possible to use a simple memory cache for these icons, as the number of those generated icons will stay quite low?

It would be great, if you could point me into the right direction.

Hehe. Well there are 2 things of course.

We want to cache server side so serving is fast

We want to cache client side so we don't have to request stuff from the server every time

They are two different optimizations but work towards the same goal of less request => less server load & faster loading

I'll see if we can make #136 happen sooner rather than later. because I don't really feel comfortable just dumping this all in the root data folder.

That would be great. :)

Signed-off-by: Julius Haertl <jus@bitgrid.net>

juliushaertl · 2016-11-18T10:53:23Z

@rullzer @LukasReschke @nextcloud/theming Changes are in. Ready for another review.

MorrisJobke · 2016-11-18T14:35:37Z

👍

jancborchardt · 2016-11-18T14:37:33Z

Good stuff @juliushaertl :) as always

juliushaertl · 2016-11-18T14:43:08Z

🎉

@jancborchardt Thanks. I'm always glad to help. 😉

juliushaertl added enhancement 2. developing Work in progress feature: theming labels Aug 11, 2016

juliushaertl mentioned this pull request Aug 24, 2016

Theming: Add logo and background to ThemingDefaults #1026

Merged

juliushaertl force-pushed the theming-icon-endpoint branch from 3cca71f to 98346a1 Compare August 25, 2016 14:20

LukasReschke mentioned this pull request Aug 26, 2016

Theming: Fix missing color usage #771

Merged

5 tasks

oparoz added this to the Nextcloud 11.0 milestone Aug 26, 2016

juliushaertl force-pushed the theming-icon-endpoint branch from 98346a1 to 2bc77d0 Compare August 26, 2016 15:55

jancborchardt mentioned this pull request Aug 26, 2016

All theming issues still to be fixed #454

Closed

10 tasks

juliushaertl force-pushed the theming-icon-endpoint branch 2 times, most recently from 9880e58 to cc88a84 Compare August 29, 2016 10:32

juliushaertl force-pushed the theming-icon-endpoint branch 2 times, most recently from 86bcc4a to 6fcd411 Compare August 30, 2016 07:50

juliushaertl added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 30, 2016

juliushaertl force-pushed the theming-icon-endpoint branch from d4d7b0d to a34091b Compare August 30, 2016 10:02

LukasReschke self-assigned this Aug 30, 2016

rullzer reviewed Aug 31, 2016
View reviewed changes

juliushaertl force-pushed the theming-icon-endpoint branch from a34091b to f4550d0 Compare September 21, 2016 11:40

juliushaertl added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 21, 2016

juliushaertl added 15 commits November 18, 2016 10:23

Theming: Add icon caching

6c7ebb1

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Fix IconController after rebase

4945592

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Add cachebuster to favicons

9ac5476

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Theming: Add IconController tests

ac7f852

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Improve unit tests for image generation

b466628

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Add IconBuilder class to encapsulate icon generation

af8976a

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Check if dynamic icons can be used

2370348

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Theming: Code cleanup and cache buster for mime icons

9e28a3b

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Caching for icon files using AppData

492d0b9

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Improve caching

2e8dd21

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Fix svg resizing and remove deprecated method call

43097ea

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Fix icon-folder css

cc8b1d3

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Replace null return with NotFoundException

3a400f9

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Sanitize input and small fixes

78de213

Signed-off-by: Julius Haertl <jus@bitgrid.net>

Error handling and tests if file was not found

d409fe1

Signed-off-by: Julius Haertl <jus@bitgrid.net>

juliushaertl force-pushed the theming-icon-endpoint branch from 56a5f4a to 2b4bc2e Compare November 18, 2016 09:49

Use IAppManager instead of OC_App

2ab4d1e

Signed-off-by: Julius Haertl <jus@bitgrid.net>

juliushaertl force-pushed the theming-icon-endpoint branch from 2b4bc2e to 2ab4d1e Compare November 18, 2016 10:08

MorrisJobke merged commit faee255 into master Nov 18, 2016

MorrisJobke deleted the theming-icon-endpoint branch November 18, 2016 14:35

This was referenced Nov 18, 2016

Theming: coloring of folders and log in background #378

Closed

Theming app: also change other icons to selected logo #218

Closed

Theming: Add documentation on themed icons nextcloud/documentation#233

Closed

jancborchardt mentioned this pull request Nov 25, 2016

add app-specific favicon for better recognition among tabs nextcloud/spreed#133

Merged

This was referenced Dec 5, 2016

Add documentation on icon theming (closes #233) nextcloud/documentation#248

Merged

coloring of external storage folders not working #2574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic icon creation #840

Add dynamic icon creation #840

juliushaertl commented Aug 11, 2016 •

edited

Loading

mention-bot commented Aug 11, 2016

rullzer commented Aug 14, 2016

juliushaertl commented Aug 15, 2016

rullzer commented Aug 15, 2016

jancborchardt commented Aug 26, 2016

juliushaertl commented Aug 26, 2016

juliushaertl commented Aug 29, 2016

juliushaertl commented Aug 30, 2016

MorrisJobke commented Aug 30, 2016

rullzer Aug 31, 2016

juliushaertl Aug 31, 2016

rullzer Sep 1, 2016

juliushaertl Sep 4, 2016

juliushaertl commented Nov 18, 2016

MorrisJobke commented Nov 18, 2016

jancborchardt commented Nov 18, 2016

juliushaertl commented Nov 18, 2016

Add dynamic icon creation #840

Add dynamic icon creation #840

Conversation

juliushaertl commented Aug 11, 2016 • edited Loading

mention-bot commented Aug 11, 2016

rullzer commented Aug 14, 2016

juliushaertl commented Aug 15, 2016

rullzer commented Aug 15, 2016

jancborchardt commented Aug 26, 2016

juliushaertl commented Aug 26, 2016

juliushaertl commented Aug 29, 2016

juliushaertl commented Aug 30, 2016

MorrisJobke commented Aug 30, 2016

rullzer Aug 31, 2016

Choose a reason for hiding this comment

juliushaertl Aug 31, 2016

Choose a reason for hiding this comment

rullzer Sep 1, 2016

Choose a reason for hiding this comment

juliushaertl Sep 4, 2016

Choose a reason for hiding this comment

juliushaertl commented Nov 18, 2016

MorrisJobke commented Nov 18, 2016

jancborchardt commented Nov 18, 2016

juliushaertl commented Nov 18, 2016

juliushaertl commented Aug 11, 2016 •

edited

Loading