-
Notifications
You must be signed in to change notification settings - Fork 23
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
Feature/noid/imagecaching #133
Conversation
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
# Conflicts: # appinfo/info.xml
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... |
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! |
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... |
Hello, thanks for the work, I tested and it works for the login page but not when you are logged in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR! unsplash currently prevents me from updating to nc28, so this PR is very welcomed. I just request a few tiny changes, in order to give you some feedback and make this ready for merge. Feel free to challenge some of my review points, since I am not maintainer of this project.
Unfortunately I shouldn't review your javascript files and I still need to
- test this in production
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 |
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
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! |
@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 👍 Edit: Sorry for the screenshot being german, but I couldn't be bothered |
Also I managed to produce some exceptions with the UnsplashAPI provider (I didn't provide a valid token)
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 |
So there is no issue at all and that part work for you now?
Good catch!
How bad is it, do you have some numbers? |
No there isn't, this was just a configuration issue!
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? |
It should be clickable, do you have javascript disabled? Could also be a cache problem, can you reload with caching disabled? |
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
fix wrong reference Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Love to see the progress on this, came back to this because v29 was released. |
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! |
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 |
Thank you ! I read the doc and I'll try that. |
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" At my very small level, it looks like a go :-) |
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
When will the next release occur? I saw issues were fixed but last release has been awhile back. |
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. |
Oh, and i need to add some setup-documentation for the new unsplash api token. |
Got it working on 29! Maybe rename token to "access key" to match Unsplashed naming, i was using secret key and it failed. NB: code has Title case L in login - core.login.showLoginForm: This was causing error: Call to undefined method OCP\AppFramework\Http\Events\BeforeLoginTemplateRenderedEvent::isLoggedIn() 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. |
@MiWCryptAnalytics can you create a PR for nextcloud 29? Also I described the issue you found for NC29 in #139 |
Hi What's the norma release flow of nextcloud? When can we expect an "official" update to the nextcloud store? |
closes #123
closes #131
implements #115
implements #2
closes #7
closes #9