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

Use public preview api to retrieve thumbnails #3017

Merged
merged 5 commits into from
Nov 25, 2020

Conversation

abelgardep
Copy link
Contributor

@abelgardep abelgardep commented Nov 12, 2020

Implements #2926 and #2927


QA

Bugs & improvements:

@abelgardep abelgardep linked an issue Nov 12, 2020 that may be closed by this pull request
@abelgardep abelgardep self-assigned this Nov 12, 2020
@abelgardep abelgardep marked this pull request as ready for review November 13, 2020 08:39
@abelgardep abelgardep linked an issue Nov 13, 2020 that may be closed by this pull request
@abelgardep abelgardep force-pushed the feature/use_public_preview_api branch from 7b871e4 to 7204896 Compare November 17, 2020 08:55
@jesmrec
Copy link
Collaborator

jesmrec commented Nov 18, 2020

Let's QA this one

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 18, 2020

(1)

I've checked out the requests and they have the following form:

/remote.php/dav/files/user1/ownCloud%20Manual.pdf?x=336&y=336&c=&preview=1

  • c parameter is empty. It use to contain the etag. Is that intended somehow?
  • I lack of an API doc, so i checked the iOS client who actually implements that API. This is how an iOS request looks like:

Archive.zip?y=120&preview=1&x=120&c=%22483f40d8166bcebaab085ea9cca57a79%22&a=1

it contains also an a parameter. By checking the iOS code:

request.parameters = [NSMutableDictionary dictionaryWithObjectsAndKeys:
				@(size.width).stringValue, 	@"x",
				@(size.height).stringValue,	@"y",
				item.eTag, 			@"c",
				@"1",				@"a", // Request resize respecting aspect ratio 
				@"1", 				@"preview",
			nil];

should we include also the a to keep a correct ratio?

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 23, 2020

(2) [DONE]

@abelgardep as you commented me, this part of the code:

if (!updatedLocalFile.isFolder() &&
                        remoteFile.isImage() &&
                        remoteFile.getModificationTimestamp() != localFile.getModificationTimestamp()) {
                    updatedLocalFile.setNeedsUpdateThumbnail(true);

should be changed, since not only images contain a thumbnail.

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 23, 2020

(3) [WONT FIX]

  1. In an account, sync a folder that contains an txt file. Thumbnail is shown
  2. In server, modify the txt file including some different text on the top (where the thumbnail pic is rendered)
  3. Sync the folder / download the new version of the file

Current: Thumbnail does not change
Expected: Thumbnail change to show the new content

Same behaviour works fine with images

Pixel 2 Android 11
b34f119

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 23, 2020

(1) -> etag is sent fro mthe moment it is stored in the DB.

@abelgardep abelgardep force-pushed the feature/use_public_preview_api branch from b34f119 to 4ec916e Compare November 23, 2020 14:07
@abelgardep
Copy link
Contributor Author

I think that (2) and (3) refers to the same problem and should be fixed now. We will ask for new thumbnails when something changes in the server even if it is not an image (Previous behaviour).

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 24, 2020

About (2) and (3)

Thumbnails of txt files are correctly updated only if the file is not downloaded, and with a browsing back and forth. That means, with steps:

  1. txt is not downloaded in the app
  2. in web or other client, modify the txt
  3. in the app, pull down to refresh the containing folder

Result: thumbnail not refreshed

  1. in the app, browse to other folder and browse back to the containing folder

Result: thumbnail is now refreshed

  1. tap on the txt file to download
  2. close the file, so you are again in the list of files
  3. in web or other client, modify the txt
  4. in the app pull down to refresh the containing folder

Result: thumbnail not refreshed

  1. browse to another folder and then browse back to the containing folder

Result: thumbnail not refreshed

Expected: refresh should realise on the new version. Downloaded files must refresh also the thumbnail

NOTE:

Updating an non-downloaded image, makes the thumbnail to upload with only a refresh. If the image is downloaded, it runs into another issue (#3021). The current report is oriented to txt files behaviour.

Pixel 2 Android 11
4ec916e

@abelgardep
Copy link
Contributor Author

That's the behaviour since the beginning. If the file is available locally, we extract the thumbnail from the local file. The problem here is that the local thumbnail extraction seems not to work with txt files. So, the thumbnail is not updated.

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 24, 2020

So, we have two different issues regarding previews:

  • For txt files. We can not extract thumbnails from downloaded files.

  • For images. If a downloaded image changes, thumbnail is not updated and you see the preview of the old version.

Any solution for this? getting rid of the local thumbnails and use only remote thumbnails, even when the file is locally downloaded? it will increase the number of request, but is any case we are going to request preview for every file. This will be addressed in a different ticket, for sure.

Which are the expectations @michaelstingl?

@abelgardep
Copy link
Contributor Author

it will increase the number of request, but is any case we are going to request preview for every file

We only request thumbnails the first time and then if something changes in the server, but not always. If file is available locally we don't request for new thumbnails at the moment.

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 24, 2020

We only request thumbnails the first time and then if something changes in the server, but not always. If file is available locally we don't request for new thumbnails at the moment.

yes, for this reason, a posible solution would be requesting always the remote thumbnails, so that the preview is always updated. But first, we have to know the expectations for this feature.

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 25, 2020

After discussions and calls, the final conclusion is that thumbnail and preview updates will need more elaboration. It will be moved to another issue (#3024):

Goals of the current PR:

  • Replace the old endpoint with the new Preview API
  • Request for every file. Pending: request also for downloaded. This will be moved to the new issue.

@abelgardep abelgardep force-pushed the feature/use_public_preview_api branch from 4ec916e to 79c3d73 Compare November 25, 2020 16:14
@abelgardep abelgardep merged commit 43fcae1 into master Nov 25, 2020
@abelgardep abelgardep deleted the feature/use_public_preview_api branch November 25, 2020 17:11
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.

Request previews for EVERY file type Use public preview API
3 participants