Thumbnail support #533
Thumbnail support #533
Conversation
…ng thumbnail from the DiskLruCache
Also using cloud.id to be able to distinguish between two files with same path but on different instances of the same cloud
…tFanta01/cryptomator-android into feature/thumbnail-playground
WalkthroughThis update enhances Cryptomator's handling of media files, particularly focusing on image files by introducing thumbnail caching and management. It streamlines file operations like moving and deleting in the cache, and integrates thumbnail generation settings, improving user interaction and file management efficiency in cloud storage environments. Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
presentation/src/main/res/values/arrays.xmlis excluded by!**/*.xmlpresentation/src/main/res/values/strings.xmlis excluded by!**/*.xmlpresentation/src/main/res/xml/preferences.xmlis excluded by!**/*.xml
Files selected for processing (14)
- data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoFile.kt (2 hunks)
- data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt (7 hunks)
- data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormat7.kt (5 hunks)
- data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormatPre7.kt (3 hunks)
- presentation/src/main/java/org/cryptomator/presentation/model/CloudFileModel.kt (1 hunks)
- presentation/src/main/java/org/cryptomator/presentation/model/CloudNodeModel.kt (2 hunks)
- presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt (2 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/adapter/BrowseFilesAdapter.kt (4 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/bottomsheet/FileSettingsBottomSheet.kt (2 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt (2 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.kt (6 hunks)
- util/src/main/java/org/cryptomator/util/SharedPreferencesHandler.kt (2 hunks)
- util/src/main/java/org/cryptomator/util/ThumbnailsOption.kt (1 hunks)
- util/src/main/java/org/cryptomator/util/file/LruFileCacheUtil.kt (2 hunks)
Files skipped from review due to trivial changes (2)
- presentation/src/main/java/org/cryptomator/presentation/model/CloudNodeModel.kt
- util/src/main/java/org/cryptomator/util/ThumbnailsOption.kt
Additional comments not posted (22)
presentation/src/main/java/org/cryptomator/presentation/model/CloudFileModel.kt (1)
14-14: Addition ofthumbnailproperty to handle optional thumbnail files forCloudFileModellooks good.data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoFile.kt (1)
16-16: Addition of nullablethumbnailproperty inCryptoFileclass is implemented correctly.presentation/src/main/java/org/cryptomator/presentation/ui/bottomsheet/FileSettingsBottomSheet.kt (1)
37-43: Handling of thumbnail display inFileSettingsBottomSheetusingBitmapFactoryand conditional UI updates is implemented correctly.util/src/main/java/org/cryptomator/util/file/LruFileCacheUtil.kt (1)
34-34: Addition ofLOCALoption to theCacheenum to support local caching of thumbnails is appropriate.presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt (1)
29-29: Import ofSharedPreferencesHandlerinBrowseFilesFragmentfor accessing preferences is correctly added.data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormatPre7.kt (1)
131-142: Integration of thumbnail caching inCryptoImplVaultFormatPre7is implemented correctly, handling cache retrieval and setting thumbnails appropriately.util/src/main/java/org/cryptomator/util/SharedPreferencesHandler.kt (1)
164-171: Implementation ofgenerateThumbnails()inSharedPreferencesHandlerto retrieve user preferences for thumbnail generation is correctly done.presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.kt (3)
31-31: Ensure that the newly added constantTHUMBNAIL_GENERATIONis used consistently throughout the code.
259-259: Ensure the functionality ofthumbnailGenerationChangeListeneris implemented as it is crucial for handling user preferences changes.
342-342: The constantTHUMBNAIL_GENERATIONis appropriately declared.presentation/src/main/java/org/cryptomator/presentation/ui/adapter/BrowseFilesAdapter.kt (2)
151-157: Thumbnail display logic correctly checks if the file is an image and has a thumbnail before attempting to display it. Good use of Kotlin's safe call and smart cast.
159-160: The methodisImageMediaTypeeffectively determines if the file is an image based on its MIME type. This is crucial for the feature's correctness.data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt (5)
78-81: The thread pool for thumbnail generation is correctly configured with a custom thread factory. This is good for debugging and resource management.
83-85: MethodgetLruCacheForcorrectly abstracts the retrieval of the LRU cache based on the cloud type. This encapsulation aids maintainability.
98-109: TherenameFileInCachemethod correctly handles renaming of files in the cache. It checks if the old cache key exists before attempting to rename, which is a safe approach.
Line range hint
384-500: The methodreadhandles the reading of files and the conditional generation of thumbnails. It uses aPipedOutputStreamandPipedInputStreamfor thumbnail generation, which is an appropriate use of Java I/O for this purpose. However, ensure that resources are always closed in case of exceptions to avoid resource leaks.
446-466: ThestartThumbnailGeneratorThreadmethod correctly handles the generation of thumbnails in a separate thread. It uses a future to manage the asynchronous task, which is a good practice for concurrent operations.data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormat7.kt (3)
168-179: Thumbnail caching logic added to the listing process.This change integrates thumbnail caching into the file listing process, which should enhance the user experience by providing quick access to image previews. Ensure that the thumbnail generation and caching logic is thoroughly tested, especially in scenarios where the cache might become full or corrupted.
394-394: Ensure proper handling of file renaming in cache during file moves.This method is crucial for maintaining consistency in the cache when files are moved. It's important to verify that it handles edge cases such as renaming non-existent files, concurrent access scenarios, and potential race conditions.
464-472: Properly handle thumbnail cache deletion during file deletion.The implementation correctly checks for the existence of a thumbnail in the cache and deletes it if present. This is a necessary step to prevent orphaned cache entries which can lead to wasted storage space and potential privacy issues if sensitive thumbnails are not properly deleted.
presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt (2)
5-5: Ensure thatBitmapFactoryis used appropriately in the file to handle image processing for thumbnails.
517-517: The implementation for handling image previews appears to be correctly integrated with the new thumbnail feature. Ensure that theImagePreviewFilesStoreis properly tested, especially its interaction with the UI.
| private fun setupThumbnailGeneration() { | ||
| val preference = findPreference(THUMBNAIL_GENERATION) as Preference? | ||
| // TODO ... | ||
| } |
There was a problem hiding this comment.
Complete the implementation of setupThumbnailGeneration or provide additional details on the intended functionality.
| private val thumbnailGenerationChangeListener = Preference.OnPreferenceChangeListener { _, newValue -> | ||
| // TODO ... | ||
| true | ||
| } |
There was a problem hiding this comment.
Complete the implementation of thumbnailGenerationChangeListener or provide additional details on the intended functionality.
| @@ -1,5 +1,6 @@ | |||
| package org.cryptomator.presentation.ui.adapter | |||
|
|
|||
| import android.graphics.BitmapFactory | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The analysis of the BitmapFactory usage in CryptoImplDecorator.kt shows that it is configured safely. The BitmapFactory.Options is used with inSampleSize set to 4, effectively reducing the memory usage during image decoding. Additionally, the image is resized to a thumbnail, further mitigating memory concerns.
However, the other two files, FileSettingsBottomSheet.kt and BrowseFilesAdapter.kt, do not use any BitmapFactory.Options, which could potentially lead to unsafe configurations, especially in scenarios involving large images.
Recommended Actions:
- Review and potentially refactor the usage of
BitmapFactory.decodeFileinFileSettingsBottomSheet.ktandBrowseFilesAdapter.ktto include BitmapFactory.Options with appropriate settings to manage memory usage safely.
Analysis chain
Ensure BitmapFactory is used securely to avoid potential vulnerabilities related to image decoding.
The script results indicate that BitmapFactory.decodeFile and BitmapFactory.decodeStream are used in three different files. However, only in CryptoImplDecorator.kt is there an indication that BitmapFactory.Options might be used, which could potentially configure the decoding process safely. The other two usages in FileSettingsBottomSheet.kt and BrowseFilesAdapter.kt do not use any options, which might lead to unsafe configurations.
To confirm the safety of the usage in CryptoImplDecorator.kt and to provide specific guidance on what changes might be necessary in the other files, I need to inspect the options used in CryptoImplDecorator.kt.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that BitmapFactory is used with safe configurations throughout the project.
# Test: Search for unsafe usage of BitmapFactory. Expect: No occurrences of unsafe configurations.
rg --type kotlin "BitmapFactory.decode"
Length of output: 512
Script:
#!/bin/bash
# Description: Inspect the BitmapFactory.Options used in CryptoImplDecorator.kt to determine if they configure safe usage.
# Test: Extract the relevant lines around the BitmapFactory.decodeStream usage to see the configuration of BitmapFactory.Options.
rg --context 5 "BitmapFactory.decodeStream" data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt
Length of output: 543
|
@JustFanta01 Not a review, but a general suggestion: Edit: my bad, this was the wrong dependency. |
|
Thank you so much for this contribution ❤️, will have a closer look to it on Monday!
@infeo can you please explain in detail why we should switch from DiskLruCache to Caffeine? |
|
@SailReal I withdraw my suggestion^^ First, i thought this was an outdated, unmaintained dependency, but i was wrong. Second, the project already uses this dependency and then it is good practice to use what's already there. And third, the dependency targets Android, so i guess it is also "optimized" for the OS in some way. Regarding Caffeine: It uses a different algorithm with a statistically higher hit rate. See also https://github.com/ben-manes/caffeine/wiki/Efficiency. |
There was a problem hiding this comment.
In general, it looks really good, thanks for your contribution!
Besides the comments in the code, here are some general thoughts:
- Remove "Per folder" everywhere in the code, as it is not implemented yet.
- Remove the Thumbnail category in the settings and move the setting to General to avoid having a category with only one entry.
- I'll discuss this with Tobi, but I think the default should be to generate thumbnails by file, not never. Will come back with the result.
- Always use brackets in if statements
- The code is formatted almost everywhere, but some files are not completely 😅
- Verify the performance impact of listing huge folders with a lot of generated thumbnails
| private fun getOrCreateLruCache(key: LruFileCacheUtil.Cache, cacheSize: Int): DiskLruCache? { | ||
| return diskLruCache.computeIfAbsent(key) { |
There was a problem hiding this comment.
| private fun getOrCreateLruCache(key: LruFileCacheUtil.Cache, cacheSize: Int): DiskLruCache? { | |
| return diskLruCache.computeIfAbsent(key) { | |
| private fun getOrCreateLruCache(cache: LruFileCacheUtil.Cache, cacheSize: Int): DiskLruCache? { | |
| return diskLruCache.computeIfAbsent(cache) { |
| val where = LruFileCacheUtil(context).resolve(it) | ||
| try { | ||
| DiskLruCache.create(where, cacheSize.toLong()) | ||
| } catch (e: IOException) { | ||
| Timber.tag("CryptoImplDecorator").e(e, "Failed to setup LRU cache for $where.name") |
There was a problem hiding this comment.
| val where = LruFileCacheUtil(context).resolve(it) | |
| try { | |
| DiskLruCache.create(where, cacheSize.toLong()) | |
| } catch (e: IOException) { | |
| Timber.tag("CryptoImplDecorator").e(e, "Failed to setup LRU cache for $where.name") | |
| val cacheFile = LruFileCacheUtil(context).resolve(it) | |
| try { | |
| DiskLruCache.create(cacheFile, cacheSize.toLong()) | |
| } catch (e: IOException) { | |
| Timber.tag("CryptoImplDecorator").e(e, "Failed to setup LRU cache for $cacheFile.name") |
| } | ||
| } | ||
| } | ||
| protected fun renameFileInCache(source: CryptoFile, target: CryptoFile){ |
There was a problem hiding this comment.
Add a newline before this method and a blank before the {
| return buildString { | ||
| if (cloudFile.cloud?.id() != null) | ||
| this.append(cloudFile.cloud!!.id()) | ||
| else | ||
| this.append("c") // "common" | ||
| this.append("-") | ||
| this.append(cloudFile.path.hashCode()) | ||
| } |
There was a problem hiding this comment.
| return buildString { | |
| if (cloudFile.cloud?.id() != null) | |
| this.append(cloudFile.cloud!!.id()) | |
| else | |
| this.append("c") // "common" | |
| this.append("-") | |
| this.append(cloudFile.path.hashCode()) | |
| } | |
| return String.format("%s-%d", cloudFile.cloud?.id() ?: "common", cloudFile.path.hashCode()) |
| } | ||
|
|
||
| private fun isGenerateThumbnailsEnabled(cache: DiskLruCache?, fileName: String): Boolean { | ||
| return sharedPreferencesHandler.useLruCache() && |
There was a problem hiding this comment.
From a user perspective, it is currently not obvious that you need to enable caching for thumbnail generation to work. We may need to show a dialogue when changing the thumbnail generation from NONE to something else, and also for when caching gets disabled.
Even better, we could also decouple it here completely from the response cache so that we do not depend on this response caching feature, right?
| @@ -513,6 +514,7 @@ class BrowseFilesPresenter @Inject constructor( // | |||
| ) | |||
| } else if (!lowerFileName.endsWith(".gif") && isImageMediaType(cloudFile.name)) { | |||
| val cloudFileNodes = previewCloudFileNodes | |||
|
|
|||
| if(iv_file_image.drawable == null) | ||
| iv_file_image.setImageResource(cloudFileModel.icon.iconResource) |
There was a problem hiding this comment.
Please use brackets and auto format.
| @@ -106,6 +107,7 @@ class BrowseFilesFragment : BaseFragment() { | |||
| navigationMode?.let { cloudNodesAdapter.updateNavigationMode(it) } | |||
|
|
|||
| recyclerView.layoutManager = LinearLayoutManager(context()) | |||
| // recyclerView.layoutManager = GridLayoutManager(context(), 2) | |||
| private fun setupThumbnailGeneration() { | ||
| val preference = findPreference(THUMBNAIL_GENERATION) as Preference? | ||
| // TODO ... | ||
| } |
| private val thumbnailGenerationChangeListener = Preference.OnPreferenceChangeListener { _, newValue -> | ||
| // TODO ... | ||
| true | ||
| } |
|
Also there is a bug if you use an svg-file, then In this case, the file can not be opened anymore. Please also test it with further other file types. |
Hello guys 👋
This is our proposal for implementing thumbnail support [/issues/41]
Me, @WheelyMcBones and @taglioIsCoding have made the following changes.
We have implemented a uniform solution that works both for local and the remote clouds, this solution exploits the DiskLruCache in the CryptoImplDecorator and in the CryptoImplVaultFormat(Pre)7. We have decided these two location because we have access to all necessary informations: the decrypted image and the cloud type.
In cache we save a thumbnail when someone reads an image file and retrieve it from the same cache during the listing process. Thumbnails are stored as decrypted files in cache and, unlike other files in the /decrypted folder, these are persistent until the cache is deleted. We added the attribute ".thumbnail" in the CryptoFile pointing to the file in the disk cache and the CloudFileModel wraps it around.
We also added the Preference in the Settings for when it is supposed to generate the thumbnails; at the moment, the "Per Folder" preference is present but not implemented.
Finally we got rid of the full duplication of the image by elaborating the thumbnail in stream with an ad-hoc Thread pool.