Skip to content

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented May 10, 2022

Implement a legacy compatibility layer for the svg icons colours

Refs: #32072 #32261

@skjnldsv skjnldsv added this to the Nextcloud 25 milestone May 10, 2022
@skjnldsv skjnldsv requested review from a team, artonge, nickvergessen and szaimen May 10, 2022 15:03
@skjnldsv skjnldsv self-assigned this May 10, 2022
@skjnldsv skjnldsv requested review from vanpertsch and removed request for a team May 10, 2022 15:03
@skjnldsv skjnldsv force-pushed the fix/icons-cacher branch 2 times, most recently from e9a3e53 to 201e0c1 Compare May 10, 2022 15:21
@skjnldsv
Copy link
Member Author

@CarlSchwan that should drastically remove some load as the svgController was getting pinged a lot for each page load! 🚀

@skjnldsv skjnldsv force-pushed the fix/icons-cacher branch 2 times, most recently from 057441d to 3eee3c4 Compare May 10, 2022 15:41
@CarlSchwan
Copy link
Member

CarlSchwan commented May 10, 2022

@CarlSchwan that should drastically remove some load as the svgController was getting pinged a lot for each page load! rocket

I'm not sure how the SvgController works. It seems that before and after this PR there was zero network request against it even after a browser full refresh.

Still, I can see significantly fewer DB queries when loading the cache and the scsscacher is cold, from 375 queries to 210 queries.

Edit: I figured out how the iconcacher works, it basically inject the icons directly inside the CSS by using file_get_contents and then encoding it as base64. So that's why I didn't saw it in my apache logs.

@skjnldsv
Copy link
Member Author

Edit: I figured out how the iconcacher works, it basically inject the icons directly inside the CSS by using file_get_contents and then encoding it as base64. So that's why I didn't saw it in my apache logs.

Ah right, we also did that, not always serving those via the svg Controller 🤔
I think the svg controller is only used on specific cases, like when you enable the custom dark theme or change some theming values?
We moved the implementation a it, i' don't recall all the usages 🙈

@skjnldsv skjnldsv force-pushed the fix/icons-cacher branch from 3eee3c4 to e3364f1 Compare May 10, 2022 21:01
@skjnldsv skjnldsv requested a review from CarlSchwan May 10, 2022 21:01
@skjnldsv skjnldsv force-pushed the fix/icons-cacher branch from e3364f1 to 0c5ffe2 Compare May 10, 2022 21:24
@skjnldsv
Copy link
Member Author

Yeah, enable dark theme and see the madness 😝
image

@skjnldsv
Copy link
Member Author

Tested a bit, one svg request is 10 DB queries.
Loading files with minimum content is at least 17 svg requests, so it's 170 DB requests saved (most likely much more) 🚀

Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv skjnldsv force-pushed the fix/icons-cacher branch from 0c5ffe2 to cb73fe2 Compare May 11, 2022 06:35
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

YOLO

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 11, 2022
@skjnldsv skjnldsv merged commit 5a0b28d into master May 11, 2022
@skjnldsv skjnldsv deleted the fix/icons-cacher branch May 11, 2022 07:29
@skjnldsv skjnldsv mentioned this pull request May 11, 2022
46 tasks
@marekful
Copy link

Hey @skjnldsv

I'm not sure how the SvgController works.

The Svg color api was a way to fetch internal icons (like those in the notifications list) and installed apps' icons in SVG format and optionally in a specified colour. I'm not sure if this API was ever used internally by Nextcloud server or other components (e.g. in relation to themes?). As you can see it's still mentioned in the latest developer guide.

Because this API was public prior to being removed and might have had 3rd party integrations relying on it, it would be nice to offer an alternative if there isn't one already.

Can you suggest a way to fetch said icons in SVG format (with or without specific colour) or know someone who could?

A few example request URIs of old API:

/svg/notifications/notifications-dark?color=d8dee9
/svg/passwords/app-dark?color=d8dee9
/svg/core/places/calendar?color=d8dee9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement performance 🚀 technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants