Feature/noid/imagecaching

[newhinton]

• Adds image caching to some providers
• Support for nextcloud 28

closes #123 closes #131 implements #115 implements #2 closes #7 closes #9

[EtienneHenger]

Is there an estimate as to when this will be merged into main? Asking as I am looking to update to NC28 and am wondering if it is worth the hassle to update this app via the terminal as opposed to waiting a bit and having it in the official app release...

[newhinton]

Generally speaking, this PR is feature-complete. However, since i wrote it over a couple of month, and it introduces some big changes, i want to do a full review before merging. That still might take a while. Also, any help is appreciated and will speed this up!

[EtienneHenger]

Thanks for the quick reply! I would be happy to help any way I can; so how can I help? Mind you I am a novice in PHP, JavaScript, and CSS...

[pikacchuu]

Hello, thanks for the work, I tested and it works for the login page but not when you are logged in. How can I help you?

[newhinton]

@mwinkens

Thank you for your review! Your work is very appreciated!

I will try to work through your notes later, the linting-idea is a good one, and i'll do something about it!

[mwinkens]

@mwinkens

Thank you for your review! Your work is very appreciated!

I will try to work through your notes later, the linting-idea is a good one, and i'll do something about it!

I also updated your description, feel free to change it accordingly! I noticed you mentioned the closed issues also in the commits

[newhinton]

I have not introduced an dedicated linter, however, i used my ide's tools to reformat the files. I am a bit hesitant to use a tool, because it seems most depend on some package manager like compose, and at the moment this app does not use one because it is rather simple, dependency-wise.

But i have worked through your helpful annotations and a lot of that was helpful! Thank you for making that effort!

[mwinkens]

Hello, thanks for the work, I tested and it works for the login page but not when you are logged in. How can I help you?

@newhinton I tested this PR now manually and I have the same problem. Normally you have a(n) (un)splash background when logged in, this is mostly visible in the dashboard. I can add screenshots if you want.

Otherwise this looks good, you still have a few conversations open (see above), I closed the ones that are resolved. Thank you for fixing the linting issues :+1:

Edit: @pikacchuu I found out, that this is an admin setting, you can enable/disable splash for the login and dashboard. Go To AdminSettings - Design (bottom).

Sorry for the screenshot being german, but I couldn't be bothered

[mwinkens]

