Thumbnail support #533
Thumbnail support #533
Conversation
WalkthroughThis update enhances Cryptomator's functionality for managing image files by implementing thumbnail caching and management. It introduces new methods for associating thumbnails with cloud files, improves cache handling during file operations, and integrates user preferences for thumbnail generation. The changes also include enhancements to the user interface for better interaction with visible cloud file nodes. Changes
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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. |
…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
JustFanta01
force-pushed
the
feature/thumbnail-playground
branch
from
a487f6d to
e05f206
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (4)
presentation/src/main/java/org/cryptomator/presentation/ui/adapter/BrowseFilesAdapter.kt (1)
142-148: Add Error Handling for BitmapFactory.decodeFileThe
BitmapFactory.decodeFilemethod inBrowseFilesAdapter.ktlacks error handling, which could lead to runtime exceptions if the file path is invalid or the file is not accessible. Consider adding error handling to manage these cases effectively. The fallback mechanism inbindCloudNodeImageappears robust, but ensure it is thoroughly tested.
- File:
presentation/src/main/java/org/cryptomator/presentation/ui/adapter/BrowseFilesAdapter.kt- Lines: 142-148
Analysis chain
Ensure efficient image handling and error checking.
The
bindNodeImagefunction now handles thumbnails, which is a significant improvement. However, ensure that:
- The
BitmapFactory.decodeFilemethod is used efficiently, and consider adding error handling if the file path is invalid or the file is not accessible.- The fallback mechanism (
bindCloudNodeImage) is robust and handles all non-image cases effectively.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the efficiency and error handling of image decoding. # Test: Check for any reported issues or crashes related to image handling. grep -R "BitmapFactory.decodeFile" logs/Length of output: 79
Script:
#!/bin/bash # Search for the usage of BitmapFactory.decodeFile to check for error handling. rg "BitmapFactory.decodeFile" -A 5 # Search for the implementation of bindCloudNodeImage to verify the fallback mechanism. rg "fun bindCloudNodeImage" -A 10 # Search for any test files related to BrowseFilesAdapter.kt to check for tests on image handling. fd -e kt | xargs rg "BrowseFilesAdapter"Length of output: 4699
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt (1)
Line range hint
384-433: Suggestion: Improve handling of SVG and other unsupported formats in thumbnail generation.The method
readhas been effectively modified to include thumbnail generation. However, as noted in the PR comments, SVG files and potentially other formats are not supported byBitmapFactory.decodeStream, leading to exceptions. Consider implementing format-specific handling or a fallback mechanism to prevent crashes and ensure robustness across all supported file types.data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormat7.kt (1)
384-384: Important update for cache consistency on file move.The addition of the
renameFileInCachemethod within themoveoperation is crucial for maintaining cache consistency when files are renamed or moved. This ensures that thumbnails and other cached data are correctly updated, which is essential for data integrity and user experience.presentation/src/main/res/values/strings.xml (1)
Line range hint
635-649: Clear and concise string resources for thumbnail settings.The new string resources for controlling thumbnail generation are clear and concise, enhancing the user interface by providing understandable options for thumbnail management. Ensure that these strings are appropriately localized to maintain clarity across all supported languages.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- 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/presenter/VaultListPresenter.kt (1 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/adapter/BrowseFilesAdapter.kt (3 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/bottomsheet/FileSettingsBottomSheet.kt (2 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.kt (5 hunks)
- presentation/src/main/res/values/arrays.xml (1 hunks)
- presentation/src/main/res/values/strings.xml (2 hunks)
- presentation/src/main/res/xml/preferences.xml (1 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 not reviewed due to server errors (3)
- data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormatPre7.kt
- util/src/main/java/org/cryptomator/util/SharedPreferencesHandler.kt
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.kt
Files skipped from review due to trivial changes (1)
- util/src/main/java/org/cryptomator/util/ThumbnailsOption.kt
Additional context used
detekt
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt
[warning] 121-121: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
[warning] 442-442: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 471-471: String.format("%s-%d", cloudFile.cloud?.id() ?: "common", cloudFile.path.hashCode()) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
Additional comments not posted (16)
presentation/src/main/java/org/cryptomator/presentation/model/CloudFileModel.kt (2)
3-3: Approved import for new functionality.The import of
CryptoFileis necessary for accessing the new thumbnail functionality specific to encrypted files.
14-14: Approved conditional initialization of thumbnail property.The conditional initialization of the
thumbnailproperty ensures that onlyCryptoFileinstances are handled, which aligns with the PR's objectives. However, ensure that this does not introduce any unintended side effects, especially in scenarios whereCryptoFilemight not have a thumbnail.Verification successful
Usage of
thumbnailproperty is appropriate and aligns with conditional initialization logic.The
thumbnailproperty is used in various parts of the codebase, particularly in UI components, with checks for nullability to prevent errors. The presence of thumbnail generation logic inCryptoImplDecorator.ktsuggests that the system is designed to handle cases where thumbnails are not initially available, aligning with the conditional initialization inCloudFileModel.kt. This ensures robustness and prevents unintended side effects.
FileSettingsBottomSheet.ktandBrowseFilesAdapter.ktcorrectly handle null thumbnails.CryptoImplDecorator.ktincludes logic for thumbnail generation and storage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the thumbnail property is used correctly across the application. # Test: Search for usage of the thumbnail property in the application. rg --type kotlin -A 5 $'thumbnail'Length of output: 16371
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoFile.kt (1)
16-16: Approved addition of thumbnail property.The addition of the
thumbnailproperty to theCryptoFileclass is crucial for linking thumbnails to encrypted files. Ensure that this addition does not affect existing functionalities or introduce performance issues.presentation/src/main/res/values/arrays.xml (1)
45-54: Approved addition of thumbnail generation settings.The new string arrays
thumbnail_generation_entriesandthumbnail_generation_valuesare well-defined and enhance user customization for thumbnail generation. Ensure these settings are properly integrated and accessible in the application settings.Verification successful
Integration of thumbnail generation settings verified.
The string arrays
thumbnail_generation_entriesandthumbnail_generation_valuesare correctly integrated into the application settings through aListPreferenceinpresentation/src/main/res/xml/preferences.xml. This allows users to select their preferred thumbnail generation setting. The integration is complete and functional.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of thumbnail generation settings in the application. # Test: Search for the usage of the new string arrays in the application settings. rg --type xml -A 5 $'thumbnail_generation'Length of output: 3263
presentation/src/main/java/org/cryptomator/presentation/ui/bottomsheet/FileSettingsBottomSheet.kt (2)
3-3: Approved import for thumbnail handling.The addition of
android.graphics.BitmapFactoryis necessary for decoding thumbnail files and is correctly placed.
31-37: Thumbnail handling logic is well-implemented but monitor performance.The logic to check and display thumbnails is implemented correctly using Kotlin's safe calls and let function. However, consider monitoring performance as decoding image files can be resource-intensive.
util/src/main/java/org/cryptomator/util/file/LruFileCacheUtil.kt (2)
23-23: Approved addition ofLOCALto theCacheenum.The addition of
LOCALto the enum is necessary for handling local caching scenarios and is correctly implemented.
34-34: Approved handling ofLOCALin theresolvemethod.The addition of a case for
LOCALin theresolvemethod is correctly implemented and follows the established pattern for handling different cache types.presentation/src/main/res/xml/preferences.xml (1)
123-133: Well-implemented thumbnail generation settings.The addition of the new
PreferenceCategoryandListPreferencefor thumbnail generation settings is well-implemented. The use of string resources and arrays for entries and values follows best practices for localization and maintainability.presentation/src/main/java/org/cryptomator/presentation/ui/adapter/BrowseFilesAdapter.kt (1)
150-151: Good implementation of MIME type checking.The
isImageMediaTypefunction is well-implemented for checking if a file is an image based on its MIME type. This is crucial for the new thumbnail functionality.data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt (6)
4-8: Approved: Imports and thread pool initialization for thumbnail generation.The imports are essential for handling images and threading, which are crucial for the new thumbnail functionality. The lazy initialization of the
thumbnailExecutorServiceis efficient, ensuring resources are used only when necessary.Also applies to: 78-81
83-85: Approved: Efficient cache retrieval method.The method
getLruCacheForefficiently retrieves or creates an LRU cache based on the cloud type. Consider adding specific error handling or logging for unsupported cloud types to enhance robustness.
87-97: Approved with suggestion: Robust cache creation with good error handling.The method
getOrCreateLruCacheis well-implemented with proper use ofcomputeIfAbsentfor thread safety and efficiency. Consider enhancing the error message to include more details about the failure context, which could aid in debugging.
99-110: Approved: Efficient cache file renaming.The method
renameFileInCacheefficiently handles renaming files in the cache by checking the existence of the old cache key before proceeding. This prevents unnecessary operations and potential errors.
447-468: Approved with suggestion: Well-implemented asynchronous thumbnail generation.The method
startThumbnailGeneratorThreadis well-designed for handling thumbnail generation in a non-blocking manner. Consider adding more detailed logging within the catch block to aid in debugging and to better understand the nature of the exceptions that may occur.
521-523: Approved: Efficient media type checking for images.The method
isImageMediaTypeefficiently determines if a given filename corresponds to an image media type, leveraging existing utilities to reduce code duplication and ensure consistency.
| import org.cryptomator.util.file.MimeType | ||
| import org.cryptomator.util.file.MimeTypes | ||
| import javax.inject.Inject | ||
|
|
||
| class BrowseFilesAdapter @Inject | ||
| constructor( | ||
| private val dateHelper: DateHelper, // | ||
| private val fileSizeHelper: FileSizeHelper, // | ||
| private val fileUtil: FileUtil, // | ||
| private val sharedPreferencesHandler: SharedPreferencesHandler | ||
| private val sharedPreferencesHandler: SharedPreferencesHandler, // | ||
| private val mimeTypes: MimeTypes // |
There was a problem hiding this comment.
Refactor suggestion: Consider using a more efficient MIME type checking.
The addition of mimeTypes as a dependency is a good practice for handling MIME types. However, consider using a more efficient or standardized library if available, especially if the MIME type checking becomes a performance bottleneck.
| protected fun generateCacheKey(cloudFile: CloudFile): String { | ||
| return String.format("%s-%d", cloudFile.cloud?.id() ?: "common", cloudFile.path.hashCode()) | ||
| } |
There was a problem hiding this comment.
Suggestion: Specify locale in string formatting for cache key generation.
The method generateCacheKey effectively generates a unique key for caching. However, to avoid potential issues with different locales affecting the format, specify a locale (e.g., Locale.US) in the String.format method.
Tools
detekt
[warning] 471-471: String.format("%s-%d", cloudFile.cloud?.id() ?: "common", cloudFile.path.hashCode()) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
| protected fun associateThumbnailIfInCache(list: List<CryptoNode?>): List<CryptoNode?> { | ||
| if (isGenerateThumbnailsEnabled()) { | ||
| val firstCryptoFile = list.find { it is CryptoFile } ?: return list | ||
| val cloudType = (firstCryptoFile as CryptoFile).cloudFile.cloud?.type() ?: return list | ||
| val diskCache = getLruCacheFor(cloudType) ?: return list | ||
| list.forEach { cryptoNode -> | ||
| if (cryptoNode is CryptoFile && isImageMediaType(cryptoNode.name)) { | ||
| val cacheKey = generateCacheKey(cryptoNode.cloudFile) | ||
| val cacheFile = diskCache[cacheKey] | ||
| if (cacheFile != null) { | ||
| cryptoNode.thumbnail = cacheFile | ||
| } else { | ||
| // TODO | ||
| // force thumbnail generation (~PER FOLDER) | ||
| val trash = File.createTempFile(cryptoNode.name, ".temp", internalCache) | ||
| read(cryptoNode, trash.outputStream(), ProgressAware.NO_OP_PROGRESS_AWARE_DOWNLOAD) | ||
| trash.delete() | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return list |
There was a problem hiding this comment.
Approved with follow-up: Efficient thumbnail association with a note on future enhancements.
The method associateThumbnailIfInCache efficiently checks and associates thumbnails from the cache. The TODO comment about forcing thumbnail generation suggests further enhancements are planned. Consider creating an issue in the project's tracker to ensure this enhancement is not overlooked.
Would you like me to open a GitHub issue to track the implementation of the forced thumbnail generation feature?
| @@ -166,6 +165,8 @@ open class CryptoImplVaultFormat7 : CryptoImplDecorator { | |||
| } | |||
| }.map { node -> | |||
| ciphertextToCleartextNode(cryptoFolder, dirId, node) | |||
| }.also { | |||
| associateThumbnailIfInCache(it) | |||
There was a problem hiding this comment.
Good addition of thumbnail association in cache.
The method associateThumbnailIfInCache is a valuable addition for efficiently managing thumbnails in the application. Ensure that this method is optimized for performance, especially when handling directories with a large number of files, to avoid UI delays or excessive computational overhead.
| @@ -449,6 +451,15 @@ open class CryptoImplVaultFormat7 : CryptoImplDecorator { | |||
| } else { | |||
| cloudContentRepository.delete(node.cloudFile) | |||
| } | |||
|
|
|||
| val cacheKey = generateCacheKey(node.cloudFile) | |||
There was a problem hiding this comment.
Proper implementation of cache key generation.
The generateCacheKey method is well-placed to ensure unique identification of files in the cache. It's crucial that this method generates keys that are unique and consistent to avoid collisions and ensure effective cache usage. Consider using robust hashing algorithms and including unique file identifiers to enhance key uniqueness.
Sorry, clearly it was an oversight!
Multithreaded association and generation of thumbnails for a pseudo PER FOLDER option
…ain Thread Created a new usecase to fetch thumbnails trat updates the UI with a progressAware
…ally generate thumbnails for latest photos
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (20)
util/src/main/java/org/cryptomator/util/ThumbnailsOption.kt (1)
3-8: Add KDoc comments to improve code documentation.While the enum is straightforward, adding KDoc comments would enhance its usability and maintainability. Consider adding a class-level comment explaining the purpose of this enum and brief comments for each constant to clarify their meanings.
Here's a suggested implementation with KDoc comments:
/** * Represents the various options for thumbnail generation and management in the application. */ enum class ThumbnailsOption { /** Never generate thumbnails. */ NEVER, /** Generate thumbnails for read-only operations. */ READONLY, /** Generate thumbnails on a per-file basis. */ PER_FILE, /** Generate thumbnails on a per-folder basis. */ PER_FOLDER }domain/src/main/java/org/cryptomator/domain/usecases/cloud/AssociateThumbnails.java (3)
18-22: LGTM: Constructor is well-implemented with a minor suggestion.The constructor properly initializes the fields and follows the principle of dependency injection. The use of the
@Parameterannotation on thelistparameter is noted.Consider adding a null check for the
cloudContentRepositoryparameter to ensure it's not null, as it's a critical dependency:public AssociateThumbnails(CloudContentRepository cloudContentRepository, // @Parameter List<CloudNode> list) { + if (cloudContentRepository == null) { + throw new IllegalArgumentException("cloudContentRepository must not be null"); + } this.cloudContentRepository = cloudContentRepository; this.list = list; }
24-26: LGTM: Execute method is well-implemented with a minor suggestion.The
executemethod is appropriately defined and follows good practices:
- It takes a
ProgressAware<FileTransferState>parameter for progress tracking.- It can throw a
BackendExceptionfor error handling.- It correctly delegates the work to the repository, adhering to the separation of concerns principle.
Consider adding a null check for the
progressAwareparameter to provide a clearer error message if it's null:public void execute(ProgressAware<FileTransferState> progressAware) throws BackendException { + if (progressAware == null) { + throw new IllegalArgumentException("progressAware must not be null"); + } cloudContentRepository.associateThumbnails(list, progressAware); }
1-27: Overall assessment: Well-implemented use case for thumbnail association.This new
AssociateThumbnailsclass is a well-structured implementation of the thumbnail association use case. It aligns with the PR objectives and follows good software engineering practices:
- Clear separation of concerns
- Dependency injection
- Proper error handling
- Use of custom annotations for better code organization
The suggested minor improvements (null checks) would further enhance the robustness of the code.
Consider adding unit tests for this class to ensure its behavior is correct, especially testing edge cases and error scenarios.
presentation/src/main/res/values/arrays.xml (2)
45-50: LGTM! Consider clarifying the "per folder" option.The
thumbnail_generation_entriesarray is well-structured and provides a good range of options for thumbnail generation. It follows the existing pattern in the file and uses string resources, which is great for localization.Consider adding a comment or documentation to clarify that the "per folder" option (
@string/thumbnail_generation_folder) is not yet implemented, as mentioned in the PR objectives. This will help prevent confusion for other developers or users.
45-57: Overall, good addition of thumbnail generation options.The new
thumbnail_generation_entriesandthumbnail_generation_valuesarrays are well-structured and provide valuable configuration options for thumbnail generation in the application. They align with the PR objectives and follow the existing patterns in the file.A few points to consider:
- Clarify the status of the "per folder" option in comments or documentation.
- Ensure consistency between the arrays and the actual implementation, especially regarding the "PER_FOLDER" option.
- These changes will likely require corresponding updates in the application's settings UI and backend logic to handle the new options.
As you implement the logic for these new thumbnail generation options, consider the following:
- Ensure that the option selected by the user is properly persisted and retrieved.
- Implement appropriate logic in the thumbnail generation process to respect these settings.
- Update the UI to reflect the chosen option and possibly disable the "PER_FOLDER" option if it's not yet implemented.
- Consider adding unit tests to verify that the thumbnail generation behavior correctly follows the selected option.
domain/src/main/java/org/cryptomator/domain/repository/CloudContentRepository.kt (2)
98-101: LGTM: New methodassociateThumbnailsaddedThe new method
associateThumbnailsis well-structured and consistent with the interface's style. It aligns with the PR objectives of implementing thumbnail functionality.Consider adding KDoc comments to describe the method's purpose, parameters, and potential exceptions. For example:
/** * Associates thumbnails with a list of cloud nodes. * * @param list The list of cloud nodes to associate thumbnails with. * @param progressAware An object to track the progress of the thumbnail association process. * @throws BackendException if there's an error during the thumbnail association process. */ @Throws(BackendException::class) fun associateThumbnails(list: List<NodeType>, progressAware: ProgressAware<FileTransferState>) { // default implementation }
Line range hint
1-102: Summary: Thumbnail support added to CloudContentRepository interfaceThe changes to
CloudContentRepository.ktare minimal and focused, adding support for thumbnail functionality as outlined in the PR objectives. The newassociateThumbnailsmethod provides a clear extension point for implementing thumbnail association in concrete classes. The interface remains flexible and consistent with its existing design.As the thumbnail functionality is implemented, consider the following architectural points:
- Ensure that concrete implementations of this interface handle potential errors gracefully, especially when dealing with different file types (e.g., SVG files as mentioned in the PR comments).
- Consider adding a method to retrieve thumbnails, which might be useful for UI components that display file listings with thumbnails.
- As the thumbnail feature evolves, you might want to add methods for managing the thumbnail cache (e.g., clearing, updating, or checking cache status) in this interface or a related one.
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloudContentRepository.kt (2)
99-102: LGTM: New methodassociateThumbnailsimplemented correctly.The new method is well-implemented and aligns with the PR objectives for thumbnail functionality. It correctly delegates the call to the
cryptoImplinstance and usesProgressAwarefor tracking progress.Consider adding a KDoc comment to describe the purpose of this method and its parameters. For example:
/** * Associates thumbnails with a list of CryptoNode objects. * * @param list The list of CryptoNode objects to associate thumbnails with. * @param progressAware A ProgressAware object to track the progress of the operation. * @throws BackendException if there's an error during the thumbnail association process. */ @Throws(BackendException::class) override fun associateThumbnails(list: List<CryptoNode>, progressAware: ProgressAware<FileTransferState>) { cryptoImpl.associateThumbnails(list, progressAware) }This documentation will improve code readability and maintainability.
Line range hint
1-140: Summary: Thumbnail functionality successfully implemented.The changes in this file effectively implement the thumbnail functionality as described in the PR objectives. The new
associateThumbnailsmethod is well-integrated into the existingCryptoCloudContentRepositoryclass, following the established patterns and coding style.Key points:
- The new import and method are correctly placed and implemented.
- The changes are minimal and focused, reducing the risk of unintended side effects.
- The use of
ProgressAwareallows for tracking the progress of thumbnail association, which is beneficial for user experience.These changes contribute to the overall goal of providing thumbnail support for both local and remote cloud storage in the Cryptomator Android application.
As the thumbnail functionality evolves, consider the following architectural points:
- Ensure that the
cryptoImplimplementations (e.g.,CryptoImplVaultFormat8,CryptoImplVaultFormat7, etc.) have consistent implementations of theassociateThumbnailsmethod.- Monitor the performance impact of thumbnail generation and association, especially for large folders or slow network connections.
- Consider implementing caching strategies to optimize thumbnail retrieval in future iterations.
data/src/main/java/org/cryptomator/data/repository/DispatchingCloudContentRepository.kt (1)
168-180: Good implementation, but consider adding documentation and a safety check.The
associateThumbnailsmethod is well-implemented and follows the existing patterns in the class. However, there are a couple of suggestions for improvement:
- Add KDoc documentation to explain the purpose of the method and its parameters.
- Consider adding a check to ensure all nodes in the list belong to the same cloud, as the current implementation assumes this is the case.
Here's a suggested implementation with these improvements:
/** * Associates thumbnails with the given list of CloudNodes. * * @param list The list of CloudNodes to associate thumbnails with. * @param progressAware A ProgressAware object to track the file transfer state. * @throws IllegalArgumentException if the list contains nodes from different clouds. * @throws IllegalStateException if the cloud is null for any node. * @throws AuthenticationException if there's an authentication error. */ @Throws(BackendException::class) override fun associateThumbnails(list: List<CloudNode>, progressAware: ProgressAware<FileTransferState>) { if (list.isEmpty()) { return } val cloud = list[0].cloud ?: throw IllegalStateException("Cloud shouldn't be null") // Ensure all nodes belong to the same cloud if (list.any { it.cloud != cloud }) { throw IllegalArgumentException("All nodes must belong to the same cloud") } try { networkConnectionCheck.assertConnectionIsPresent(cloud) delegateFor(list[0]).associateThumbnails(list, progressAware) } catch (e: AuthenticationException) { delegates.remove(cloud) throw e } }This implementation adds documentation, ensures all nodes belong to the same cloud, and simplifies the null check for the cloud.
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormatPre7.kt (3)
131-131: Approve with suggestion: Consider logging unexpected null values.The addition of
.filterNotNull()is a good defensive programming practice that aligns with Kotlin's null safety features. It prevents potential null pointer exceptions in the code that consumes this list.However, if null values are not expected in this list, their presence might indicate an underlying issue that should be addressed.
Consider adding logging for unexpected null values before filtering them out. This can help in identifying and addressing the root cause of null values. For example:
.map { node -> ciphertextToCleartextNode(cryptoFolder, dirId, node) }.toList().onEach { if (it == null) Timber.w("Unexpected null value in list for folder: ${cryptoFolder.name}") }.filterNotNull()
229-229: Approve with suggestion: Add error handling for cache renaming.The addition of
renameFileInCache(source, target)is a good step to maintain consistency between the file system and the cache during move operations. This aligns with the thumbnail caching functionality mentioned in the PR objectives.However, it's important to ensure that the move operation remains robust even if the cache renaming fails.
Consider adding error handling for the cache renaming operation. This will ensure that the move operation completes successfully even if there's an issue with updating the cache. For example:
try { renameFileInCache(source, target) } catch (e: Exception) { Timber.e(e, "Failed to rename file in cache during move operation. Source: ${source.name}, Target: ${target.name}") // Optionally, you might want to invalidate the cache entry if renaming fails // invalidateCacheEntry(source) }
250-258: Approve with suggestions: Add error handling and consider potential race conditions.The addition of cache deletion logic when deleting a
CryptoFileis a good improvement. It ensures that the cache remains consistent with the actual file system, preventing potential issues with stale cached data.However, there are a few points to consider:
- Error Handling: Add try-catch block to handle potential exceptions during cache deletion.
- Race Conditions: Consider potential race conditions where the cache entry might be accessed while being deleted.
- Logging: Add logging for cache deletion operations for better traceability.
Here's a suggested implementation addressing these points:
val cacheKey = generateCacheKey(node) node.cloudFile.cloud?.type()?.let { cloudType -> getLruCacheFor(cloudType)?.let { diskCache -> try { diskCache.edit(cacheKey)?.let { editor -> editor.abort() // Prevent further access to this cache entry diskCache.remove(cacheKey) Timber.d("Successfully deleted cache entry for file: ${node.name}") } } catch (e: Exception) { Timber.e(e, "Failed to delete cache entry for file: ${node.name}") } } }This implementation uses the
edit()andabort()methods to prevent race conditions, adds error handling, and includes logging for better traceability.util/src/main/java/org/cryptomator/util/SharedPreferencesHandler.kt (2)
164-172: LGTM! Consider using an enum for better type safety.The
generateThumbnails()method is well-implemented and correctly handles different thumbnail generation options. It provides a good default value and uses Kotlin'swhenexpression effectively.To improve type safety and readability, consider defining an enum for the thumbnail generation options:
enum class ThumbnailGenerationOption(val value: String) { NEVER("NEVER"), READONLY("READONLY"), PER_FILE("PER_FILE"), PER_FOLDER("PER_FOLDER"); companion object { fun fromString(value: String): ThumbnailGenerationOption = values().find { it.value == value } ?: NEVER } }Then, update the
generateThumbnails()method:fun generateThumbnails(): ThumbnailsOption { val option = defaultSharedPreferences.getValue(THUMBNAIL_GENERATION, ThumbnailGenerationOption.NEVER.value) return ThumbnailGenerationOption.fromString(option).toThumbnailsOption() } private fun ThumbnailGenerationOption.toThumbnailsOption(): ThumbnailsOption { return when (this) { ThumbnailGenerationOption.NEVER -> ThumbnailsOption.NEVER ThumbnailGenerationOption.READONLY -> ThumbnailsOption.READONLY ThumbnailGenerationOption.PER_FILE -> ThumbnailsOption.PER_FILE ThumbnailGenerationOption.PER_FOLDER -> ThumbnailsOption.PER_FOLDER } }This approach provides better type safety and makes it easier to add or modify options in the future.
164-172: Thumbnail-related changes are well-integrated.The new
generateThumbnails()method andTHUMBNAIL_GENERATIONconstant are seamlessly integrated into theSharedPreferencesHandlerclass. They follow the existing patterns and conventions, maintaining consistency with the rest of the codebase.To further improve the implementation, consider adding a method to set the thumbnail generation option:
fun setThumbnailGeneration(option: ThumbnailsOption) { defaultSharedPreferences.setValue(THUMBNAIL_GENERATION, option.name) }This would provide a convenient way to update the thumbnail generation setting and ensure type safety when setting the value.
Also applies to: 331-331
presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.kt (2)
341-341: Consider using a resource string for the thumbnail generation preference keyThe addition of the
THUMBNAIL_GENERATIONconstant is good for managing the thumbnail generation preference. However, to improve consistency and maintainability, consider using a resource string for the preference key instead of a hardcoded string.Here's a suggested improvement:
- Add a new string resource in your
strings.xmlfile:<string name="pref_key_thumbnail_generation" translatable="false">thumbnailGeneration</string>
- Update the constant in the
SettingsFragment:private const val THUMBNAIL_GENERATION = R.string.pref_key_thumbnail_generation
- Update the usage in
useLruChangedListener:findPreference<ListPreference>(getString(THUMBNAIL_GENERATION))?.let { preference -> // ... existing code ... }This approach ensures consistency across the app and makes it easier to update the key if needed in the future.
Line range hint
1-341: Summary of SettingsFragment changesThe changes to the
SettingsFragmentimplement the thumbnail generation preference as described in the PR objectives. The new functionality is logically tied to the LRU cache setting, which is appropriate. However, there are several areas where the implementation can be improved:
- The
setupThumbnailGenerationmethod is still missing and should be implemented to properly initialize the thumbnail generation preference.- The handling of the LRU cache and thumbnail generation settings in
useLruChangedListenercan be refactored for better organization and error handling.- Consider using a resource string for the
THUMBNAIL_GENERATIONconstant to improve maintainability and consistency.These improvements will enhance the overall quality and robustness of the implementation. Please address these points in your next iteration.
To further improve the code:
- Consider extracting the thumbnail generation logic into a separate class or helper methods to keep the
SettingsFragmentfocused on UI-related tasks.- Implement proper error handling for cases where preferences might not be found.
- Ensure that the state of the thumbnail generation preference is properly persisted and restored across app restarts.
- Add unit tests for the new functionality to ensure its correctness and prevent regressions.
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormat7.kt (1)
452-460: Good addition of cache cleanup during file deletion.The new code ensures that the cache is updated when a file is deleted, maintaining consistency between the cache and the file system. This is a valuable improvement to prevent stale cache entries.
Consider adding error handling for potential exceptions during cache operations to improve robustness.
Consider wrapping the cache operations in a try-catch block to handle potential exceptions:
val cacheKey = generateCacheKey(node) node.cloudFile.cloud?.type()?.let { cloudType -> getLruCacheFor(cloudType)?.let { diskCache -> - if (diskCache[cacheKey] != null) { - diskCache.delete(cacheKey) + try { + if (diskCache[cacheKey] != null) { + diskCache.delete(cacheKey) + } + } catch (e: Exception) { + Timber.tag("CryptoFs").e(e, "Failed to delete cache entry for %s", cacheKey) } } }presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt (1)
89-89: Remove unnecessary@Overrideannotations in KotlinThe
@Overrideannotation is not used in Kotlin when overriding methods; instead, theoverridekeyword is sufficient. You can remove these annotations to clean up the code.Apply this diff:
private val onFastScrollStateChangeListener = object : OnFastScrollStateChangeListener { - @Override override fun onFastScrollStop() { thumbnailsForVisibleNodes() } - @Override override fun onFastScrollStart() { } } private val onScrollListener = object : RecyclerView.OnScrollListener() { - @Override override fun onScrollStateChanged(recyclerView: RecyclerView, newState: Int) { super.onScrollStateChanged(recyclerView, newState) if (newState == SCROLL_STATE_IDLE) {Also applies to: 94-94, 100-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
- data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloudContentRepository.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 (3 hunks)
- data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormatPre7.kt (3 hunks)
- data/src/main/java/org/cryptomator/data/repository/DispatchingCloudContentRepository.kt (2 hunks)
- domain/src/main/java/org/cryptomator/domain/repository/CloudContentRepository.kt (2 hunks)
- domain/src/main/java/org/cryptomator/domain/usecases/cloud/AssociateThumbnails.java (1 hunks)
- presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt (7 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt (4 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.kt (3 hunks)
- presentation/src/main/res/values/arrays.xml (1 hunks)
- presentation/src/main/res/values/strings.xml (2 hunks)
- presentation/src/main/res/xml/preferences.xml (1 hunks)
- util/src/main/java/org/cryptomator/util/SharedPreferencesHandler.kt (2 hunks)
- util/src/main/java/org/cryptomator/util/ThumbnailsOption.kt (1 hunks)
🧰 Additional context used
🪛 detekt
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt
[warning] 124-124: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
[warning] 449-449: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 477-477: String.format("%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode()) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt
[warning] 95-96: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (15)
util/src/main/java/org/cryptomator/util/ThumbnailsOption.kt (1)
1-8: Overall assessment: Good implementation with room for improvement.The
ThumbnailsOptionenum is well-structured and aligns with the PR objectives for implementing thumbnail support. It provides a clear set of options for thumbnail generation and management. However, adding KDoc comments and addressing the unimplementedPER_FOLDERoption would significantly enhance its clarity and usability.To ensure this enum is being used correctly throughout the project, let's run a quick check:
This script will help verify the integration of the new enum in the project and identify any premature usage of the unimplemented
PER_FOLDERoption.domain/src/main/java/org/cryptomator/domain/usecases/cloud/AssociateThumbnails.java (2)
1-11: LGTM: Package declaration and imports are appropriate.The package declaration and imports are well-organized and relevant to the functionality of the class. The use of custom annotations (
@Parameterand@UseCase) indicates a structured approach to use case implementation.
12-17: LGTM: Class declaration and fields are well-defined.The class is properly annotated as a use case, and the fields are correctly declared as private and final. The types of the fields (CloudContentRepository and List) are appropriate for the intended functionality.
presentation/src/main/res/values/arrays.xml (1)
51-56: LGTM! Ensure consistency with implementation status.The
thumbnail_generation_valuesarray is well-structured and provides clear, descriptive values that correspond to the entries in thethumbnail_generation_entriesarray.To ensure consistency between the arrays and the actual implementation, please verify that the "PER_FOLDER" option is handled correctly in the code. If it's not yet implemented, consider one of the following options:
- Remove the "PER_FOLDER" option from both arrays until it's implemented.
- Keep it in the arrays but ensure it's properly handled in the code (e.g., showing as disabled or with a "coming soon" indicator).
You can use the following script to check for the usage of "PER_FOLDER" in the codebase:
This will help ensure that the UI options align with the actual implementation.
✅ Verification successful
Verification Successful: "PER_FOLDER" is correctly implemented.
The usage of "PER_FOLDER" has been confirmed in both Kotlin and XML files, ensuring consistency between the
thumbnail_generation_valuesarray and the actual implementation in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of "PER_FOLDER" option in the codebase # Search for "PER_FOLDER" in Kotlin files echo "Searching for 'PER_FOLDER' usage in Kotlin files:" rg --type kotlin "PER_FOLDER" # Search for "thumbnail_generation_folder" in XML files echo "Searching for 'thumbnail_generation_folder' usage in XML files:" rg --type xml "thumbnail_generation_folder"Length of output: 952
domain/src/main/java/org/cryptomator/domain/repository/CloudContentRepository.kt (1)
11-11: LGTM: New import for FileTransferStateThe import of
FileTransferStateis appropriate and necessary for the newassociateThumbnailsmethod.data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloudContentRepository.kt (1)
16-16: LGTM: Import statement added correctly.The new import for
FileTransferStateis necessary for the newly addedassociateThumbnailsmethod and follows the existing import style.presentation/src/main/res/xml/preferences.xml (1)
144-151: LGTM! Consider adding a comment for clarity.The new ListPreference for thumbnail generation is well-structured and appropriately placed within the "LRU cache" category. The attributes are correctly set, using string resources for localization and following Android best practices.
Consider adding a comment above this preference to explain its purpose and potential impact on the app's behavior. This would be helpful for future maintenance.
Also, ensure that the following resources are defined and appropriate:
@string/dialog_thumbnail_generation_title@array/thumbnail_generation_entries@array/thumbnail_generation_valuesTo verify the uniqueness of the preference key and the existence of required resources, you can run the following script:
✅ Verification successful
Preference Key and Resources Verified
The
thumbnailGenerationpreference key is unique within the codebase. All required string and array resources (dialog_thumbnail_generation_title,thumbnail_generation_entries,thumbnail_generation_values) are present and correctly defined.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify uniqueness of preference key and existence of required resources # Check for duplicate keys echo "Checking for duplicate preference keys:" grep -n 'android:key="thumbnailGeneration"' $(find . -name '*.xml') # Check for required string resource echo "Checking for dialog_thumbnail_generation_title string resource:" grep -n 'name="dialog_thumbnail_generation_title"' $(find . -name 'strings.xml') # Check for required array resources echo "Checking for thumbnail_generation_entries array resource:" grep -n 'name="thumbnail_generation_entries"' $(find . -name 'arrays.xml') echo "Checking for thumbnail_generation_values array resource:" grep -n 'name="thumbnail_generation_values"' $(find . -name 'arrays.xml')Length of output: 21729
data/src/main/java/org/cryptomator/data/repository/DispatchingCloudContentRepository.kt (1)
17-17: LGTM: Import statement added correctly.The new import for
FileTransferStateis necessary for the newassociateThumbnailsmethod and follows the existing import style.util/src/main/java/org/cryptomator/util/SharedPreferencesHandler.kt (2)
331-331: LGTM! Constant is well-defined.The
THUMBNAIL_GENERATIONconstant is correctly defined and follows the established naming convention in the companion object.
164-172: Overall assessment: Thumbnail generation changes are well-implemented.The additions for thumbnail generation support in the
SharedPreferencesHandlerclass are well-designed and properly integrated. The code follows existing patterns and maintains consistency with the rest of the class. The suggestions provided for using an enum and adding a setter method would further enhance type safety and usability.Great job on implementing this feature! The changes align well with the PR objectives of adding thumbnail support to the Cryptomator Android application.
Also applies to: 331-331
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormat7.kt (3)
382-382: Good addition of cache management during file move operations.The new call to
renameFileInCacheis a valuable improvement. It ensures that the cache is updated when a file is moved, maintaining consistency between the cache and the actual file system. This addition enhances the overall reliability of the file management system.
Line range hint
1-577: Overall assessment: Good improvements to cache management and data processing.The changes in this file enhance the consistency between the file system and the cache, particularly during file move and delete operations. The addition of data decoration before writing is also a positive change. While no major issues were identified, consider implementing the suggested error handling improvement for cache operations to further increase the robustness of the system.
505-505: Appropriate use of data decoration before writing.The addition of
data.decorate()is a good practice, likely applying necessary transformations or additional processing to the data before writing. This aligns well with the cryptographic nature of the class.To ensure this change doesn't introduce any unintended side effects, could you provide more information about the
decoratemethod? Specifically:
- What operations does it perform on the data?
- Are there any performance implications?
- How does it interact with the encryption process?
✅ Verification successful
Verified: Addition of
data.decorate()aligns with existing patterns.The use of
data.decorate()in the specified location is consistent with its implementation across other components, ensuring proper data transformation before writing.
decoratemethod is defined and utilized appropriately in related classes:
DataSource.ktUriBasedDataSource.ktCryptoImplDecorator.kt🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the definition and usages of the 'decorate' method echo "Searching for 'decorate' method definition:" ast-grep --lang kotlin --pattern 'fun decorate($$$): $$$' echo "\nSearching for 'decorate' method usages:" rg '\bdecorate\b' --type kotlinLength of output: 1525
presentation/src/main/res/values/strings.xml (1)
635-640: New string resources for thumbnail generation look good.The new string resources for thumbnail generation settings have been added correctly. They follow the existing naming conventions and are properly formatted. Here's a breakdown of the changes:
- Four options for thumbnail generation have been added: "Never", "Read Only", "Generate Per File", and "Generate Per Folder".
- A toast message for when LRU cache is disabled has been included.
- A title for the thumbnail generation dialog has been added.
These additions enhance the app's capability to manage thumbnail generation preferences, providing users with more control over how thumbnails are created and displayed.
Also applies to: 650-650
presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt (1)
130-131: EnsureRecyclerViewsupports fast scroll listener methodsVerify that
setOnFastScrollStateChangeListeneris available for yourRecyclerView. If you're using a customRecyclerViewor a library that adds this method, ensure it is properly integrated. If not, you may need to implement fast scroll functionality differently.Run the following script to check for the method's availability:
| NEVER, | ||
| READONLY, | ||
| PER_FILE, | ||
| PER_FOLDER |
There was a problem hiding this comment.
Address the unimplemented PER_FOLDER option.
The PR objectives mention that the PER_FOLDER option is currently not implemented. It's important to clearly indicate this in the code to prevent misuse and provide clarity for other developers.
Consider one of the following approaches:
- Add a TODO comment to remind developers about the unimplemented state.
- Use the
@Deprecatedannotation with a message explaining that it's not yet implemented. - If it's not intended for use at all, consider removing it from the enum.
Here's an example using the @Deprecated annotation:
/** Generate thumbnails on a per-folder basis. */
@Deprecated("Not yet implemented")
PER_FOLDER|
|
||
| findPreference<ListPreference>(THUMBNAIL_GENERATION)?.let { preference -> | ||
| preference.isSelectable = false | ||
| } | ||
| Toast.makeText(context, context?.getString(R.string.thumbnail_generation__deactivation_toast), Toast.LENGTH_LONG).show() | ||
| } | ||
|
|
||
| if (TRUE == newValue) { | ||
| findPreference<ListPreference>(THUMBNAIL_GENERATION)?.let { preference -> | ||
| preference.isSelectable = true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve handling of LRU cache and thumbnail generation settings
The changes implement a logical dependency between the LRU cache and thumbnail generation settings. However, there are a few improvements that can be made:
- The toast message for LRU cache changes is displayed regardless of whether the value actually changed. Consider only showing the toast when the value changes.
- When the LRU cache is enabled, consider calling a method to restore the previous state of the thumbnail generation preference, as it might have been enabled before the LRU cache was disabled.
- The implementation of
setupThumbnailGenerationis still missing, as mentioned in a past review comment. This method should be implemented to properly initialize the thumbnail generation preference.
Here's a suggested improvement:
private val useLruChangedListener = Preference.OnPreferenceChangeListener { _, newValue ->
val lruEnabled = newValue as Boolean
if (lruEnabled != sharedPreferencesHandler.useLruCache()) {
if (!lruEnabled) {
LruFileCacheUtil(requireContext()).clear()
setupLruCacheSize()
disableThumbnailGeneration()
} else {
restoreThumbnailGenerationState()
}
Toast.makeText(context, context?.getString(R.string.screen_settings_lru_cache_changed__restart_toast), Toast.LENGTH_SHORT).show()
}
true
}
private fun disableThumbnailGeneration() {
findPreference<ListPreference>(THUMBNAIL_GENERATION)?.let { preference ->
preference.isSelectable = false
}
Toast.makeText(context, context?.getString(R.string.thumbnail_generation__deactivation_toast), Toast.LENGTH_LONG).show()
}
private fun restoreThumbnailGenerationState() {
findPreference<ListPreference>(THUMBNAIL_GENERATION)?.let { preference ->
preference.isSelectable = true
// Restore the previous state if needed
}
}
// TODO: Implement this method
private fun setupThumbnailGeneration() {
// Initialize the thumbnail generation preference
}Don't forget to call setupThumbnailGeneration() in the onCreatePreferences method.
| @@ -6,10 +6,13 @@ import android.util.TypedValue | |||
| import android.view.View | |||
| import android.view.View.GONE | |||
| import android.view.View.VISIBLE | |||
| import android.widget.AbsListView.OnScrollListener.SCROLL_STATE_IDLE | |||
There was a problem hiding this comment.
Use RecyclerView.SCROLL_STATE_IDLE instead of AbsListView
The constant SCROLL_STATE_IDLE is being imported from android.widget.AbsListView.OnScrollListener, which is intended for ListView. Since you are working with RecyclerView, you should import SCROLL_STATE_IDLE from androidx.recyclerview.widget.RecyclerView to ensure consistency and avoid potential issues.
Apply this diff to fix the import:
-import android.widget.AbsListView.OnScrollListener.SCROLL_STATE_IDLE
+import androidx.recyclerview.widget.RecyclerView.SCROLL_STATE_IDLE📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import android.widget.AbsListView.OnScrollListener.SCROLL_STATE_IDLE | |
| import androidx.recyclerview.widget.RecyclerView.SCROLL_STATE_IDLE |
| override fun onFastScrollStart() { | ||
| } |
There was a problem hiding this comment.
Remove empty onFastScrollStart method
The onFastScrollStart method is empty and can be removed if no action is required when fast scrolling starts. This will simplify your code.
Apply this diff to remove the empty method:
override fun onFastScrollStop() {
thumbnailsForVisibleNodes()
}
- override fun onFastScrollStart() {
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override fun onFastScrollStart() { | |
| } | |
| override fun onFastScrollStop() { | |
| thumbnailsForVisibleNodes() | |
| } | |
| } |
🧰 Tools
🪛 detekt
[warning] 95-96: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
| val first = layoutManager.findFirstVisibleItemPosition() | ||
| val last = layoutManager.findLastVisibleItemPosition() | ||
| val visibleCloudNodes = cloudNodesAdapter.renderedCloudNodes().subList(first, last + 1) |
There was a problem hiding this comment.
Handle potential -1 values from findFirstVisibleItemPosition() and findLastVisibleItemPosition()
The methods findFirstVisibleItemPosition() and findLastVisibleItemPosition() can return -1 if no items are visible, which could lead to an IndexOutOfBoundsException when calling subList. Consider adding a check to ensure that first and last are valid indices before proceeding.
Apply this diff to handle the potential issue:
val layoutManager = binding.recyclerViewLayout.recyclerView.layoutManager as LinearLayoutManager
val first = layoutManager.findFirstVisibleItemPosition()
val last = layoutManager.findLastVisibleItemPosition()
+if (first != RecyclerView.NO_POSITION && last != RecyclerView.NO_POSITION) {
val visibleCloudNodes = cloudNodesAdapter.renderedCloudNodes().subList(first, last + 1)
if (!binding.swipeRefreshLayout.isRefreshing) {
browseFilesPresenter.thumbnailsForVisibleNodes(visibleCloudNodes)
}
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val first = layoutManager.findFirstVisibleItemPosition() | |
| val last = layoutManager.findLastVisibleItemPosition() | |
| val visibleCloudNodes = cloudNodesAdapter.renderedCloudNodes().subList(first, last + 1) | |
| val first = layoutManager.findFirstVisibleItemPosition() | |
| val last = layoutManager.findLastVisibleItemPosition() | |
| if (first != RecyclerView.NO_POSITION && last != RecyclerView.NO_POSITION) { | |
| val visibleCloudNodes = cloudNodesAdapter.renderedCloudNodes().subList(first, last + 1) | |
| if (!binding.swipeRefreshLayout.isRefreshing) { | |
| browseFilesPresenter.thumbnailsForVisibleNodes(visibleCloudNodes) | |
| } | |
| } |
| Timber.e("Bitmap generation crashed") | ||
| } | ||
| } |
There was a problem hiding this comment.
Log exception details when catching exceptions
Catching exceptions without logging the exception details can make it difficult to diagnose problems. Logging the exception provides valuable information for debugging.
Apply this diff to include the exception in the log:
} catch (e: Exception) {
- Timber.e("Bitmap generation crashed")
+ Timber.e(e, "Bitmap generation crashed")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Timber.e("Bitmap generation crashed") | |
| } | |
| } | |
| Timber.e(e, "Bitmap generation crashed") | |
| } | |
| } |
| options.inSampleSize = 4 // pixel number reduced by a factor of 1/16 | ||
| val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options) | ||
| val thumbnailWidth = 100 | ||
| val thumbnailHeight = 100 | ||
| thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight) | ||
| if (thumbnailBitmap != null) { | ||
| storeThumbnail(diskCache, cacheKey, thumbnailBitmap) | ||
| } |
There was a problem hiding this comment.
Handle potential null Bitmap from BitmapFactory.decodeStream
BitmapFactory.decodeStream can return null if the input stream cannot be decoded into a bitmap (e.g., unsupported formats like SVG). Not handling this case may lead to a NullPointerException when ThumbnailUtils.extractThumbnail is called.
Modify the code to check if bitmap is not null before proceeding:
val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options)
+ if (bitmap != null) {
val thumbnailWidth = 100
val thumbnailHeight = 100
val thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight)
if (thumbnailBitmap != null) {
storeThumbnail(diskCache, cacheKey, thumbnailBitmap)
}
+ } else {
+ Timber.e("Failed to decode bitmap from input stream for file: ${cryptoFile.name}")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| options.inSampleSize = 4 // pixel number reduced by a factor of 1/16 | |
| val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options) | |
| val thumbnailWidth = 100 | |
| val thumbnailHeight = 100 | |
| thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight) | |
| if (thumbnailBitmap != null) { | |
| storeThumbnail(diskCache, cacheKey, thumbnailBitmap) | |
| } | |
| options.inSampleSize = 4 // pixel number reduced by a factor of 1/16 | |
| val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options) | |
| if (bitmap != null) { | |
| val thumbnailWidth = 100 | |
| val thumbnailHeight = 100 | |
| thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight) | |
| if (thumbnailBitmap != null) { | |
| storeThumbnail(diskCache, cacheKey, thumbnailBitmap) | |
| } | |
| } else { | |
| Timber.e("Failed to decode bitmap from input stream for file: ${cryptoFile.name}") | |
| } |
| @Override | ||
| override fun onFinished() { |
There was a problem hiding this comment.
Remove unnecessary @Override annotation
The @Override annotation is unnecessary in Kotlin. It should be removed to prevent potential issues.
Apply this diff to remove the annotation:
- @Override
override fun onFinished() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| override fun onFinished() { | |
| override fun onFinished() { |
| @Override | ||
| override fun onFinished() { | ||
| val images = view?.renderedCloudNodes()?.filterIsInstance<CloudFileModel>()?.filter { file -> isImageMediaType(file.name) } ?: return | ||
| val firstImages = images.subList(0, 10) |
There was a problem hiding this comment.
Prevent IndexOutOfBoundsException when sublisting images
Using images.subList(0, 10) can throw an IndexOutOfBoundsException if the list has fewer than 10 elements. Use images.take(10) to safely take up to 10 elements.
Apply this diff to fix the issue:
- val firstImages = images.subList(0, 10)
+ val firstImages = images.take(10)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val firstImages = images.subList(0, 10) | |
| val firstImages = images.take(10) |
| @Override | ||
| override fun onProgress(progress: Progress<FileTransferState>) { |
There was a problem hiding this comment.
Remove unnecessary @Override annotation
The @Override annotation is not used in Kotlin and can lead to compilation errors. The override keyword is sufficient to override methods.
Apply this diff to remove the unnecessary annotation:
- @Override
override fun onProgress(progress: Progress<FileTransferState>) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| override fun onProgress(progress: Progress<FileTransferState>) { | |
| override fun onProgress(progress: Progress<FileTransferState>) { |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt (1)
89-98: Remove emptyonFastScrollStartmethodThe
onFastScrollStartmethod is empty and can be safely removed. This will simplify the code without affecting functionality.Apply this diff to remove the empty method:
private val onFastScrollStateChangeListener = object : OnFastScrollStateChangeListener { @Override override fun onFastScrollStop() { thumbnailsForVisibleNodes() } - @Override - override fun onFastScrollStart() { - } }🧰 Tools
🪛 detekt
[warning] 96-97: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt (2)
Line range hint
387-440: Improve error handling in thumbnail generation process.The modifications to the
readmethod for incorporating thumbnail generation are generally good. However, there's a potential issue with error handling:
- If an exception occurs during the file reading process, the
thumbnailWriterandthumbnailReadermight not be properly closed.- The
futureThumbnail.get()call could potentially block indefinitely if there's an issue with the thumbnail generation thread.Consider wrapping the thumbnail-related operations in a try-catch block and ensure proper resource cleanup in case of exceptions. Also, consider adding a timeout to the
futureThumbnail.get()call to prevent potential deadlocks.Here's a suggested improvement:
try { // ... existing code ... } catch (e: IOException) { + closeQuietly(thumbnailWriter) + closeQuietly(thumbnailReader) throw FatalBackendException(e) } finally { if (genThumbnail) { - futureThumbnail.get() + try { + futureThumbnail.get(30, TimeUnit.SECONDS) + } catch (e: Exception) { + Timber.e(e, "Error generating thumbnail") + } } }
Line range hint
1-683: Summary: Good implementation with room for improvement in error handling and resource management.The changes to
CryptoImplDecorator.ktsuccessfully implement thumbnail generation and caching functionality. The code is generally well-structured and efficient. However, there are several areas where improvements can be made:
- Error handling: Enhance exception handling in the
readmethod andstartThumbnailGeneratorThreadto provide more specific error messages and ensure proper resource cleanup.- Resource management: Ensure that all resources (especially
InputStreams and temporary files) are properly closed or deleted, even in error scenarios.- Consistency: Use explicit locales in string formatting to ensure consistent behavior across different devices.
- Logging: Improve logging to aid in debugging and monitoring, particularly for the
closeQuietlymethod.Addressing these points will enhance the robustness and maintainability of the code. Overall, the implementation is a solid foundation for the new thumbnail functionality.
🧰 Tools
🪛 detekt
[warning] 449-449: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 482-482: String.format("%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode()) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt (2)
164-178: New methodthumbnailsForVisibleNodes: Looks good with a minor suggestion.The method correctly implements the logic for identifying visible nodes that need thumbnails and initiates the thumbnail generation process. It appropriately checks user preferences before proceeding.
Consider using
filterinstead offorEachfor a more functional approach:val toDownload = visibleCloudNodes.filter { node -> node is CloudFileModel && isImageMediaType(node.name) && node.thumbnail == null }This would make the code more concise and potentially more efficient.
290-312: New methodassociateThumbnails: Good implementation with a suggestion for optimization.The method correctly implements the logic for associating thumbnails with cloud nodes, including proper checks for user preferences. However, there's a potential performance optimization in the thumbnail generation part:
images.take(10).filter { img -> img.thumbnail == null }.let { firstImagesWithoutThumbnails -> if (firstImagesWithoutThumbnails.isNotEmpty()) { thumbnailsForVisibleNodes(firstImagesWithoutThumbnails) } }This code first takes 10 images and then filters them. It would be more efficient to filter first and then take up to 10 images:
images.filter { img -> img.thumbnail == null }.take(10).let { firstImagesWithoutThumbnails -> if (firstImagesWithoutThumbnails.isNotEmpty()) { thumbnailsForVisibleNodes(firstImagesWithoutThumbnails) } }This ensures that we're only processing up to 10 images that actually need thumbnails, potentially reducing unnecessary work.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt (7 hunks)
- presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt (7 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt (4 hunks)
🧰 Additional context used
🪛 detekt
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt
[warning] 449-449: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 482-482: String.format("%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode()) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt
[warning] 96-97: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (10)
presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt (3)
12-14: LGTM: Correct imports added for new functionalityThe new imports for RecyclerView and SCROLL_STATE_IDLE are appropriate for the added scroll state management functionality. Good job on using the correct constant from RecyclerView instead of AbsListView.
100-108: LGTM: Efficient scroll listener implementationThe new
onScrollListenerimplementation is well-designed. It callsthumbnailsForVisibleNodes()only when the scroll state is idle, which is an efficient approach to loading thumbnails. This prevents unnecessary processing during active scrolling, improving performance.
131-132: LGTM: Proper setup of new listenersThe new listeners
onFastScrollStateChangeListenerandonScrollListenerare correctly added to the RecyclerView in thesetupViewmethod. This ensures that the thumbnail loading functionality will work as intended during scrolling and fast scrolling.data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt (2)
4-7: LGTM: New imports and member variables for thumbnail functionality.The added imports and member variables are appropriate for implementing the new thumbnail generation and caching features. The use of a dedicated thread pool for thumbnail generation is a good practice to avoid blocking the main thread.
Also applies to: 31-39, 75-84
489-518: LGTM: Well-implementedassociateThumbnailsmethod.The
associateThumbnailsmethod is well-structured and efficient:
- It properly checks if thumbnail generation is enabled before proceeding.
- The filtering process ensures only relevant files (images without thumbnails) are processed.
- The use of
measureTimeMillisfor performance logging is a good practice for monitoring and future optimizations.- The method handles potential null values appropriately.
The implementation looks good and should perform well.
presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt (5)
28-28: New imports for thumbnail functionality: LGTM!The new imports for
AssociateThumbnailsUseCaseandFileTransferStateare correctly added and necessary for the implementation of the thumbnail functionality.Also applies to: 33-33
102-102: Constructor modification for thumbnail functionality: LGTM!The addition of the
associateThumbnailsUseCaseparameter to the constructor is correct and follows the existing pattern of dependency injection.
261-261: Modification togetCloudListmethod: LGTM!The addition of
associateThumbnails(cloudNodes)after showing cloud nodes in the view is a logical and correct place to perform this operation. It ensures that thumbnails are associated with cloud nodes immediately after retrieving the cloud list.
1407-1408: Modifications toinitblock: LGTM!The additions to unsubscribe
getDecryptedCloudForVaultUseCaseandassociateThumbnailsUseCasein theinitblock are correct and necessary. This ensures proper cleanup of resources when the presenter is destroyed, preventing potential memory leaks and unnecessary background work.
Line range hint
1-1408: Overall assessment of changes in BrowseFilesPresenter.ktThe modifications to implement thumbnail functionality in the
BrowseFilesPresenterclass are well-designed and properly integrated with the existing codebase. The new methods and changes to existing methods logically implement the required functionality while maintaining consistency with the class's overall structure and coding style.Key points:
- Proper dependency injection for the new use case.
- Correct implementation of user preference checks before performing thumbnail operations.
- Appropriate error handling and progress reporting in the new methods.
- Logical placement of thumbnail association within the existing workflow.
While the implementation is generally solid, a few minor optimizations have been suggested to improve performance and code clarity. These suggestions, if implemented, would further enhance the quality of the code.
In conclusion, the changes effectively implement the desired thumbnail functionality while maintaining good coding practices and consistency with the existing codebase.
| private fun thumbnailsForVisibleNodes() { | ||
| val layoutManager = binding.recyclerViewLayout.recyclerView.layoutManager as LinearLayoutManager | ||
| val first = layoutManager.findFirstVisibleItemPosition() | ||
| val last = layoutManager.findLastVisibleItemPosition() | ||
| if (first == NO_POSITION || last == NO_POSITION) { | ||
| return | ||
| } | ||
| val visibleCloudNodes = cloudNodesAdapter.renderedCloudNodes().subList(first, last + 1) | ||
| if (!binding.swipeRefreshLayout.isRefreshing) { | ||
| browseFilesPresenter.thumbnailsForVisibleNodes(visibleCloudNodes) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM with a minor optimization suggestion
The thumbnailsForVisibleNodes method is well-implemented. It correctly handles edge cases and avoids unnecessary work when the SwipeRefreshLayout is refreshing.
Consider this minor optimization to reduce object creation:
-val visibleCloudNodes = cloudNodesAdapter.renderedCloudNodes().subList(first, last + 1)
if (!binding.swipeRefreshLayout.isRefreshing) {
- browseFilesPresenter.thumbnailsForVisibleNodes(visibleCloudNodes)
+ browseFilesPresenter.thumbnailsForVisibleNodes(cloudNodesAdapter.renderedCloudNodes(), first, last)
}This change avoids creating a new sublist object on each call. You'll need to update the thumbnailsForVisibleNodes method in the BrowseFilesPresenter to accept the full list and the range indices.
Committable suggestion was skipped due to low confidence.
| private fun closeQuietly(closeable: Closeable) { | ||
| try { | ||
| closeable.close(); | ||
| } catch (e: IOException) { | ||
| // ignore | ||
| } | ||
| } |
There was a problem hiding this comment.
Log exceptions in closeQuietly method.
The closeQuietly method is a useful utility, but swallowing exceptions without logging them can hide potential issues and make debugging difficult.
Consider logging the exception at a debug level:
private fun closeQuietly(closeable: Closeable) {
try {
closeable.close()
} catch (e: IOException) {
- // ignore
+ Timber.d(e, "IOException occurred while closing Closeable")
}
}This change will help in identifying and troubleshooting any issues related to resource closing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun closeQuietly(closeable: Closeable) { | |
| try { | |
| closeable.close(); | |
| } catch (e: IOException) { | |
| // ignore | |
| } | |
| } | |
| private fun closeQuietly(closeable: Closeable) { | |
| try { | |
| closeable.close(); | |
| } catch (e: IOException) { | |
| Timber.d(e, "IOException occurred while closing Closeable") | |
| } | |
| } |
🧰 Tools
🪛 detekt
[warning] 449-449: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
| protected fun generateCacheKey(cryptoFile: CryptoFile): String { | ||
| return String.format("%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode()) | ||
| } |
There was a problem hiding this comment.
Use explicit locale in generateCacheKey method.
The generateCacheKey method effectively generates a unique key for caching thumbnails. However, using the default locale in String.format can lead to inconsistencies across different devices.
To ensure consistent behavior across all devices, specify an explicit locale:
protected fun generateCacheKey(cryptoFile: CryptoFile): String {
- return String.format("%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode())
+ return String.format(Locale.US, "%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode())
}This change ensures that the cache key generation is consistent regardless of the device's locale settings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected fun generateCacheKey(cryptoFile: CryptoFile): String { | |
| return String.format("%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode()) | |
| } | |
| protected fun generateCacheKey(cryptoFile: CryptoFile): String { | |
| return String.format(Locale.US, "%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode()) | |
| } |
🧰 Tools
🪛 detekt
[warning] 482-482: String.format("%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode()) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
| private fun storeThumbnail(cache: DiskLruCache?, cacheKey: String, thumbnailBitmap: Bitmap) { | ||
| val thumbnailFile: File = File.createTempFile(UUID.randomUUID().toString(), ".thumbnail", internalCache) | ||
| thumbnailBitmap.compress(Bitmap.CompressFormat.JPEG, 100, thumbnailFile.outputStream()) | ||
|
|
||
| try { | ||
| cache?.let { | ||
| LruFileCacheUtil.storeToLruCache(it, cacheKey, thumbnailFile) | ||
| } ?: Timber.tag("CryptoImplDecorator").e("Failed to store item in LRU cache") | ||
| } catch (e: IOException) { | ||
| Timber.tag("CryptoImplDecorator").e(e, "Failed to write the thumbnail in DiskLruCache") | ||
| } | ||
|
|
||
| thumbnailFile.delete() | ||
| } |
There was a problem hiding this comment.
Improve resource management in storeThumbnail method.
The storeThumbnail method is generally well-implemented, but there's a potential resource leak if an exception occurs during the bitmap compression or cache storage process. The temporary file might not be deleted in such cases.
Consider using Kotlin's use function or a try-finally block to ensure the temporary file is always deleted:
private fun storeThumbnail(cache: DiskLruCache?, cacheKey: String, thumbnailBitmap: Bitmap) {
- val thumbnailFile: File = File.createTempFile(UUID.randomUUID().toString(), ".thumbnail", internalCache)
- thumbnailBitmap.compress(Bitmap.CompressFormat.JPEG, 100, thumbnailFile.outputStream())
-
- try {
- cache?.let {
- LruFileCacheUtil.storeToLruCache(it, cacheKey, thumbnailFile)
- } ?: Timber.tag("CryptoImplDecorator").e("Failed to store item in LRU cache")
- } catch (e: IOException) {
- Timber.tag("CryptoImplDecorator").e(e, "Failed to write the thumbnail in DiskLruCache")
+ val thumbnailFile = File.createTempFile(UUID.randomUUID().toString(), ".thumbnail", internalCache)
+ try {
+ thumbnailFile.outputStream().use { outputStream ->
+ thumbnailBitmap.compress(Bitmap.CompressFormat.JPEG, 100, outputStream)
+ }
+
+ try {
+ cache?.let {
+ LruFileCacheUtil.storeToLruCache(it, cacheKey, thumbnailFile)
+ } ?: Timber.tag("CryptoImplDecorator").e("Failed to store item in LRU cache")
+ } catch (e: IOException) {
+ Timber.tag("CryptoImplDecorator").e(e, "Failed to write the thumbnail in DiskLruCache")
+ }
+ } finally {
+ thumbnailFile.delete()
}
-
- thumbnailFile.delete()
}This change ensures that the temporary file is always deleted, even if an exception occurs during the process.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun storeThumbnail(cache: DiskLruCache?, cacheKey: String, thumbnailBitmap: Bitmap) { | |
| val thumbnailFile: File = File.createTempFile(UUID.randomUUID().toString(), ".thumbnail", internalCache) | |
| thumbnailBitmap.compress(Bitmap.CompressFormat.JPEG, 100, thumbnailFile.outputStream()) | |
| try { | |
| cache?.let { | |
| LruFileCacheUtil.storeToLruCache(it, cacheKey, thumbnailFile) | |
| } ?: Timber.tag("CryptoImplDecorator").e("Failed to store item in LRU cache") | |
| } catch (e: IOException) { | |
| Timber.tag("CryptoImplDecorator").e(e, "Failed to write the thumbnail in DiskLruCache") | |
| } | |
| thumbnailFile.delete() | |
| } | |
| private fun storeThumbnail(cache: DiskLruCache?, cacheKey: String, thumbnailBitmap: Bitmap) { | |
| val thumbnailFile = File.createTempFile(UUID.randomUUID().toString(), ".thumbnail", internalCache) | |
| try { | |
| thumbnailFile.outputStream().use { outputStream -> | |
| thumbnailBitmap.compress(Bitmap.CompressFormat.JPEG, 100, outputStream) | |
| } | |
| try { | |
| cache?.let { | |
| LruFileCacheUtil.storeToLruCache(it, cacheKey, thumbnailFile) | |
| } ?: Timber.tag("CryptoImplDecorator").e("Failed to store item in LRU cache") | |
| } catch (e: IOException) { | |
| Timber.tag("CryptoImplDecorator").e(e, "Failed to write the thumbnail in DiskLruCache") | |
| } | |
| } finally { | |
| thumbnailFile.delete() | |
| } | |
| } |
| private fun startThumbnailGeneratorThread(cryptoFile: CryptoFile, diskCache: DiskLruCache, cacheKey: String, thumbnailReader: PipedInputStream): Future<*> { | ||
| return thumbnailExecutorService.submit { | ||
| try { | ||
| val options = BitmapFactory.Options() | ||
| val thumbnailBitmap: Bitmap? | ||
| options.inSampleSize = 4 // pixel number reduced by a factor of 1/16 | ||
| val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options) | ||
| if (bitmap == null) { | ||
| closeQuietly(thumbnailReader) | ||
| return@submit | ||
| } | ||
|
|
||
| val thumbnailWidth = 100 | ||
| val thumbnailHeight = 100 | ||
| thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight) | ||
| if (thumbnailBitmap != null) { | ||
| storeThumbnail(diskCache, cacheKey, thumbnailBitmap) | ||
| } | ||
| closeQuietly(thumbnailReader) | ||
|
|
||
| cryptoFile.thumbnail = diskCache[cacheKey] | ||
| } catch (e: Exception) { | ||
| Timber.e(e, "Bitmap generation crashed") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Enhance error handling and resource management in startThumbnailGeneratorThread.
The startThumbnailGeneratorThread method is well-structured, but a few improvements can be made:
- The
BitmapFactory.decodeStreamcall can returnnullfor unsupported formats (e.g., SVG). This case should be handled explicitly. - The
thumbnailReaderis not always closed, which could lead to resource leaks. - Exception handling could be more specific and informative.
Consider the following improvements:
private fun startThumbnailGeneratorThread(cryptoFile: CryptoFile, diskCache: DiskLruCache, cacheKey: String, thumbnailReader: PipedInputStream): Future<*> {
return thumbnailExecutorService.submit {
try {
val options = BitmapFactory.Options()
- val thumbnailBitmap: Bitmap?
options.inSampleSize = 4 // pixel number reduced by a factor of 1/16
val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options)
if (bitmap == null) {
+ Timber.w("Failed to decode bitmap for file: ${cryptoFile.name}")
closeQuietly(thumbnailReader)
return@submit
}
val thumbnailWidth = 100
val thumbnailHeight = 100
val thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight)
if (thumbnailBitmap != null) {
storeThumbnail(diskCache, cacheKey, thumbnailBitmap)
+ cryptoFile.thumbnail = diskCache[cacheKey]
}
- closeQuietly(thumbnailReader)
-
- cryptoFile.thumbnail = diskCache[cacheKey]
- } catch (e: Exception) {
- Timber.e(e, "Bitmap generation crashed")
+ } catch (e: OutOfMemoryError) {
+ Timber.e(e, "Out of memory error while generating thumbnail for ${cryptoFile.name}")
+ } catch (e: IOException) {
+ Timber.e(e, "IO error while generating thumbnail for ${cryptoFile.name}")
+ } catch (e: Exception) {
+ Timber.e(e, "Unexpected error while generating thumbnail for ${cryptoFile.name}")
+ } finally {
+ closeQuietly(thumbnailReader)
}
}
}These changes improve error handling, ensure proper resource cleanup, and provide more specific error logging.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun startThumbnailGeneratorThread(cryptoFile: CryptoFile, diskCache: DiskLruCache, cacheKey: String, thumbnailReader: PipedInputStream): Future<*> { | |
| return thumbnailExecutorService.submit { | |
| try { | |
| val options = BitmapFactory.Options() | |
| val thumbnailBitmap: Bitmap? | |
| options.inSampleSize = 4 // pixel number reduced by a factor of 1/16 | |
| val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options) | |
| if (bitmap == null) { | |
| closeQuietly(thumbnailReader) | |
| return@submit | |
| } | |
| val thumbnailWidth = 100 | |
| val thumbnailHeight = 100 | |
| thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight) | |
| if (thumbnailBitmap != null) { | |
| storeThumbnail(diskCache, cacheKey, thumbnailBitmap) | |
| } | |
| closeQuietly(thumbnailReader) | |
| cryptoFile.thumbnail = diskCache[cacheKey] | |
| } catch (e: Exception) { | |
| Timber.e(e, "Bitmap generation crashed") | |
| } | |
| } | |
| } | |
| private fun startThumbnailGeneratorThread(cryptoFile: CryptoFile, diskCache: DiskLruCache, cacheKey: String, thumbnailReader: PipedInputStream): Future<*> { | |
| return thumbnailExecutorService.submit { | |
| try { | |
| val options = BitmapFactory.Options() | |
| options.inSampleSize = 4 // pixel number reduced by a factor of 1/16 | |
| val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options) | |
| if (bitmap == null) { | |
| Timber.w("Failed to decode bitmap for file: ${cryptoFile.name}") | |
| closeQuietly(thumbnailReader) | |
| return@submit | |
| } | |
| val thumbnailWidth = 100 | |
| val thumbnailHeight = 100 | |
| val thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight) | |
| if (thumbnailBitmap != null) { | |
| storeThumbnail(diskCache, cacheKey, thumbnailBitmap) | |
| cryptoFile.thumbnail = diskCache[cacheKey] | |
| } | |
| } catch (e: OutOfMemoryError) { | |
| Timber.e(e, "Out of memory error while generating thumbnail for ${cryptoFile.name}") | |
| } catch (e: IOException) { | |
| Timber.e(e, "IO error while generating thumbnail for ${cryptoFile.name}") | |
| } catch (e: Exception) { | |
| Timber.e(e, "Unexpected error while generating thumbnail for ${cryptoFile.name}") | |
| } finally { | |
| closeQuietly(thumbnailReader) | |
| } | |
| } | |
| } |
| private fun downloadAndGenerateThumbnails(visibleCloudFiles: List<CloudFileModel>) { | ||
| view?.showProgress( | ||
| visibleCloudFiles, // | ||
| ProgressModel( | ||
| progressStateModelMapper.toModel( // | ||
| DownloadState.download(visibleCloudFiles[0].toCloudNode()) | ||
| ), 0 | ||
| ) | ||
| ) | ||
| downloadFilesUseCase // | ||
| .withDownloadFiles(downloadFileUtil.createDownloadFilesFor(this, visibleCloudFiles)) // | ||
| .run(object : DefaultProgressAwareResultHandler<List<CloudFile>, DownloadState>() { | ||
| override fun onFinished() { | ||
| view?.hideProgress(visibleCloudFiles) | ||
| } | ||
|
|
||
| override fun onProgress(progress: Progress<DownloadState>) { | ||
| if (!progress.isOverallComplete) { | ||
| view?.showProgress( | ||
| cloudFileModelMapper.toModel(progress.state().file()), // | ||
| progressModelMapper.toModel(progress) | ||
| ) | ||
| } | ||
| if (progress.isCompleteAndHasState) { | ||
| val cloudFile = progress.state().file() | ||
| val cloudFileModel = cloudFileModelMapper.toModel(cloudFile) | ||
| view?.addOrUpdateCloudNode(cloudFileModel) | ||
| } | ||
| } | ||
|
|
||
| override fun onError(e: Throwable) { | ||
| view?.hideProgress(visibleCloudFiles) | ||
| super.onError(e) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
New method downloadAndGenerateThumbnails: Good implementation with a minor concern.
The method correctly implements the download and generation of thumbnails, including proper progress reporting and view updates. However, there's a potential issue with error handling:
In the onError callback, after calling super.onError(e), the method continues to hide the progress. This might lead to inconsistent UI state if the superclass method throws an exception or performs some critical error handling.
Consider refactoring the error handling like this:
override fun onError(e: Throwable) {
view?.hideProgress(visibleCloudFiles)
super.onError(e)
}This ensures that the progress is always hidden before any potential exception in the superclass method.
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.
Finally we got rid of the full duplication of the image by elaborating the thumbnail in stream with an ad-hoc Thread pool.