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

generate user themed icons #35182

Merged
merged 1 commit into from
Nov 18, 2022
Merged

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented Nov 15, 2022

Close #34654

Signed-off-by: Simon L szaimen@e.mail.de

For my own testing
docker run -it \
--name nextcloud-easy-test \
-p 8443:443 \
-e TRUSTED_DOMAIN=192.168.24.128 \
-e SERVER_BRANCH=enh/34654/try-go-generate-user-themed-icons \
--volume="nextcloud_easy_test_npm_cache_volume:/var/www/.npm" \
ghcr.io/szaimen/nextcloud-easy-test:latest

@szaimen szaimen added the 2. developing Work in progress label Nov 15, 2022
@szaimen szaimen added this to the Nextcloud 26 milestone Nov 15, 2022
apps/theming/lib/ThemingDefaults.php Fixed Show fixed Hide fixed
apps/theming/lib/ThemingDefaults.php Fixed Show fixed Hide fixed
apps/theming/lib/ThemingDefaults.php Fixed Show fixed Hide fixed
apps/theming/lib/ThemingDefaults.php Fixed Show fixed Hide fixed
apps/theming/lib/Service/JSDataService.php Fixed Show fixed Hide fixed
apps/theming/lib/ThemingDefaults.php Fixed Show fixed Hide fixed
apps/theming/lib/Service/JSDataService.php Fixed Show fixed Hide fixed
apps/theming/lib/Service/JSDataService.php Fixed Show fixed Hide fixed
apps/theming/lib/ThemingDefaults.php Fixed Show fixed Hide fixed
apps/theming/lib/ThemingDefaults.php Fixed Show fixed Hide fixed
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

To simplify the code, I would put a getCacheBuster method in
https://github.com/nextcloud/server/blob/99191167167dd6c98dc3ae9b0eca947526e7939f/apps/theming/lib/Util.php

And use it from here.
You can then benefit from the dependency injection of the Util class.

@szaimen
Copy link
Contributor Author

szaimen commented Nov 15, 2022

Just tested again and there seem to be some issue with the admin disabling user theming. So this needs to be verified as well...

apps/theming/lib/ThemingDefaults.php Outdated Show resolved Hide resolved
apps/theming/lib/ThemingDefaults.php Outdated Show resolved Hide resolved
apps/theming/lib/Controller/IconController.php Outdated Show resolved Hide resolved
@szaimen szaimen force-pushed the enh/34654/try-go-generate-user-themed-icons branch 3 times, most recently from 4db5142 to 2ccde00 Compare November 15, 2022 23:12
apps/theming/lib/Service/JSDataService.php Fixed Show fixed Hide fixed
apps/theming/lib/Service/JSDataService.php Fixed Show fixed Hide fixed
apps/theming/lib/ThemingDefaults.php Fixed Show fixed Hide fixed
lib/private/legacy/OC_Util.php Fixed Show fixed Hide fixed
lib/private/legacy/OC_Util.php Fixed Show fixed Hide fixed
@szaimen szaimen force-pushed the enh/34654/try-go-generate-user-themed-icons branch from 2ccde00 to 4680a29 Compare November 15, 2022 23:23
apps/theming/lib/Util.php Fixed Show fixed Hide fixed
apps/theming/lib/Util.php Fixed Show fixed Hide fixed
apps/theming/lib/Util.php Fixed Show fixed Hide fixed
apps/theming/lib/Util.php Fixed Show fixed Hide fixed
apps/theming/lib/Util.php Fixed Show fixed Hide fixed
@szaimen szaimen force-pushed the enh/34654/try-go-generate-user-themed-icons branch from bdf6071 to 3762bcf Compare November 15, 2022 23:37
@szaimen szaimen changed the title WIP: try to generate user themed icons generate user themed icons Nov 15, 2022
@szaimen szaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 15, 2022
@szaimen
Copy link
Contributor Author

szaimen commented Nov 15, 2022

This works now and is ready for re-review :)

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Please also directly integrate the system cachebuster into the Util

$cacheBuster = $this->config->getAppValue('theming', 'cachebuster', '0');
if ($this->userId !== null) {
// need to bust the cache for the CSS file when the user background changed as its
// URL is served in those files
$userCacheBuster = $this->config->getUserValue($this->userId, Application::APP_ID, 'userCacheBuster', '0');
$cacheBuster .= $this->userId . '_' . $userCacheBuster;
}

That way we can standardise and use $this->util->getCacheBuster() in ThemeInjectionService too

#34654 (comment)

@szaimen szaimen force-pushed the enh/34654/try-go-generate-user-themed-icons branch from 5886533 to 756ee39 Compare November 17, 2022 10:26
apps/theming/lib/Util.php Fixed Show fixed Hide fixed
apps/theming/lib/Util.php Fixed Show fixed Hide fixed
@szaimen szaimen force-pushed the enh/34654/try-go-generate-user-themed-icons branch from 756ee39 to 5512b8b Compare November 17, 2022 10:38
apps/theming/lib/Util.php Fixed Show fixed Hide fixed
@szaimen szaimen force-pushed the enh/34654/try-go-generate-user-themed-icons branch 3 times, most recently from 8d5c994 to b44cef3 Compare November 17, 2022 11:20
@szaimen
Copy link
Contributor Author

szaimen commented Nov 17, 2022

@skjnldsv @come-nc do you know how I can fix these tests? I don't get it :/
image

apps/theming/lib/Util.php Outdated Show resolved Hide resolved
apps/theming/lib/Util.php Outdated Show resolved Hide resolved
@szaimen szaimen force-pushed the enh/34654/try-go-generate-user-themed-icons branch from a24eb65 to 2abad31 Compare November 17, 2022 15:31
@szaimen
Copy link
Contributor Author

szaimen commented Nov 17, 2022

@skjnldsv @come-nc the same nodb test is still failing. Can someone please help me to fix it? Is maybe a mock missing or something? See
image

@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 Nov 18, 2022
Signed-off-by: Simon L <szaimen@e.mail.de>
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
Co-Authored-By: Côme Chilliet <91878298+come-nc@users.noreply.github.com>
@szaimen szaimen force-pushed the enh/34654/try-go-generate-user-themed-icons branch from 87aadfc to 04b236e Compare November 18, 2022 14:03
@szaimen szaimen merged commit 8e80f04 into master Nov 18, 2022
@szaimen szaimen deleted the enh/34654/try-go-generate-user-themed-icons branch November 18, 2022 15:02
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 bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NC 25 - Theme-Colored Folder-Icons
5 participants