Also I managed to produce some exceptions with the UnsplashAPI provider (I didn't provide a valid token)

{"reqId":"3PdFVjay1aLCnS1Ix60x","level":3,"time":"2024-03-18T11:46:52+00:00","remoteAddr":"127.0.0.1","user":"nextcloud27","app":"index","method":"GET","url":"/index.php/apps/unsplash/api/dashboard.css","message":"/appdata_oc9arpx6pmoc/unsplash/UnsplashAPI","userAgent":"Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:123.0) Gecko/20100101 Firefox/123.0","version":"28.0.1.1","exception":{"Exception":"OCP\\Files\\NotFoundException","Message":"/appdata_oc9arpx6pmoc/unsplash/UnsplashAPI","Code":0,"Trace":[{"file":"/var/www/nextcloud28/lib/private/Files/Node/LazyFolder.php","line":161,"function":"get","class":"OC\\Files\\Node\\Root","type":"->"},{"file":"/var/www/nextcloud28/lib/private/Files/AppData/AppData.php","line":132,"function":"get","class":"OC\\Files\\Node\\LazyFolder","type":"->"},{"file":"/home/marvin/workspace/unsplash/lib/ProviderHandler/Provider.php","line":236,"function":"getFolder","class":"OC\\Files\\AppData\\AppData","type":"->"},{"file":"/home/marvin/workspace/unsplash/lib/Provider/UnsplashAPI.php","line":51,"function":"getImageFolder","class":"OCA\\Unsplash\\ProviderHandler\\Provider","type":"->"},{"file":"/home/marvin/workspace/unsplash/lib/Provider/UnsplashAPI.php","line":46,"function":"getMetadata","class":"OCA\\Unsplash\\Provider\\UnsplashAPI","type":"->"},{"file":"/home/marvin/workspace/unsplash/lib/Services/SettingsService.php","line":263,"function":"getCachedImageURL","class":"OCA\\Unsplash\\Provider\\UnsplashAPI","type":"->"},{"file":"/home/marvin/workspace/unsplash/lib/Controller/CssController.php","line":114,"function":"headerbackgroundLink","class":"OCA\\Unsplash\\Services\\SettingsService","type":"->"},{"file":"/home/marvin/workspace/unsplash/lib/Controller/CssController.php","line":88,"function":"innerCSS","class":"OCA\\Unsplash\\Controller\\CssController","type":"->"},{"file":"/home/marvin/workspace/unsplash/lib/Controller/CssController.php","line":58,"function":"mediaQuery","class":"OCA\\Unsplash\\Controller\\CssController","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Http/Dispatcher.php","line":230,"function":"dashboard","class":"OCA\\Unsplash\\Controller\\CssController","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Http/Dispatcher.php","line":137,"function":"executeController","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/App.php","line":184,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/var/www/nextcloud28/lib/private/Route/Router.php","line":315,"function":"main","class":"OC\\AppFramework\\App","type":"::"},{"file":"/var/www/nextcloud28/lib/base.php","line":1069,"function":"match","class":"OC\\Route\\Router","type":"->"},{"file":"/var/www/nextcloud28/index.php","line":39,"function":"handleRequest","class":"OC","type":"::"}],"File":"/var/www/nextcloud28/lib/private/Files/Node/Root.php","Line":207,"message":"/appdata_oc9arpx6pmoc/unsplash/UnsplashAPI","exception":{},"CustomMessage":"/appdata_oc9arpx6pmoc/unsplash/UnsplashAPI"}}

The other providers seem to work fine, but also seem to fill the cache folder, which is funny because the WikiCommon provider fills the nature photos with cars

[newhinton]

@mwinkens

I found out, that this is an admin setting, you can enable/disable splash for the login and dashboard.

So there is no issue at all and that part work for you now?

Also I managed to produce some exceptions with the UnsplashAPI provider (I didn't provide a valid token)

Good catch!

The other providers seem to work fine, but also seem to fill the cache folder

How bad is it, do you have some numbers?

[mwinkens]

@newhinton

So there is no issue at all and that part work for you now?

No there isn't, this was just a configuration issue!

The other providers seem to work fine, but also seem to fill the cache folder

The number of images isn't the problem, but with different providers the image content changes. If I switch between providers, I fill the image cache with cars and nature photos. Maybe the image cache needs to be deleted on provider switch?

Otherwise this looks good! I noticed, that there is a card on the dashboard and login screen with image information, but unfortunately it isn't clickable, is this part of this PR?

[newhinton]

Otherwise this looks good! I noticed, that there is a card on the dashboard and login screen with image information, but unfortunately it isn't clickable, is this part of this PR?

It should be clickable, do you have javascript disabled? Could also be a cache problem, can you reload with caching disabled?

[JW-CH]

Love to see the progress on this, came back to this because v29 was released.

[mwinkens]

Otherwise this looks good! I noticed, that there is a card on the dashboard and login screen with image information, but unfortunately it isn't clickable, is this part of this PR?

It should be clickable, do you have javascript disabled? Could also be a cache problem, can you reload with caching disabled?

I disabled the cache and reloaded the page, but the problem persists. This button just redirects me from the dashboard to the dashboard

Imo this PR is good enough, I would merge and address issues in follow ups!

[CharlesDelorme]

I would like to help ? How can I install this on my NC28 ?

[mwinkens]

I would like to help ? How can I install this on my NC28 ?

clone PR into nextcloud apps directory under the unsplash directory, don't forget to change directory permission with chown, php occ app:enable unsplash

[CharlesDelorme]

I would like to help ? How can I install this on my NC28 ?

clone PR into nextcloud apps directory under the unsplash directory, don't forget to change directory permission with chown, php occ app:enable unsplash

Thank you ! I read the doc and I'll try that.

[CharlesDelorme]

Unsplash 3.0.0 installed on 28.0.6

(For those not familiar with Git like me, I downloaded https://github.com/nextcloud/unsplash/archive/refs/heads/feature/noid/imagecaching.zip, unzip it in /var/www/nextcloud/apps, renamed it to unsplash and "occ app:enable unsplash", thank to mwinkens)

Both login and dashboard work, tint included. "high viz" (I try to translate from french) options don't have an obvious effect and I didn't set a token. Changing keywords worked also. All sources (not tested unsplash api) work also.

Logs in nextcloud.log show several php error for filter_var "Constant FILTER_SANITIZE_STRING is deprecated at" /var/www/nextcloud/apps/unsplash/lib/Controller/AdminSettingsController.php#60 #62 #97

At my very small level, it looks like a go :-)

[asheroto]

When will the next release occur? I saw issues were fixed but last release has been awhile back.

[newhinton]

I need to update the version and write a changelog, theb it'll be released for 28. I dont think suppoert for 29 requires changes, but i need to validate that.

[newhinton]

Oh, and i need to add some setup-documentation for the new unsplash api token.

[MiWCryptAnalytics]

Got it working on 29!

Maybe rename token to "access key" to match Unsplashed naming, i was using secret key and it failed. It worked with the access key.

NB: code has Title case L in login - core.login.showLoginForm: lib/EventListener/BeforeTemplateRenderedEventListener.php:63 should be case 'core.login.showloginform':

This was causing error:

Call to undefined method OCP\AppFramework\Http\Events\BeforeLoginTemplateRenderedEvent::isLoggedIn() It was hitting the default case due to the switch case spelling, and because a BeforeLoginTemplateRenderedEvent does not defne isLoggedIn. App would HTTP/500 on the login screen. I tried handling this case specifically, but the root cause is the case spelling.

I might suggest not checking for isLoggedIn() for all cases, as BeforeLoginTemplateRenderedEvent does not provide this, and might fall through to default case for other BeforeLoginTemplateRenderedEvent event generating routes.

[mwinkens]

@MiWCryptAnalytics can you create a PR for nextcloud 29? Also I described the issue you found for NC29 in #139

[CrazyWolf13]

Hi Thanks for this PR.

What's the norma release flow of nextcloud? When can we expect an "official" update to the nextcloud store?