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

Add Support for more ImageProvider (like WikimediaCommons) #44

Closed
wants to merge 15 commits into from

Conversation

newhinton
Copy link
Contributor

@newhinton newhinton commented Jan 25, 2019

Created Provider Structure

Signed-off-by: fnuesse felix.nuesse@t-online.de

Closes #40
Closes #5
And in Combination with #43
Closes #33

Signed-off-by: fnuesse <felix.nuesse@t-online.de>

Created Provider Structure

Signed-off-by: fnuesse <felix.nuesse@t-online.de>

Created Provider Structure

Signed-off-by: fnuesse <felix.nuesse@t-online.de>
Signed-off-by: fnuesse <felix.nuesse@t-online.de>
…em was preselected

Signed-off-by: fnuesse <felix.nuesse@t-online.de>
Signed-off-by: fnuesse <felix.nuesse@t-online.de>
Signed-off-by: fnuesse <felix.nuesse@t-online.de>
Signed-off-by: fnuesse <felix.nuesse@t-online.de>
Signed-off-by: fnuesse <felix.nuesse@t-online.de>
Signed-off-by: fnuesse <felix.nuesse@t-online.de>
@newhinton
Copy link
Contributor Author

@jancborchardt @marius-wieschollek what do you think about this?

@newhinton
Copy link
Contributor Author

This is still missing some things: i want to be able to give the admin the ability to choose the categories, and if @marius-wieschollek allows me to 'steal' his code from the unsplash image info thingy, i want to include that if the provider supports it.

Also the wikimedia api is quiet finnicky, so for now only dogs or cats :D :D

@newhinton newhinton changed the title Created Provider Structure Add Support for more ImageProvider (like WikimediaCommons) Jan 26, 2019
@dartcafe
Copy link

This is great. Worked on a quick look without problems. Why not releasing it and adding more options later. The most importent provider after that would be an own nextcloud folder as provider.

@newhinton newhinton removed the WIP label Mar 14, 2019
@newhinton
Copy link
Contributor Author

@dartcafe Shouldn't be that hard, but that will be another update ;)

@dartcafe
Copy link

@dartcafe Shouldn't be that hard, but that will be another update ;)

That's what i meant. Fire this up and wait for responses. As far as I saw it works.

@newhinton
Copy link
Contributor Author

@jancborchardt any comments on my pr's ? :D

@dartcafe
Copy link

@jancborchardt @newhinton Any progress on this PR?

@newhinton
Copy link
Contributor Author

@dartcafe We want to transfer this app to the nextcloud-namespace, which will change some stuff. That may take some additional time

@e-alfred
Copy link

@newhinton Anything that can be done to get this merged and released to the App Store?

@newhinton
Copy link
Contributor Author

newhinton commented Oct 24, 2019

@e-alfred My goal is to release this together with #42
That needs some testing. Also it's not working so good, i dont quiet understand the api.
If you are able to help, or know a guy, that would be awesome!

Also, if you want to extend this with additional providers, you can add the proper file to a pull request :D

@newhinton
Copy link
Contributor Author

@marius-wieschollek What do you think about this approach of adding more providers? Do you think this is reasonable, or do you prefer a different way of allowing more providers to this app?

I'd like to release this in the near future, so any input is appreciated!

@marius-wieschollek
Copy link
Contributor

Oje, i've now put this off for nearly two years...

There are many things here that i think are good base to start with and build upon.
There are some things that i would like to have added, like an interface or service that can be used by other apps (like Passwords). I would also like to make it possible for other apps to add their own providers. But that can be added later.

There are two major issues i have with this approach.
1. The css files now contain PHP
What's good? This is a convenient way to get the variable parts that providers bring with them done.
What's bad? It also means that we generate the same css over and over again. For something that is probably changed less often than i review PRs, i don't think this is a good solution.
Just as a reference: the current login.css is delivered by my old and slow AMD Athlon 5350 in 6 milliseconds. Doing the same with PHP lets the request time skyrocket to 1,16 seconds (nearly 200 times as much). And with a static file, the second request can be delivered from the browser cache and takes neither bandwidth nor measurable time to complete. This solution does currently not provide caching and if we want to do so, we need to implement it ourselves instead of leaving this to the webserver.
Around 10% of the userbase (of my app) uses even slower hardware like a RaspberryPi. They're going to get even worse results.
Since we're making an app that "only" makes cosmetic changes to the UI, i think we should keep in mind that this should have as little performance hit as possible.
If the stylesheet has noticeable delays, this also means that users might get more "pop-in" where the image loads long after everything else has finished.
Alternatives? With the BeforeTemplateRenderedEvent provided by Nextcloud we can add the complete static style, a static JS and the dynamic configuration to the actual page template. The JS could then apply the options defined in the config on the client side. CSS-Variables are a good way to make the urls and possibly also colors in the css configurable.

2. Two custom entrypoints (login.php & header.php)

  • No offense: That's just horrible.
  • It's not going to work on any setup that uses the configuration example for Nginx that the documentation provided up to NC 19. (including mine)
  • It also won't work with any NC where Splash is not installed in the apps folder because the URL is hardcoded. Which is any NC using the official docker image. (and mine)
  • It might come with high maintenance. As i see it, we're basically instantiating parts of Nextcloud ourselves. That's bound to break. If we want to be able to support this solution in the long run, low maintenance should be important to us.
  • It's probably insecure. I can't prove that, but these two files somehow just scream "exploit me".

@newhinton
Copy link
Contributor Author

newhinton commented Jan 19, 2021

Thanks for taking a look!

To your points:
Security: I dont think this is an issue. Only an serveradmin can influence the values introduced by php, and therefore he can already inject stuff way easier (jsloader, customcss)

load-times: You are right. This can be an issue, and while we already have pop-ins because we load stuff externally, this would further worsen it.

To the rest: Since i dont use other methods of running nextcloud, i cant really judge that.

The Takeaway:

One idea i had was not serving any "external" content at all. (I should get rid of "custom" css) Maybe it is easier to fetch the source in the backend and then deliver it via fixed endpoints from unsplash itself. This has some downsides on it's own, but is probably more stable. Serving prebuild css files is not really an option, since i aim to provide more customization than before, and that is not possible with prebuild css.

Even then, since the imagecontent is dynamically fetched, the loadtimes will take a penalty if we allow any form of customization.

@newhinton newhinton added the WIP label Jan 19, 2021
newhinton added 6 commits May 1, 2022 12:30
…ercapsule

# Conflicts:
#	css/header.css
#	css/login.css
#	js/settings.js
#	lib/AppInfo/Application.php
#	lib/Controller/AdminSettingsController.php
#	lib/Services/SettingsService.php
#	lib/Settings/AdminSettings.php
#	templates/settings/admin.php
@newhinton newhinton closed this May 1, 2022
@newhinton
Copy link
Contributor Author

This has been superseeded by the dev-branch. I started out proxying the images, but now i serve css code with the urls directly as an api-endpoint, more or less identical to the custom-css app.

@jancborchardt jancborchardt deleted the feature/noid/providercapsule branch August 20, 2024 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants