Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Oct 14, 2016

This PR improves the preview system:

  • Previews are stored in the AppData
    • Allows sharing of previews for shared data
    • Allows sharing of previews for external storage
  • Now always respects min/max preview size
  • Previews are stored in power of 2 sizes (should be more than enough)

Todo:

  • React on update/deletion of files (and fodlers for delete) (CC: @icewind1991)
  • Move all endpoints over
  • Move to controller
  • Test file access control
  • Tests!
  • Cleanup commits
  • Add cache
  • Remove old previews
  • Look into command to generate new previews

FYI @oparoz

@mention-bot
Copy link

@rullzer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @LukasReschke, @nickvergessen and @icewind1991 to be potential reviewers.

@@ -0,0 +1,338 @@
<?php

namespace OC;
Copy link
Member

Choose a reason for hiding this comment

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

Please namespace (we have a Preview namespace) then you can also get rid of the 2

header missing

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah well actually the old preview should go. But yeah lets move it.

/** @var IAppData */
private $appData;

public function __construct(
Copy link
Member

Choose a reason for hiding this comment

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

docs

@rullzer rullzer force-pushed the new_preview branch 2 times, most recently from 5c30d19 to adf6b5c Compare October 16, 2016 13:40
@rullzer
Copy link
Member Author

rullzer commented Oct 16, 2016

File_sharing (public previews) is moved over. Awesome side effect is that we now (thanks to the FileDisplayResponse) get preview client side caching for free!

@codecov-io
Copy link

codecov-io commented Oct 17, 2016

Current coverage is 58.03% (diff: 66.92%)

Merging #1741 into master will increase coverage by 0.27%

@@             master      #1741   diff @@
==========================================
  Files          1088       1092     +4   
  Lines         62156      62424   +268   
  Methods        6938       6978    +40   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          35901      36227   +326   
+ Misses        26255      26197    -58   
  Partials          0          0          
Diff Coverage File Path
0% apps/files_versions/lib/Storage.php
0% apps/files_sharing/appinfo/routes.php
0% apps/files_versions/appinfo/routes.php
0% new lib/private/Preview/GeneratorHelper.php
0% apps/files_trashbin/appinfo/routes.php
0% core/routes.php
•• 20% lib/private/PreviewManager.php
••••• 55% apps/files/lib/Controller/ApiController.php
•••••• 60% new .../files_versions/lib/Controller/PreviewController.php
•••••• 62% new ...s_sharing/lib/Controller/PublicPreviewController.php

Review all 17 files changed

Powered by Codecov. Last update b129adf...f44cd7a

@rullzer rullzer force-pushed the new_preview branch 3 times, most recently from f234df8 to d2dfa2b Compare October 18, 2016 09:48
@rullzer
Copy link
Member Author

rullzer commented Oct 18, 2016

Ok all is looking good from my POV.
I'm just not sure how to handle migration. Since there might be more (different) previews of the same image. And picking the right one can be tricky. Also moving everything could take a long time! (even on installations that don't have a dedicated cron job there could be a lot of previews).

Any ideas? @oparoz @MorrisJobke @nickvergessen ?

@icewind1991
Copy link
Member

Maybe don't move old previews over but allow reading the old preview when there is none in the new location, then when a file is changed delete the old ones while generating the new ones

@rullzer
Copy link
Member Author

rullzer commented Oct 18, 2016

@icewind1991 mmm could work. But that would still mean that currently wrong previews would still be displayed wrong (so for shared files etc)

@adsworth
Copy link

or just delete the previews in the old location when first generating a preview with the new logic. unlinking around 5-10 (I guess that's how many there might be) files shouldn't take to long.

Not migrating old preview would create extra load on big installs though, since all previews need to be regenerated.

@rullzer
Copy link
Member Author

rullzer commented Oct 19, 2016

Ok after some more thinking I think just deleting all previews and starting over is the easiest and cleanest way forward.

  • For a single file there might now be a few (different) previews lying around
  • Even small (sqlite) instances might have a shitload of previews so migrating them when there is no dedicated cron is going to time out.
  • Looking into two places is not very performant. Risks selecting the wrong preview. And at some point we have to get rid of the old preview folders anyways.

So I vote for just killing all preview folders. And have the previews be regenerated. In most cases this should be significantly less previews since the previews are now shared.

@oparoz
Copy link
Member

oparoz commented Oct 19, 2016

This will require some good communication. Last time this happened, everybody complained that Pictures/Gallery was slow, because the initial load would take minutes. Now at least we show something in the first few seconds, but I'm just saying that it won't be great for people will large photo collections.

Maybe it's time to resurrect the occ thumbnail generator. It can take hours for the process to complete, but it's better than having to wait when using the GUI imo.

@rullzer
Copy link
Member Author

rullzer commented Oct 21, 2016

@oparoz yes I agree. Good communication is key here. We have a nice list of improvements. But gettings this properly is key.

I would not mind the thumbnail generator resurrection. But I also have been thinking about a listner that injects new/edited files. So we can say generate those in the background. But that is a discussion for another time.

I'll see if the thumbnail generator is easy to resurrect. Because that seems like the fastest solution.

@rullzer
Copy link
Member Author

rullzer commented Oct 28, 2016

See https://github.com/rullzer/PreviewGenerator for my ultra simple cli preview generator. It isn't smart. Just looks over all files and if it can generate previews.

It doesn't do anything fancy. because of the caching previews if a preview exists it just returns the file.

@rullzer
Copy link
Member Author

rullzer commented Oct 28, 2016

Ok I will update the previewgenerator app with a command to also delete the old previews. That seems the most sensible solution IMO.

So review time.

@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 28, 2016
@MorrisJobke
Copy link
Member

Code looks good and also works 👍

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
* Only connect the watcher once the instance is properly setup else
AppData fails hard.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
* PreviewController test
* PublicPreview test
* Versions Preview test
* Trash Preview test

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@oparoz
Copy link
Member

oparoz commented Nov 3, 2016

Cool about the changelog. We'll have to test a few apps I guess and communicate heavily if things break so that people can fix their app for launch.

@rullzer
Copy link
Member Author

rullzer commented Nov 3, 2016

So 1 more thumbs up plz :)

@LukasReschke
Copy link
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants