-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
core/ajax/preview.php to controller #18675
Conversation
Endpoint is now a nice controller. * Added unit tests * Not yet possible to force icon returned
A new inspection was created. |
Great initiative :) Maybe move this to the app domain (of core) instead of keeping it into core since making Preview an app is on @georgehrke's roadmap for 8.2? |
forceIcon defaults to true, so we have to check every place a preview url is called ...
No, was outsourcing previews to an app discussed somewhere or is there an issue for that? |
@georgehrke #13610 |
In this issue I meant moving it to a controller and getting rid of the ajax end points (just as this pr does). |
OK, but why not keep all appframework based stuff in the apps folder? /core contains old ajax endpoints, assets, templates. It doesn't seem right to add Preview there. |
avatar stuff is also in /core |
@rullzer Yeah, one controller. That's why I'm thinking it wouldn't be a bad thing to start the trend, but the project's architects have to voice their opinion. |
Because the preview api is an api offered by ownCloud's core. If we'd move it to an app, we would have apps depending on other apps. That's something we try to avoid. |
I'm still not convinced because the provisioning API is an app and the public sharing API is also in an app. So we end up with APIs being scattered around. |
@oparoz good point. This also reminds me that previews can be disabled in the config. But it also means that the previews app knows how to inject previews in various apps like files_versions, sidebar, etc. Or alternatively have other apps check if there is a preview provider and get a preview from there. This sounds like a bigger change that needs to be thought through. |
@PVince81 with previews that would be "simple" since the dependency is via GET request. so if it is not there it will return a 404... just like the PR does when there is no preview :P |
@PVince81 - I agree that this might require a bigger discussion, but iirc, the end goal is to make previews completely optional, for @LukasReschke reasons, and there is a fallback mechanism with the media type icons, so user can always kind of see of which type each file is. They just don't get the previews. |
Well, but moving it to a dedicated app would create some really messed up dependencies.
Some providers, but not all. We want to outsource some of them / make the preview system extendable, because it would be insane to have preview providers for everything in core. (like some file formats used in hospitals or whatsoever) |
Maybe we're talking about 2 different things? lib/private/preview.php stays where it is. That's what apps are using when they're injecting the dependencies. Gallery currently does that. The Preview app/API would use the same class, but provide REST endpoints for apps which need them. Like Gallery at some point. That's the app created in this PR and it can sit anywhere, not necessarily in core. Other apps connect and either get a preview or a media type icon. Or am I still missing something? |
* @param int $y The max Y | ||
* @param bool $scalingUp | ||
* @param bool $a Keep aspect ratio | ||
* @param bool $forceIcon |
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.
not part of the method
Previews will be part of the next Generation webdav endpoint in 9.0 Kind of mit neccesary to move to a controller now. |
@@ -88,6 +89,14 @@ public function __construct(array $urlParams=array()){ | |||
$c->query('UserFolder') | |||
); | |||
}); | |||
$container->registerService('PreviewController', function(SimpleContainer $c) { |
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.
How reusable is this?
Creating the endpoint in the trash bin app should just be a matter of replacing the folder that's being handed over.
OK :) |
Ok lets close this then and keep previews the way it is. |
@deepdiver - This PR brings needed features like proper return codes and aspect ratio support. On top of that it's fully tested. Would it be that bad to bring this in for 8.2? |
Yet another ajax endpoint that can go. And thus functionality we can actually test!
At the moment I dropped the forceIcon argument. Since it makes the code ugly. And we should not serve tiny png files anyway. Does anybody know if the forceIcon is actually used? (the files app already sets it to 0).
Unit tests are added and coverage is at 100%.
CC: @PVince81 @nickvergessen @Xenopathic @MorrisJobke