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

Thumbnail for upload #746

Merged
merged 19 commits into from Jan 14, 2015
Merged

Thumbnail for upload #746

merged 19 commits into from Jan 14, 2015

Conversation

tobiasKaminsky
Copy link
Contributor

See #595

@tobiasKaminsky
Copy link
Contributor Author

@davivel There is one problem left:
The thumbnails are generated and stored to cache but the imageView is not being updated.
I know it has something to do with the weakReference, but unfortunately I do not have a clue right now how to solve it...

@tobiasKaminsky
Copy link
Contributor Author

@davivel Bug is fixed.
If you want, please review it.

@tobiasKaminsky
Copy link
Contributor Author

@davivel
Currently I am using
for local files: String.valueOf(mFile.hashCode());
for remote files: String.valueOf(mFile.getRemoteId());

This means if a local file (as File) is listed a different thumbnail is created as if the same file (as OCFile) is shown in owncloud.
What about generating a unique id based on the file that is the same on server and on device?
Maybe md5sum as this is should be available on all platforms?
As we use the same cache for local and remote files there will be no new generation of a thumbnail once the local file is being uploaded to the server.

@@ -184,4 +222,12 @@ private int compareNames(File lhs, File rhs) {
}
notifyDataSetChanged();
}

private boolean isImage(File file) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, move this method to BitmapUtils as a static.

@davivel
Copy link
Contributor

davivel commented Dec 18, 2014

The idea of MD5 makes sense, but we will have to consider the impact on performance. I would do it in a separate branch.

This code needs some refactoring; there are new methods in ThumbnailsCacheManager almost dupicating exising ones, but for the File Vs. OCFile parameter. We should rewrite it to reuse common code in private methods.

@rperezb rperezb added this to the 2014-sprint-06-current milestone Dec 19, 2014
@jabarros
Copy link
Contributor

Hi @tobiasKaminsky
Just for your know. We are going to start the refactoring in our side in order to prepare it for being validated.

@jabarros
Copy link
Contributor

@purigarcia Ready to be validated

@@ -234,7 +233,9 @@ private static String getExtension(String filename) {
*/
public static String unixTimeToHumanReadable(long milliseconds) {
Date date = new Date(milliseconds);
return date.toLocaleString();
DateFormat df = DateFormat.getDateTimeInstance();
//return date.toLocaleString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Self-note: remove commented code

return null;
}

public static class AsyncGlobalDrawable extends BitmapDrawable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, the Global part is not providing any real meaning.

thumbnail = getBitmapFromDiskCache(imageKey);

// Not found in disk cache
if (thumbnail == null || ((OCFile)mFile).needsUpdateThumbnail()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, define a local OCFile variable at the beginning of the method and set it with (OCFile)mFile; castings are not free.

@davivel
Copy link
Contributor

davivel commented Jan 12, 2015

OK, ready again.

Please, @purigarcia , make a quick retest.

davivel added a commit that referenced this pull request Jan 14, 2015
Thumbnails added to Upload activity
@davivel davivel merged commit 9bf4e0f into develop Jan 14, 2015
@davivel davivel deleted the thumbnailForUpload branch January 14, 2015 07:25
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.

None yet

4 participants