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

adds thumbnail images for image files #11

Merged
merged 3 commits into from
Mar 7, 2017
Merged

adds thumbnail images for image files #11

merged 3 commits into from
Mar 7, 2017

Conversation

csoni111
Copy link
Member

@csoni111 csoni111 commented Mar 1, 2017

This pr aims to implement feature #2191. Check the screenshot below:

thumbnailimages

@cpg
Copy link
Member

cpg commented Mar 1, 2017

It works well! Great job!

My only complaint is that it's a little inefficient in that it loads the pictures once. As the preview advances, the whole picture is loaded and displayed in the thumbnail. Then when the user clicks on a picture, it loads the picture again. This is not a big problem locally in a LAN, but it matters over cell data.

There are two things that could help with this:

  1. If the picture could be transparently cached as it's loaded first, and the app could get it straight from the thumbnail cache, it may just solve the issue
  2. If the app and the server were to be using best practices for web protocols, we could be using If-Not-Match (Etag), If-Modified-Since, etc. so that a new request would return a 304 Not Modified code and the re-download of the whole image (megabytes, these days) would be avoided.

Looking a bit more on our server (a beefed up version of this server), I believe it respects If-None-Match/ETag headers, but not If-Modified-Since. We probably should implement those, however!

Would you research this a bit more to see if there is some setting that could affect the app to take advantage of If-None-Match/Etag?

@csoni111
Copy link
Member Author

csoni111 commented Mar 1, 2017

I digged deeper into the docs of picasso and found that whenever we create a new instance of picasso, it creates following:

The instance is automatically initialized with defaults that are suitable to most implementations.

LRU memory cache of 15% the available application RAM
Disk cache of 2% storage space up to 50MB but no less than 5MB. (Note: this is only available on API 14+ or if you are using a standalone library that provides a disk cache on all API levels like OkHttp)
Three download threads for disk and network access.

Also in this stackoverflow answer by Jake Wharton (the developer of picasso) , he says that the ETag and conditonal requests are handled automatically by picasso.

So in short picasso along with OkHttp handles all the caching and other things automatically for us.

Moreover if required we can customize the size of the memory cache using Picasso.Builder(). Also we can provide a custom method to check Etag or If-Modified-Since headers by making a new HttpOkClient and providing it to picasso as in this answer.

So finally what should I do?

@cpg
Copy link
Member

cpg commented Mar 2, 2017

I have to whip up a local server to test Etag. I am fairly sure we tested that feature in the server.

@csoni111
Copy link
Member Author

csoni111 commented Mar 5, 2017

Similar to the issue #18 picasso loads outdated image from the cache without actually fetching from the server in one of the use case.
Consider an image with filename example.jpg on a server share, let it be fetched from the server once. Now on updating the image content on the server but keeping the filename same, picasso (checking that the url is same as previous) will loads the image from the memory cache instead of actually sending the request. This will result in new content not getting updated in the app.

@csoni111
Copy link
Member Author

csoni111 commented Mar 5, 2017

As discussed with @cpg, a simple solution would be to add mTime (last modified time) param to the url of the image to be fetched from server which will change if the image content has changed. Hence changing the url.

@cpg
Copy link
Member

cpg commented Mar 5, 2017

We did found one path where the If-None-Match/Etag was not generated properly in the back-end and we fixed it, and released it.

@cpg
Copy link
Member

cpg commented Mar 6, 2017

Please do add the change for cache-busting with mtime (all in small caps, if possible), and the removal of the okhttp caching.

@csoni111 csoni111 force-pushed the master branch 2 times, most recently from fb65276 to 358e4ea Compare March 6, 2017 12:29
@csoni111
Copy link
Member Author

csoni111 commented Mar 6, 2017

@cpg done

@cpg cpg merged commit 358e4ea into amahi:master Mar 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants