Gallery/Grid view #534
Gallery/Grid view #534
Conversation
…ng thumbnail from the DiskLruCache
Also using cloud.id to be able to distinguish between two files with same path but on different instances of the same cloud
…tFanta01/cryptomator-android into feature/thumbnail-playground
When onBackPressed(), or similar, were triggered the recycle/bind reused a ViewHolder with the selection mode already set
WalkthroughThe recent updates focus on enhancing functionality related to image thumbnails within the Cryptomator application. Key changes include the addition of thumbnail properties to files, improved handling of image files across different vault formats, and UI updates in various fragments to support these changes. Thumbnail management options have also been integrated into user settings, allowing for customizable thumbnail generation based on user preferences. Changes
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: 7
Out of diff range and nitpick comments (1)
presentation/src/main/java/org/cryptomator/presentation/ui/fragment/GalleryFragment.kt (1)
54-54: The constantCOLUMNSis hard-coded. Consider making this configurable through resources or settings to enhance flexibility.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (7)
presentation/src/main/res/drawable/rectangle_selection_mode.xmlis excluded by!**/*.xmlpresentation/src/main/res/layout/fragment_gallery_view.xmlis excluded by!**/*.xmlpresentation/src/main/res/layout/item_gallery_files_node.xmlis excluded by!**/*.xmlpresentation/src/main/res/values/arrays.xmlis excluded by!**/*.xmlpresentation/src/main/res/values/colors.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 (24)
- 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 (4 hunks)
- data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormatPre7.kt (2 hunks)
- presentation/src/main/java/org/cryptomator/presentation/di/component/ActivityComponent.java (2 hunks)
- presentation/src/main/java/org/cryptomator/presentation/intent/BrowseFilesIntent.java (1 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/AutoUploadChooseVaultPresenter.kt (1 hunks)
- presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt (4 hunks)
- presentation/src/main/java/org/cryptomator/presentation/presenter/SharedFilesPresenter.kt (1 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/activity/BrowseFilesActivity.kt (5 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/activity/VaultListActivity.kt (1 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/adapter/BrowseFilesAdapter.kt (4 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/adapter/GalleryFilesAdapter.kt (1 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 (11 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/FilesFragmentInterface.kt (1 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/GalleryFragment.kt (1 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.kt (6 hunks)
- presentation/src/main/java/org/cryptomator/presentation/util/DateHelper.kt (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 errors (1)
- data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormatPre7.kt (no review received)
Files skipped from review due to trivial changes (1)
- util/src/main/java/org/cryptomator/util/ThumbnailsOption.kt
Additional comments not posted (47)
presentation/src/main/java/org/cryptomator/presentation/intent/BrowseFilesIntent.java (1)
15-16: Addition ofvaultIdas an optional parameter is correctly implemented.presentation/src/main/java/org/cryptomator/presentation/model/CloudFileModel.kt (1)
14-14: Addition ofthumbnailproperty to handle thumbnail images for files is correctly implemented.data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoFile.kt (1)
16-16: Addition ofthumbnailproperty to store thumbnail files is correctly implemented.presentation/src/main/java/org/cryptomator/presentation/model/CloudNodeModel.kt (1)
3-3: Verify the necessity of theBitmapimport as there are no uses of it in the provided code changes.presentation/src/main/java/org/cryptomator/presentation/ui/fragment/FilesFragmentInterface.kt (1)
11-29: Addition of multiple abstract methods to enhance UI interactions and state management is correctly implemented.presentation/src/main/java/org/cryptomator/presentation/util/DateHelper.kt (1)
12-14: Addition of a lazy-initializeddateFormatterfor efficient date formatting is correctly implemented.presentation/src/main/java/org/cryptomator/presentation/ui/bottomsheet/FileSettingsBottomSheet.kt (1)
37-43: Addition of thumbnail handling in the UI is correctly implemented, enhancing the user experience by displaying visual content.util/src/main/java/org/cryptomator/util/file/LruFileCacheUtil.kt (1)
34-34: Addition ofLOCALcache option to efficiently manage local resources is correctly implemented.presentation/src/main/java/org/cryptomator/presentation/di/component/ActivityComponent.java (1)
79-79: EnsureGalleryFragmentis properly set up for dependency injection.This addition aligns with the existing architecture where fragments are injected to facilitate dependency management. Ensure that all necessary dependencies for
GalleryFragmentare provided in the corresponding module.presentation/src/main/java/org/cryptomator/presentation/presenter/AutoUploadChooseVaultPresenter.kt (1)
144-144: Ensure consistent handling ofvaultIdin intents.The addition of
vaultIdto the intent ensures that the correct vault is targeted for operations. This is crucial for the correct functioning of features that operate on specific vaults, such as auto-uploads.presentation/src/main/java/org/cryptomator/presentation/ui/activity/VaultListActivity.kt (1)
156-156: Ensure correct navigation to vault content.The method correctly constructs the intent with necessary parameters such as
vaultIdanddecryptedRoot, ensuring that the user is navigated to the appropriate content.presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt (1)
41-41: EnsureBrowseFilesFragmentcorrectly implementsFilesFragmentInterface.The change to make
BrowseFilesFragmentopen for extension and correctly implementFilesFragmentInterfaceis appropriate for allowing further customization and functionality extension.util/src/main/java/org/cryptomator/util/SharedPreferencesHandler.kt (1)
164-171: Ensure correct handling of thumbnail generation settings.The addition of handling for thumbnail generation settings in shared preferences is crucial for allowing users to control this feature based on their preferences. The implementation provides multiple options, enhancing user control.
presentation/src/main/java/org/cryptomator/presentation/ui/fragment/GalleryFragment.kt (5)
41-41: Ensure the layout resourceR.layout.fragment_gallery_viewexists.
44-48: Dependency injection is correctly set up forGalleryFilesAdapterandBrowseFilesPresenter.
56-60: Proper use of Kotlin property syntax forfolderwith custom getter and setter.
183-191: Usage of@SuppressLint("RestrictedApi")is justified due to the known Android bug. Ensure that this workaround is still necessary with the latest Android SDK updates.
284-287: The methodfileCanBeChosenuses a regex pattern to match file names. Ensure that the regex pattern is correctly defined and optimized for performance.presentation/src/main/java/org/cryptomator/presentation/presenter/SharedFilesPresenter.kt (1)
346-346: Ensure that thevaultIdis always correctly passed and handled in intents. This is crucial for maintaining the correct context when navigating between activities.presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.kt (4)
31-31: Add import forTHUMBNAIL_GENERATIONconstant.
51-51: Addition ofsetupThumbnailGenerationmethod call inonCreatePreferences.
259-259: SetthumbnailGenerationChangeListenerinonResume.
342-342: Addition ofTHUMBNAIL_GENERATIONconstant.presentation/src/main/java/org/cryptomator/presentation/ui/adapter/BrowseFilesAdapter.kt (5)
3-3: ImportBitmapFactoryfor handling bitmap operations.
31-32: ImportMimeTypeandMimeTypesfor handling MIME type operations.
52-53: InjectSharedPreferencesHandlerandMimeTypesintoBrowseFilesAdapter.
151-157: Implement thumbnail display for image files inbindNodeImage.This change allows thumbnails to be displayed for image files, enhancing the user interface by providing visual previews of images.
159-160: Check if a file is of image media type using MIME types.presentation/src/main/java/org/cryptomator/presentation/ui/adapter/GalleryFilesAdapter.kt (5)
3-8: Import necessary classes for handling views and bitmaps.
30-31: ImportMimeTypeandMimeTypesfor handling MIME type operations in the gallery view.
39-46: Constructor setup forGalleryFilesAdapterwith necessary dependencies.
157-164: Implement thumbnail display for image files in gallery view.This change enhances the gallery view by displaying thumbnails for image files, providing a more visually appealing user interface.
166-167: Check if a file is of image media type using MIME types in the gallery view.presentation/src/main/java/org/cryptomator/presentation/ui/activity/BrowseFilesActivity.kt (5)
52-53: Imports forFilesFragmentInterfaceandGalleryFragmenthave been added.This aligns with the PR's objective to integrate the new
GalleryFragmentinto the existing activity structure.
108-108: The methodcreateFragmenthas been modified to support dynamic fragment creation based on the folder and settings.This change is crucial for supporting the dynamic loading of either
BrowseFilesFragmentorGalleryFragmentbased on the context, which is a key feature of the new gallery view.
429-439: The methodcreateFragmentForhas been significantly refactored to decide between loading aBrowseFilesFragmentor aGalleryFragmentbased on whether the folder is marked for auto-upload.This is a smart use of polymorphism to handle different types of fragments dynamically. It directly supports the PR's goal of using
GalleryFragmentfor specific folders.
441-443: The methodisAutoUploadFolderchecks if the current folder is set for auto-uploads by comparing it against user settings.This method is essential for determining the context in which the
GalleryFragmentshould be used, ensuring that it is only used for folders intended for media file uploads.
565-565: The methodbrowseFilesFragmenthas been updated to return aFilesFragmentInterface.This update is necessary for the abstraction used in this activity, allowing it to interact with both
BrowseFilesFragmentandGalleryFragmentthrough a common interface.data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplDecorator.kt (5)
4-8: Imports for handling bitmaps and thumbnails have been added.These imports are necessary for the new functionality related to thumbnail generation and caching, aligning with the backend changes described in the PR summary.
72-82: Initialization ofsharedPreferencesHandler,diskLruCache, andthumbnailExecutorServicehas been added.These initializations are crucial for managing preferences and caching mechanisms for thumbnails, which is a significant part of the new functionality introduced in this PR.
Line range hint
372-399: Implementation of thumbnail generation and caching logic within thereadmethod.This implementation is a key part of the new functionality, ensuring that thumbnails are generated and cached efficiently. It uses a separate thread for thumbnail generation, which is good practice for performance.
434-455: The methodstartThumbnailGeneratorThreadhandles the generation of thumbnails in a separate thread.This method is well-implemented to handle potentially expensive image processing operations in a background thread, thus not blocking the main thread, which is crucial for maintaining application responsiveness.
468-492: The methodisGenerateThumbnailsEnabledchecks if thumbnail generation is enabled based on user preferences and file type.This method is essential for ensuring that thumbnails are only generated when appropriate, based on user settings and the media type of the files, which aligns with the user-configurable nature of the new feature.
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoImplVaultFormat7.kt (2)
168-179: Thumbnail handling logic appears correct, but verify supporting methods.Verification successful
The methods
isImageMediaTypeandgenerateCacheKeyare correctly defined inCryptoImplDecorator.ktand are inherited byCryptoImplVaultFormat7.kt, which extends this class. The usage of these methods in the thumbnail handling logic ofCryptoImplVaultFormat7.ktis therefore appropriate and correct.
isImageMediaTypeis defined in multiple locations, but the relevant definition for this context is inCryptoImplDecorator.kt.generateCacheKeyis also defined inCryptoImplDecorator.kt.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and correctness of `isImageMediaType` and `generateCacheKey` methods. # Test: Search for the methods in the codebase. Expect: Definitions or imports of these methods. rg --type kotlin $'fun isImageMediaType\\(' rg --type kotlin $'fun generateCacheKey\\('Length of output: 812
463-471: Cache deletion logic appears correct, but verify supporting methods.Verification successful
The verification process has confirmed the existence of the methods
generateCacheKeyandgetLruCacheForwithin theCryptoImplDecorator.ktfile. Both methods are defined asprotected, which implies they are accessible within the same module where they are declared, or from subclasses of the class in which they are declared.Given this information, the original review comment's concern about verifying the existence and correctness of these methods is addressed. The methods exist and are likely correctly implemented, as they are part of the same class or a closely related class (
CryptoImplDecorator.kt) that is used in theCryptoImplVaultFormat7.ktfile.Since the methods are found and there are no indications of incorrect implementation from the provided outputs, we can conclude that the cache deletion logic in the
deletemethod, which relies on these methods, is correctly supported.Final Response:
The methods
generateCacheKeyandgetLruCacheForhave been successfully verified as existing and correctly implemented in theCryptoImplDecorator.ktfile. The cache deletion logic in thedeletemethod ofCryptoImplVaultFormat7.ktis properly supported by these methods.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and correctness of `generateCacheKey` and `getLruCacheFor` methods. # Test: Search for the methods in the codebase. Expect: Definitions or imports of these methods. rg --type kotlin $'fun generateCacheKey\\(' rg --type kotlin $'fun getLruCacheFor\\('Length of output: 365
presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt (2)
517-517: Ensure theImagePreviewFilesStoreis properly initialized with valid parameters.Verification successful
The initialization of
ImagePreviewFilesStoreinBrowseFilesPresenter.ktusescloudFileNodesas the first parameter and the index of a specificcloudFilewithincloudFileNodesas the second parameter. This setup appears to be logically sound as it initializes the store with a list of nodes and a position index, which are typical and valid parameters for such a use case.Given this information, it can be concluded that
ImagePreviewFilesStoreis properly initialized with valid parameters in the context of theBrowseFilesPresenter.
- The
cloudFileNodesparameter provides the data context for the image previews.- The index parameter specifies the initial position in the list, which is essential for operations that might need to know the current focus within the list.
This setup aligns with common practices in handling collections and indices in programming, especially in a feature that deals with previews where both the collection of items and the current item's index are crucial.
Conclusion: The initialization of
ImagePreviewFilesStoreinBrowseFilesPresenter.ktis verified to be correct and uses appropriate parameters.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ImagePreviewFilesStore` is initialized with valid parameters. # Test: Search for the initialization of `ImagePreviewFilesStore`. Expect: Valid parameters. rg --type kotlin $'ImagePreviewFilesStore'Length of output: 1566
Script:
#!/bin/bash # Description: Extract the specific line from BrowseFilesPresenter.kt where ImagePreviewFilesStore is initialized. # Extract the line and a few lines around it for context. rg --type kotlin --context 2 'ImagePreviewFilesStore' presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.ktLength of output: 601
1116-1117: Ensure thevaultIdis correctly used in the intent builder.
| private val thumbnailGenerationChangeListener = Preference.OnPreferenceChangeListener { _, newValue -> | ||
| // TODO ... | ||
| true | ||
| } |
There was a problem hiding this comment.
Incomplete implementation of thumbnailGenerationChangeListener.
Please complete the implementation of the thumbnailGenerationChangeListener. This is crucial for handling changes in user preferences regarding thumbnail generation.
| private fun setupThumbnailGeneration() { | ||
| val preference = findPreference(THUMBNAIL_GENERATION) as Preference? | ||
| // TODO ... | ||
| } |
There was a problem hiding this comment.
Incomplete setup for thumbnail generation preference.
Please complete the implementation of the setupThumbnailGeneration method. This method should properly configure the preference related to thumbnail generation.
| val vauldId = view?.folder?.vault()?.vaultId | ||
| val browseFilesIntentBuilder = Intents.browseFilesIntent() // |
There was a problem hiding this comment.
Check the variable name typo: vauldId should be vaultId.
- val vauldId = view?.folder?.vault()?.vaultId
+ val vaultId = view?.folder?.vault()?.vaultIdCommittable 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.
| val vauldId = view?.folder?.vault()?.vaultId | |
| val browseFilesIntentBuilder = Intents.browseFilesIntent() // | |
| val vaultId = view?.folder?.vault()?.vaultId | |
| val browseFilesIntentBuilder = Intents.browseFilesIntent() // |
There was a problem hiding this comment.
@SailReal, I've made the necessary change to correct the typo from vauldId to vaultId.
val vaultId = view?.folder?.vault()?.vaultId
val browseFilesIntentBuilder = Intents.browseFilesIntent() //This should resolve the issue. Let me know if there's anything else you need.
New Learnings Added
User: SailReal
PR: cryptomator/android#534
File: presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt:1102-1103
Timestamp: 2024-05-14T11:00:59.728Z
Learning: In the `BrowseFilesPresenter` class, ensure the variable name `vaultId` is correctly spelled.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
| @Inject | ||
| lateinit var browseFilesPresenter: BrowseFilesPresenter | ||
|
|
||
| private var navigationMode: ChooseCloudNodeSettings.NavigationMode? = null |
There was a problem hiding this comment.
Consider initializing navigationMode to a default value to ensure it's never null when accessed before being set.
- private var navigationMode: ChooseCloudNodeSettings.NavigationMode? = null
+ private var navigationMode: ChooseCloudNodeSettings.NavigationMode = BROWSE_FILESCommittable 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.
| private var navigationMode: ChooseCloudNodeSettings.NavigationMode? = null | |
| private var navigationMode: ChooseCloudNodeSettings.NavigationMode = BROWSE_FILES |
| private val refreshListener = SwipeRefreshLayout.OnRefreshListener { browseFilesPresenter.onRefreshTriggered(folder) } | ||
|
|
||
| private val nodeClickListener = object : GalleryFilesAdapter.ItemClickListener { | ||
| override fun onFolderClicked(cloudFolderModel: CloudFolderModel) { | ||
| browseFilesPresenter.onFolderClicked(cloudFolderModel) | ||
| filterText = "" | ||
| browseFilesPresenter.invalidateOptionsMenu() | ||
| } | ||
|
|
||
| override fun onFileClicked(cloudNodeModel: CloudFileModel) { | ||
| if (fileCanBeChosen(cloudNodeModel)) { | ||
| browseFilesPresenter.onFileChosen(cloudNodeModel) | ||
| } else { | ||
| browseFilesPresenter.onFileClicked(cloudNodeModel) | ||
| } | ||
| } | ||
|
|
||
| override fun onNodeSettingsClicked(cloudNodeModel: CloudNodeModel<*>) { | ||
| browseFilesPresenter.onNodeSettingsClicked(cloudNodeModel) | ||
| } | ||
|
|
||
| override fun onNodeLongClicked() { | ||
| browseFilesPresenter.onSelectionModeActivated() | ||
| } | ||
|
|
||
| override fun onSelectedNodesChanged(selectedNodes: Int) { | ||
| browseFilesPresenter.onSelectedNodesChanged(selectedNodes) | ||
| } | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Listeners for UI events in GalleryFragment.kt do not appear to use runOnUiThread for executing UI updates on the main thread. This could potentially lead to thread safety issues when updating the UI from background threads.
GalleryFragment.kt: Ensure that all UI updates in listeners are wrapped withrunOnUiThreadto maintain thread safety.
Analysis chain
Listeners for UI events are well implemented. Ensure that all UI updates are performed on the main thread to avoid potential issues with UI thread safety.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that UI updates in listeners are performed on the main thread.
# Test: Search for UI updates in listeners. Expect: UI updates wrapped in main thread calls.
rg --type kotlin 'runOnUiThread'
Length of output: 157
| override fun setupView() { | ||
| setupNavigationMode() | ||
|
|
||
| floatingActionButton.setOnClickListener { browseFilesPresenter.onAddContentClicked() } | ||
| chooseLocationButton.setOnClickListener { browseFilesPresenter.onFolderChosen(folder) } | ||
|
|
||
| swipeRefreshLayout.setColorSchemeColors(ContextCompat.getColor(context(), R.color.colorPrimary)) | ||
| swipeRefreshLayout.setOnRefreshListener(refreshListener) | ||
|
|
||
| cloudNodesAdapter.setCallback(nodeClickListener) | ||
| cloudNodesAdapter.setChooseCloudNodeSettings(chooseCloudNodeSettings) | ||
| navigationMode?.let { cloudNodesAdapter.updateNavigationMode(it) } | ||
|
|
||
|
|
||
| recyclerView.layoutManager = GridLayoutManager(context(), COLUMNS) | ||
| recyclerView.adapter = cloudNodesAdapter | ||
| recyclerView.setHasFixedSize(true) | ||
|
|
||
| val spacing = resources.getDimensionPixelSize(global_padding) / 4 | ||
|
|
||
| // bottom = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 88f, resources.displayMetrics).toInt() | ||
| recyclerView.setPadding(spacing, spacing, spacing, spacing) | ||
| recyclerView.clipToPadding = false | ||
| recyclerView.clipChildren = false | ||
|
|
||
| recyclerView.addItemDecoration(object : RecyclerView.ItemDecoration() { | ||
| override fun getItemOffsets(outRect : Rect, view : View, parent : RecyclerView, state : RecyclerView.State) { | ||
| outRect.set(spacing, spacing, spacing, spacing) | ||
| } | ||
| }) | ||
|
|
||
| browseFilesPresenter.onFolderRedisplayed(folder) | ||
|
|
||
| when { | ||
| !hasCloudNodeSettings() -> setupViewForBrowseFilesMode() | ||
| isSelectionMode(FOLDERS_ONLY) -> setupViewForFolderSelection() | ||
| isSelectionMode(FILES_ONLY) -> setupViewForFilesSelection() | ||
| isNavigationMode(SELECT_ITEMS) -> setupViewForNodeSelectionMode() | ||
| } | ||
| } |
There was a problem hiding this comment.
The method setupView correctly configures UI components and sets up listeners. However, consider extracting complex conditional logic into separate methods for better readability and maintainability.
- when {
- !hasCloudNodeSettings() -> setupViewForBrowseFilesMode()
- isSelectionMode(FOLDERS_ONLY) -> setupViewForFolderSelection()
- isSelectionMode(FILES_ONLY) -> setupViewForFilesSelection()
- isNavigationMode(SELECT_ITEMS) -> setupViewForNodeSelectionMode()
- }
+ setupViewBasedOnSettings()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.
| override fun setupView() { | |
| setupNavigationMode() | |
| floatingActionButton.setOnClickListener { browseFilesPresenter.onAddContentClicked() } | |
| chooseLocationButton.setOnClickListener { browseFilesPresenter.onFolderChosen(folder) } | |
| swipeRefreshLayout.setColorSchemeColors(ContextCompat.getColor(context(), R.color.colorPrimary)) | |
| swipeRefreshLayout.setOnRefreshListener(refreshListener) | |
| cloudNodesAdapter.setCallback(nodeClickListener) | |
| cloudNodesAdapter.setChooseCloudNodeSettings(chooseCloudNodeSettings) | |
| navigationMode?.let { cloudNodesAdapter.updateNavigationMode(it) } | |
| recyclerView.layoutManager = GridLayoutManager(context(), COLUMNS) | |
| recyclerView.adapter = cloudNodesAdapter | |
| recyclerView.setHasFixedSize(true) | |
| val spacing = resources.getDimensionPixelSize(global_padding) / 4 | |
| // bottom = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 88f, resources.displayMetrics).toInt() | |
| recyclerView.setPadding(spacing, spacing, spacing, spacing) | |
| recyclerView.clipToPadding = false | |
| recyclerView.clipChildren = false | |
| recyclerView.addItemDecoration(object : RecyclerView.ItemDecoration() { | |
| override fun getItemOffsets(outRect : Rect, view : View, parent : RecyclerView, state : RecyclerView.State) { | |
| outRect.set(spacing, spacing, spacing, spacing) | |
| } | |
| }) | |
| browseFilesPresenter.onFolderRedisplayed(folder) | |
| when { | |
| !hasCloudNodeSettings() -> setupViewForBrowseFilesMode() | |
| isSelectionMode(FOLDERS_ONLY) -> setupViewForFolderSelection() | |
| isSelectionMode(FILES_ONLY) -> setupViewForFilesSelection() | |
| isNavigationMode(SELECT_ITEMS) -> setupViewForNodeSelectionMode() | |
| } | |
| } | |
| override fun setupView() { | |
| setupNavigationMode() | |
| floatingActionButton.setOnClickListener { browseFilesPresenter.onAddContentClicked() } | |
| chooseLocationButton.setOnClickListener { browseFilesPresenter.onFolderChosen(folder) } | |
| swipeRefreshLayout.setColorSchemeColors(ContextCompat.getColor(context(), R.color.colorPrimary)) | |
| swipeRefreshLayout.setOnRefreshListener(refreshListener) | |
| cloudNodesAdapter.setCallback(nodeClickListener) | |
| cloudNodesAdapter.setChooseCloudNodeSettings(chooseCloudNodeSettings) | |
| navigationMode?.let { cloudNodesAdapter.updateNavigationMode(it) } | |
| recyclerView.layoutManager = GridLayoutManager(context(), COLUMNS) | |
| recyclerView.adapter = cloudNodesAdapter | |
| recyclerView.setHasFixedSize(true) | |
| val spacing = resources.getDimensionPixelSize(global_padding) / 4 | |
| // bottom = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 88f, resources.displayMetrics).toInt() | |
| recyclerView.setPadding(spacing, spacing, spacing, spacing) | |
| recyclerView.clipToPadding = false | |
| recyclerView.clipChildren = false | |
| recyclerView.addItemDecoration(object : RecyclerView.ItemDecoration() { | |
| override fun getItemOffsets(outRect : Rect, view : View, parent : RecyclerView, state : RecyclerView.State) { | |
| outRect.set(spacing, spacing, spacing, spacing) | |
| } | |
| }) | |
| browseFilesPresenter.onFolderRedisplayed(folder) | |
| setupViewBasedOnSettings() | |
| } |
| override fun show(nodes: List<CloudNodeModel<*>>?) { | ||
| cloudNodesAdapter.clear() | ||
| cloudNodesAdapter.addAll(cloudNodesAdapter.filterNodes(nodes, filterText)) | ||
| updateEmptyFolderHint() | ||
| } | ||
|
|
||
| override fun showProgress(nodes: List<CloudNodeModel<*>>?, progress: ProgressModel?) { | ||
| nodes?.forEach { node -> | ||
| showProgress(node, progress) | ||
| } | ||
| } | ||
|
|
||
| override fun showProgress(node: CloudNodeModel<*>?, progress: ProgressModel?) { | ||
| val viewHolder = viewHolderFor(node) | ||
| if (viewHolder.isPresent) { | ||
| viewHolder.get().showProgress(progress) | ||
| } else { | ||
| node?.progress = progress | ||
| node?.let { addOrUpdate(it) } | ||
| } | ||
| } | ||
|
|
||
| override fun hideProgress(nodes: List<CloudNodeModel<*>>?) { | ||
| nodes?.forEach { node -> | ||
| hideProgress(node) | ||
| } | ||
| } | ||
|
|
||
| override fun hideProgress(cloudNode: CloudNodeModel<*>?) { | ||
| val viewHolder = viewHolderFor(cloudNode) | ||
| if (viewHolder.isPresent) { | ||
| viewHolder.get().hideProgress() | ||
| } else { | ||
| cloudNode?.progress = ProgressModel.COMPLETED | ||
| cloudNode?.let { addOrUpdate(it) } | ||
| } | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The search for synchronization mechanisms around ProgressModel updates in the Kotlin files of the project revealed that the term "synchronized" is used in two different files (Presenter.kt and LogRotator.kt), but neither of these usages are related to the ProgressModel or its updates. This indicates that there are no explicit synchronization mechanisms in place for the updates to ProgressModel as used in the GalleryFragment.kt.
Given this information, it appears that the updates to ProgressModel are not explicitly thread-safe, which could potentially lead to issues if these objects are accessed from multiple threads concurrently.
Final Response:
The verification process did not find any synchronization mechanisms around updates to ProgressModel in the provided context. If ProgressModel is accessed from multiple threads, this might lead to thread safety issues. It is recommended to implement appropriate synchronization if concurrent access is expected.
- File:
presentation/src/main/java/org/cryptomator/presentation/ui/fragment/GalleryFragment.kt - Concern: Updates to
ProgressModelare not thread-safe.
Analysis chain
The methods for showing and hiding progress are implemented correctly. Ensure that the ProgressModel updates are thread-safe if accessed from multiple threads.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify thread safety of `ProgressModel` updates.
# Test: Search for synchronization mechanisms around `ProgressModel` updates. Expect: Use of locks or synchronized blocks.
rg --type kotlin 'synchronized'
Length of output: 295
There was a problem hiding this comment.
Thank you so much for this contribution ❤️!
Besides the comments in the code, here are some general thoughts:
- We should remove the automatic detection of where to show the GridView and where not, and instead add a menu item to toggle the layout. Then pass this value to the next Fragment so that we only switch back when the user
- toggles the menu
- destroys the BrowseFilesActivity in which this mode is stored.
- To avoid the thumbnail "jumping around" to show progress when clicked, we can show the progress section at the bottom, but in the thumbnail.
- We should always show the name of the file/folder below it. We shrink it to the width of the icon, but we need it. Otherwise we can't know the difference between two folders or files that aren't images.
- We should also decrease the size of the thumbnail and add a
⋮next to the node to interact with it (the>from the normal view) - I'm still thinking whether we should add some overhead and reuse more code or accept that
BrowseFilesAdapter/GalleryFilesAdapter,BrowseFilesFragment/GalleryFragmentandBrowseFilesAdapter/GalleryFilesAdapterdo more or less the same thing. If we accept it, I think we should move a lot of code into a base class. Once that's decided, I'll have a closer look at those classes - We will provide some high resolution placeholders for the file icons.
- The code is formatted almost everywhere, but some files are not completely 😅
- Always use brackets in if statements
| val vauldId = view?.folder?.vault()?.vaultId | ||
| val browseFilesIntentBuilder = Intents.browseFilesIntent() // |
| // descritto da R.layout.item_gallery_files_node | ||
| // sono state importate le sue componenti | ||
| // kotlinx.android.synthetic.main.item_gallery_files_node.view.galleryCloudNodeImage |
There was a problem hiding this comment.
Translate it if we need this comment, otherwise remove it
| // durante il rebind sta probabilmente riutilizzando lo stesso oggetto grafico (itemView) | ||
| // di un precente cloudNode che era stato selezionato | ||
| // e.g. se l'item 22 viene selezionato, cambia il foreground e quando viene | ||
| // ribindato con l'indice 0 rimane il foregound sbagliato! |
There was a problem hiding this comment.
Translate it if we need this comment, otherwise remove it
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (19)
presentation/src/main/res/layout/item_gallery_files_node.xml (3)
3-10: LGTM! Consider removing redundant visibility attribute.The root LinearLayout is well-structured for a gallery item container. The use of
match_parentfor width andwrap_contentfor height is appropriate, allowing the item to fill the width of its parent while adjusting its height based on content.Consider removing the
android:visibility="visible"attribute as it's the default value and redundant.
28-46: LGTM! Consider adding content description for accessibility.The layout for the cloud file progress indicator is well-structured. The use of an
<include>tag for the progress details promotes modularity and reusability.To improve accessibility, consider adding a content description to the progress icon:
<ImageView android:id="@+id/progress_icon" android:layout_width="16dp" android:layout_height="16dp" android:contentDescription="@string/progress_icon_description" app:tint="@color/textColorLight" />Ensure you add the corresponding string resource for the description.
1-47: Overall, the layout implements the gallery view feature well.This new layout file successfully implements the gallery item view as described in the PR objectives. It provides a structure for displaying images in a grid layout with support for progress indicators for cloud file operations.
Consider the following architectural improvements:
- Evaluate the need for nested layouts. In some cases, flattening the hierarchy could improve performance.
- If this layout is used in a RecyclerView, ensure that view binding or view holder pattern is implemented efficiently to avoid performance issues with large galleries.
- Consider creating a custom view that encapsulates this layout logic, which could simplify usage and improve reusability across the app.
These suggestions aim to enhance the long-term maintainability and performance of the gallery view feature.
presentation/src/main/res/layout/fragment_gallery_view.xml (3)
13-26: Verify RecyclerView bottom padding and consider extracting as a dimension resourceThe structure with SwipeRefreshLayout and included RecyclerView looks good and promotes reusability. However:
- The bottom padding of 88dp seems large. Verify if this value is necessary to accommodate the bottom toolbar or other UI elements.
- Consider extracting the padding value to a dimension resource for better maintainability and consistency across the app.
You could refactor the padding as follows:
- android:paddingBottom="88dp" /> + android:paddingBottom="@dimen/gallery_recycler_view_bottom_padding" />Then define the dimension in a
dimens.xmlfile:<resources> <dimen name="gallery_recycler_view_bottom_padding">88dp</dimen> </resources>This approach allows for easier adjustments and maintains consistency across different screen sizes if needed.
28-32: Add ID to retry view for consistencyThe inclusion of retry and empty folder views is good for providing user feedback. For consistency and to allow easier programmatic access if needed:
- Consider adding an ID to the retry view, similar to how the empty folder view has one.
You could modify the retry view inclusion as follows:
- <include layout="@layout/view_retry" /> + <include + android:id="@+id/view_retry" + layout="@layout/view_retry" />This change will make it consistent with the empty folder view and potentially easier to reference in code if needed.
50-54: Add comment explaining FAB visibility logicThe implementation of the floating action button (FAB) looks good:
- Initially hidden state allows for dynamic showing/hiding based on user interaction or scroll position.
- Anchoring to the RecyclerView ensures proper positioning during scrolling.
Consider adding a comment to explain the visibility logic for better maintainability:
<include android:id="@+id/floating_action_button" layout="@layout/floating_action_button_layout" + <!-- TODO: FAB is initially hidden and shown based on [describe condition] --> android:visibility="gone" app:layout_anchor="@id/recycler_view" />This comment will help other developers understand when and why the FAB becomes visible.
presentation/src/main/java/org/cryptomator/presentation/ui/activity/VaultListActivity.kt (1)
165-165: LGTM! Consider adding a comment for clarity.The addition of
withVaultId(vault.vaultId)to thebrowseFilesIntent()call is a good improvement. It enhances the intent's specificity by directly associating it with the accessed vault, which aligns well with the PR's objective of implementing a new gallery view feature.Consider adding a brief comment explaining the purpose of including the vault ID, especially if it's related to the new GalleryFragment functionality. This would improve code readability and maintainability. For example:
// Include vault ID to support new GalleryFragment functionality vaultListPresenter.startIntent(browseFilesIntent().withVaultId(vault.vaultId).withTitle(vault.name).withFolder(decryptedRoot))presentation/src/main/res/xml/preferences.xml (1)
123-133: LGTM! Consider adding a comment for clarity.The new PreferenceCategory and ListPreference for thumbnail generation are well-structured and align with the PR objectives. The use of string resources and arrays for entries is commendable, facilitating localization and easy modification.
Consider adding a brief XML comment above the PreferenceCategory to explain its purpose and how it relates to the gallery view feature. This can help other developers understand the context quickly. For example:
<!-- Thumbnail generation settings for gallery view feature --> <PreferenceCategory android:title="@string/screen_settings_thumbnail_generation"> ... </PreferenceCategory>util/src/main/java/org/cryptomator/util/SharedPreferencesHandler.kt (1)
164-171: LGTM! Consider a minor readability improvement.The
generateThumbnails()method is well-implemented and correctly maps the stored preference to theThumbnailsOptionenum. Good job on using awhenexpression for clear and concise mapping.For slightly improved readability, you could use the
ThumbnailsOption.valueOf()method to directly convert the string to an enum value. Here's an optional refactor:fun generateThumbnails(): ThumbnailsOption { val value = defaultSharedPreferences.getValue(THUMBNAIL_GENERATION, "NEVER") return try { ThumbnailsOption.valueOf(value) } catch (e: IllegalArgumentException) { ThumbnailsOption.NEVER } }This approach reduces duplication and automatically handles any new enum values that might be added in the future.
presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.kt (1)
Action Required: Complete the Thumbnail Generation Functionality
The
SettingsFragment.ktcontains TODO comments indicating incomplete implementations for thumbnail generation. Please address the following:
- Complete the
thumbnailGenerationChangeListenerimplementation.- Finish the
setupThumbnailGenerationmethod.🔗 Analysis chain
Line range hint
1-344: Overall assessment: Thumbnail generation functionality partially implemented.The changes introduce new functionality for thumbnail generation in the
SettingsFragment. While the overall structure and placement of new code are correct, there are still incomplete implementations that need to be addressed:
- The
thumbnailGenerationChangeListenerneeds to be fully implemented.- The
setupThumbnailGenerationmethod needs to be completed.Please finish these implementations to complete the thumbnail generation feature.
To ensure all necessary parts are implemented, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for TODO comments and incomplete implementations in SettingsFragment.kt # Test: Search for TODO comments and empty method bodies rg --type kotlin 'TODO|= \{\s*\}' presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.ktLength of output: 151
presentation/src/main/java/org/cryptomator/presentation/ui/adapter/BrowseFilesAdapter.kt (2)
150-152: LGTM: New isImageMediaType methodThe
isImageMediaTypemethod is well-implemented and correctly determines if a file is an image based on its MIME type. Good job on handling potential null values frommimeTypes.fromFilename().For improved readability, consider extracting the MIME type determination to a separate line:
private fun isImageMediaType(filename: String): Boolean { - return (mimeTypes.fromFilename(filename) ?: MimeType.WILDCARD_MIME_TYPE).mediatype == "image" + val mimeType = mimeTypes.fromFilename(filename) ?: MimeType.WILDCARD_MIME_TYPE + return mimeType.mediatype == "image" }This change makes the method easier to read and understand at a glance.
Line range hint
1-516: Summary of BrowseFilesAdapter.kt changesThe changes to
BrowseFilesAdapter.ktsuccessfully implement thumbnail handling for image files, aligning with the PR objectives. Here's a summary of the main points:
- The addition of
BitmapFactoryimport andMimeTypesparameter to the constructor are appropriate for the new functionality.- The
bindNodeImagemethod has been updated to handle image thumbnails, but there's a potential null pointer exception that should be addressed.- The new
isImageMediaTypemethod is well-implemented, correctly determining if a file is an image based on its MIME type.Overall, the changes maintain good coding practices and integrate well with the existing adapter structure. Address the potential null pointer exception, and consider the suggested minor improvements for readability.
As the adapter now handles more complex logic for determining and displaying file types, consider if some of this functionality could be moved to a separate helper class in the future. This would help maintain the single responsibility principle and make the adapter easier to maintain as it grows.
presentation/src/main/res/values/strings.xml (1)
648-649: LGTM: Thumbnail settings strings added.The new string resources for the thumbnail settings section and dialog title are well-defined and consistent with the earlier additions. They provide appropriate labels for the thumbnail generation feature in the app's settings.
Consider adding a brief comment above these new strings to group them with the other thumbnail-related strings (lines 635-637) for better organization. For example:
<!-- Thumbnail generation settings --> <string name="thumbnail_generation_never">Never</string> <string name="thumbnail_generation_file">Per File</string> <string name="thumbnail_generation_folder">Per Folder</string> <string name="screen_settings_thumbnail_generation">Thumbnails</string> <string name="dialog_thumbnail_generation_title">Thumbnail generation</string>presentation/src/main/java/org/cryptomator/presentation/ui/fragment/BrowseFilesFragment.kt (4)
Line range hint
46-50: Consider usingParcelableinstead ofSerializablefor argumentsCurrently, the
folderproperty usesgetSerializableandputSerializable, which can be less efficient and are discouraged in favor ofParcelable. To improve performance and comply with best practices, consider makingCloudFolderModelimplementParcelableand updating the code accordingly.Here's how you could modify the code:
override var folder: CloudFolderModel get() = requireArguments().getParcelable(ARG_FOLDER) as CloudFolderModel set(updatedFolder) { arguments?.putParcelable(ARG_FOLDER, updatedFolder) }
102-102: Remove unused commented-out codeThe commented-out line setting the
recyclerViewto use aGridLayoutManagerappears to be leftover code. If it's no longer needed, consider removing it to keep the codebase clean and maintainable.
Line range hint
218-223: Clarify the logic inselectAllItemsmethodThe
selectAllItemsmethod checks if there are unselected nodes and toggles the selection state of all nodes based on this check. This behavior is more akin to a "toggle all" function rather than explicitly selecting all items. To make the method's purpose clearer, consider renaming it totoggleSelectAllItemsor modifying it to always select all items.To ensure the method selects all items, you could update it as follows:
override fun selectAllItems() { cloudNodesAdapter.renderedCloudNodes().forEach { node -> selectNode(node, true) } }
Line range hint
276-284: Handle all navigation modes innavigationModeChangedThe
navigationModeChangedmethod currently handles onlySELECT_ITEMSandBROWSE_FILESmodes. If there are other possible navigation modes, they are not accounted for, which could lead to unintended behavior. Consider using awhenexpression to handle all potential modes or adding anelseclause to manage unforeseen cases.Here's how you might refactor the method:
override fun navigationModeChanged(navigationMode: ChooseCloudNodeSettings.NavigationMode) { updateNavigationMode(navigationMode) when (navigationMode) { SELECT_ITEMS -> setupViewForNodeSelectionMode() BROWSE_FILES -> setupViewForBrowseFilesMode() else -> { // Handle other navigation modes or log a warning } } }presentation/src/main/java/org/cryptomator/presentation/ui/adapter/GalleryFilesAdapter.kt (2)
240-243: Remove commented-out code to enhance maintainabilityThere are several lines of commented-out code starting at line 240. If this code is no longer needed, consider removing it to improve code clarity and maintainability.
402-409: Eliminate deprecated commented-out code in 'apply' methodThe
applymethod within theFileDetailsclass contains multiple lines of commented-out code. Removing unnecessary commented code enhances readability and reduces confusion for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- presentation/src/main/java/org/cryptomator/presentation/ui/activity/BrowseFilesActivity.kt (5 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/activity/VaultListActivity.kt (1 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/adapter/BrowseFilesAdapter.kt (3 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/adapter/GalleryFilesAdapter.kt (1 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 (11 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/GalleryFragment.kt (1 hunks)
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.kt (6 hunks)
- presentation/src/main/res/layout/fragment_gallery_view.xml (1 hunks)
- presentation/src/main/res/layout/item_gallery_files_node.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)
🔇 Additional comments (21)
presentation/src/main/res/layout/fragment_gallery_view.xml (3)
1-56: Overall assessment: Well-structured layout with minor improvement suggestionsThe
fragment_gallery_view.xmllayout is well-organized and effectively implements the gallery view feature described in the PR objectives. Key points:
- Proper use of SwipeRefreshLayout for pull-to-refresh functionality.
- Inclusion of RecyclerView for displaying gallery items.
- Feedback views (retry, empty folder) for various states.
- Bottom toolbar for additional file browsing functionality.
- Floating action button for dynamic actions.
The suggested improvements are minor and focus on consistency, clarity, and maintainability. Once these are addressed, the layout will be in excellent shape for the new gallery view feature.
34-46: Verify content of included browse files layoutThe structure for the bottom toolbar looks good:
- The use of RelativeLayout for positioning the toolbar at the bottom is appropriate.
- Including a separate layout for file browsing functionality promotes modularity.
To ensure the included layout contains the expected elements for file browsing:
#!/bin/bash # Description: Verify the content of the browse files layout # Test: Check the content of the view_browses_files_extra_text_and_button layout cat presentation/src/main/res/layout/view_browses_files_extra_text_and_button.xmlPlease review the output to confirm that the included layout contains the necessary elements for file browsing functionality.
2-7: Verify custom SlidingCoordinatorLayout and consider adding commentsThe use of a custom
SlidingCoordinatorLayoutsuggests advanced scrolling behavior. To improve maintainability:
- Ensure that the
org.cryptomator.presentation.ui.layout.SlidingCoordinatorLayoutis properly implemented and tested.- Consider adding a comment explaining the purpose and behavior of this custom view.
- The
appbar_scrolling_view_behavioris set, but there's no visible app bar in this layout. Verify if this is intentional or if an app bar should be added.presentation/src/main/java/org/cryptomator/presentation/ui/bottomsheet/FileSettingsBottomSheet.kt (3)
Line range hint
1-85: Summary of FileSettingsBottomSheet.kt reviewThe changes to
FileSettingsBottomSheet.ktalign well with the PR objectives of implementing a gallery view feature. The addition of thumbnail support enhances the visual representation of files in the bottom sheet.Key points from the review:
- Thumbnail loading implementation can be improved for better performance and error handling.
- Minor refactoring suggestions were provided to enhance code organization and memory management.
- The overall structure and functionality of the class are appropriate for its purpose.
These changes contribute positively to the user experience by providing a more visually appealing representation of files, especially in the context of the new gallery view feature.
Line range hint
1-85: Consider implementing minor improvements for better code organization and memory managementWhile the overall structure of the
FileSettingsBottomSheetclass is good, here are a few suggestions for improvement:
Memory Management: The
Callbackinterface is defined as an inner interface. Consider making it a companion object to prevent potential memory leaks, especially if the bottom sheet is long-lived.Null Safety: Use Kotlin's null safety features more extensively. For example, replace
as CloudFileModelwithas? CloudFileModeland handle the null case.Constants: Move the file extension checks into constants for better maintainability.
Here's how you might implement these suggestions:
class FileSettingsBottomSheet : BaseBottomSheet<FileSettingsBottomSheet.Callback, DialogBottomSheetFileSettingsBinding>(DialogBottomSheetFileSettingsBinding::inflate) { companion object { private const val FILE_ARG = "file" private const val PARENT_FOLDER_PATH_ARG = "parentFolderPath" private val TEXT_FILE_EXTENSIONS = listOf(".txt", ".md", ".todo") fun newInstance(cloudFileModel: CloudFileModel, parentFolderPath: String): FileSettingsBottomSheet { return FileSettingsBottomSheet().apply { arguments = Bundle().apply { putSerializable(FILE_ARG, cloudFileModel) putString(PARENT_FOLDER_PATH_ARG, parentFolderPath) } } } interface Callback { fun onExportFileClicked(cloudFile: CloudFileModel) fun onRenameFileClicked(cloudFile: CloudFileModel) fun onDeleteNodeClicked(cloudFile: CloudNodeModel<*>) fun onShareFileClicked(cloudFile: CloudFileModel) fun onMoveFileClicked(cloudFile: CloudFileModel) fun onOpenWithTextFileClicked(cloudFile: CloudFileModel) } } override fun setupView() { val cloudFileModel = requireArguments().getSerializable(FILE_ARG) as? CloudFileModel ?: return // Handle error case val parentFolderPath = requireArguments().getString(PARENT_FOLDER_PATH_ARG) // ... (thumbnail loading code) ... binding.tvFileName.text = cloudFileModel.name binding.tvFilePath.text = parentFolderPath if (cloudFileModel.name.lowercase().endsWith(TEXT_FILE_EXTENSIONS)) { binding.openWithText.visibility = View.VISIBLE binding.openWithText.setOnClickListener { callback?.onOpenWithTextFileClicked(cloudFileModel) dismiss() } } // ... (rest of the click listeners) ... } }These changes improve code organization, null safety, and maintainability.
To ensure these changes don't conflict with existing code, run:
#!/bin/bash # Check for other usages of the Callback interface grep -r "FileSettingsBottomSheet.Callback" .If there are no conflicts, these changes can be safely implemented.
29-34: 🛠️ Refactor suggestionEnhance thumbnail loading implementation
The addition of thumbnail support is a great improvement to the visual representation of files. However, there are a few areas that could be enhanced:
Performance: The
BitmapFactory.decodeFile()call is performed on the main thread, which could lead to UI freezes for large thumbnails. Consider using an asynchronous loading mechanism or a library like Glide or Picasso for efficient image loading.Error Handling: Add error handling for the thumbnail decoding process to gracefully handle any issues that may occur during decoding.
Memory Management: For large bitmaps, consider using
BitmapFactory.Optionsto sample the image down to a reasonable size that fits theImageViewdimensions to avoid potential OutOfMemoryErrors.Here's a suggested implementation using Glide (assuming it's already a project dependency):
import com.bumptech.glide.Glide import com.bumptech.glide.load.engine.DiskCacheStrategy // ... cloudFileModel.thumbnail?.let { thumbnailFile -> Glide.with(binding.ivFileImage.context) .load(thumbnailFile) .diskCacheStrategy(DiskCacheStrategy.NONE) .error(cloudFileModel.icon.iconResource) .into(binding.ivFileImage) } ?: binding.ivFileImage.setImageResource(cloudFileModel.icon.iconResource)This implementation:
- Loads the image asynchronously
- Handles errors by falling back to the default icon
- Efficiently manages memory and caching
- Simplifies the code by removing the need for an explicit null check on
binding.ivFileImage.drawableTo ensure Glide is available in the project, run:
If Glide is not found, consider adding it to the project dependencies.
util/src/main/java/org/cryptomator/util/SharedPreferencesHandler.kt (2)
330-330: LGTM! Constant is well-defined and properly placed.The
THUMBNAIL_GENERATIONconstant is correctly added to the companion object and follows the established naming convention. Its purpose as a key for storing the thumbnail generation preference is clear.
Line range hint
1-391: Summary: Changes look good with minor suggestions for improvement.The additions to the
SharedPreferencesHandlerclass for handling thumbnail generation preferences are well-implemented and integrate smoothly with the existing code. The newgenerateThumbnails()method andTHUMBNAIL_GENERATIONconstant follow the established patterns in the class.We've suggested two minor improvements:
- A slight refactor of the
generateThumbnails()method for improved readability and future-proofing.- Adding a
setThumbnailGeneration()method for consistency with other preference-setting methods in the class.Overall, the changes are approved and ready to be merged after considering these suggestions.
presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SettingsFragment.kt (6)
31-31: LGTM: Import statement for THUMBNAIL_GENERATION added correctly.The import for the
THUMBNAIL_GENERATIONconstant fromSharedPreferencesHandleris correctly added, aligning with the new thumbnail generation functionality being implemented.
51-51: LGTM: setupThumbnailGeneration() called in onCreatePreferences.The
setupThumbnailGeneration()method is correctly called within theonCreatePreferencesmethod, consistent with the setup of other preferences.
114-117: Implementation of thumbnailGenerationChangeListener is still incomplete.The
thumbnailGenerationChangeListenerhas been added, but its implementation is still incomplete. Please complete the implementation to handle changes in the thumbnail generation preference.
150-153: Implementation of setupThumbnailGeneration is still incomplete.The
setupThumbnailGenerationmethod has been added, but its implementation is still incomplete. Please complete the implementation to properly configure the thumbnail generation preference.
259-259: LGTM: Thumbnail generation preference listener set in onResume.The thumbnail generation preference change listener is correctly set in the
onResumemethod, consistent with how other preference listeners are handled.
342-342: LGTM: THUMBNAIL_GENERATION constant added correctly.The
THUMBNAIL_GENERATIONconstant has been correctly added to the companion object, following the naming convention used for other constants in this file.presentation/src/main/java/org/cryptomator/presentation/ui/adapter/BrowseFilesAdapter.kt (2)
3-3: LGTM: New import for BitmapFactoryThe addition of
android.graphics.BitmapFactoryimport is appropriate for the new thumbnail decoding functionality.
43-44: LGTM: Constructor updated with MimeTypes parameterThe addition of the
mimeTypesparameter to the constructor is appropriate for the new image media type functionality. Good job on maintaining proper encapsulation by marking it as private.presentation/src/main/res/values/strings.xml (1)
635-637: LGTM: New thumbnail generation options added.The new string resources for thumbnail generation options ("Never", "Per File", "Per Folder") are well-defined and follow Android naming conventions. They provide clear options for users to configure thumbnail generation preferences.
presentation/src/main/java/org/cryptomator/presentation/ui/activity/BrowseFilesActivity.kt (4)
53-54: New imports forFilesFragmentInterfaceandGalleryFragmentThe imports for
FilesFragmentInterfaceandGalleryFragmentare correctly added to support the new gallery view functionality.
108-108: RefactoredcreateFragment()to delegate tocreateFragmentFor()The
createFragment()method now delegates fragment creation tocreateFragmentFor(), enhancing modularity and simplifying fragment management based on folder and navigation settings.
429-439: Conditional fragment instantiation increateFragmentFor()The
createFragmentFor()methods correctly determine whether to instantiate aGalleryFragmentor aBrowseFilesFragmentbased on whether the folder is an auto-upload folder. This provides a dynamic way to display content appropriately.
441-443: Efficient check inisAutoUploadFolder()The
isAutoUploadFolder()method efficiently checks if the providedvaultIdandfolderPathmatch the auto-upload settings insharedPreferencesHandler. This ensures that the correct fragment type is used for auto-upload folders.
| <androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android" | ||
| xmlns:app="http://schemas.android.com/apk/res-auto" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent"> | ||
|
|
||
| <ImageView | ||
| android:id="@+id/gallery_cloud_node_image" | ||
| android:layout_width="0dp" | ||
| android:layout_height="0dp" | ||
| app:layout_constraintBottom_toBottomOf="parent" | ||
| app:layout_constraintDimensionRatio="1" | ||
| app:layout_constraintLeft_toLeftOf="parent" | ||
| app:layout_constraintRight_toRightOf="parent" | ||
| app:layout_constraintTop_toTopOf="parent" /> | ||
| </androidx.constraintlayout.widget.ConstraintLayout> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the layout structure.
The current implementation uses a ConstraintLayout with a single ImageView child. While this achieves the desired 1:1 aspect ratio, it might be overly complex for a single view.
Consider simplifying the layout by replacing the ConstraintLayout with a custom view or using a simpler layout. Here's an example using a FrameLayout:
<FrameLayout
android:layout_width="match_parent"
android:layout_height="wrap_content">
<ImageView
android:id="@+id/gallery_cloud_node_image"
android:layout_width="match_parent"
android:layout_height="0dp"
android:adjustViewBounds="true"
android:scaleType="centerCrop"
app:layout_constraintDimensionRatio="1:1" />
</FrameLayout>This approach achieves the same 1:1 aspect ratio with a simpler layout structure.
| fun generateThumbnails(): ThumbnailsOption { | ||
| return when(defaultSharedPreferences.getValue(THUMBNAIL_GENERATION, "NEVER")){ | ||
| "NEVER" -> ThumbnailsOption.NEVER | ||
| "PER_FILE" -> ThumbnailsOption.PER_FILE | ||
| "PER_FOLDER" -> ThumbnailsOption.PER_FOLDER | ||
| else -> ThumbnailsOption.NEVER | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a method to set the thumbnail generation preference.
The new generateThumbnails() method and THUMBNAIL_GENERATION constant are well-implemented and integrate smoothly with the existing code. However, for consistency with other preference-handling methods in the class (e.g., setScreenStyleMode(), setDebugMode(), etc.), consider adding a method to set the thumbnail generation preference.
Here's a suggested implementation:
fun setThumbnailGeneration(option: ThumbnailsOption) {
defaultSharedPreferences.setValue(THUMBNAIL_GENERATION, option.name)
}This method would allow users of the SharedPreferencesHandler class to easily set the thumbnail generation preference, maintaining consistency with the overall design of the class.
Also applies to: 330-330
| if (node is CloudFileModel && isImageMediaType(node.name) && node.thumbnail != null) { | ||
| val bitmap = BitmapFactory.decodeFile(node.thumbnail!!.absolutePath) | ||
| binding.cloudNodeImage.setImageBitmap(bitmap) | ||
| } else { | ||
| binding.cloudNodeImage.setImageResource(bindCloudNodeImage(node)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential null pointer exception in thumbnail handling
The logic for handling image thumbnails is good, but there's a potential null pointer exception on line 143. Consider using the safe call operator ?. to handle potential null values.
Here's a suggested improvement:
if (node is CloudFileModel && isImageMediaType(node.name) && node.thumbnail != null) {
- val bitmap = BitmapFactory.decodeFile(node.thumbnail!!.absolutePath)
+ val bitmap = BitmapFactory.decodeFile(node.thumbnail?.absolutePath)
binding.cloudNodeImage.setImageBitmap(bitmap)
} else {
binding.cloudNodeImage.setImageResource(bindCloudNodeImage(node))
}This change ensures that absolutePath is only called if node.thumbnail is not null, preventing potential null pointer exceptions.
📝 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.
| if (node is CloudFileModel && isImageMediaType(node.name) && node.thumbnail != null) { | |
| val bitmap = BitmapFactory.decodeFile(node.thumbnail!!.absolutePath) | |
| binding.cloudNodeImage.setImageBitmap(bitmap) | |
| } else { | |
| binding.cloudNodeImage.setImageResource(bindCloudNodeImage(node)) | |
| } | |
| } | |
| if (node is CloudFileModel && isImageMediaType(node.name) && node.thumbnail != null) { | |
| val bitmap = BitmapFactory.decodeFile(node.thumbnail?.absolutePath) | |
| binding.cloudNodeImage.setImageBitmap(bitmap) | |
| } else { | |
| binding.cloudNodeImage.setImageResource(bindCloudNodeImage(node)) | |
| } | |
| } |
| override fun showLoading(loading: Boolean?) { | ||
| loading?.let { binding.swipeRefreshLayout.isRefreshing = it } | ||
| } |
There was a problem hiding this comment.
Handle null loading parameter in showLoading method
The showLoading method updates isRefreshing only when loading is non-null. To ensure consistent behavior, consider defaulting to false when loading is null.
Apply this diff to handle the null case:
override fun showLoading(loading: Boolean?) {
- loading?.let { binding.swipeRefreshLayout.isRefreshing = it }
+ binding.swipeRefreshLayout.isRefreshing = loading ?: false
}📝 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 showLoading(loading: Boolean?) { | |
| loading?.let { binding.swipeRefreshLayout.isRefreshing = it } | |
| } | |
| override fun showLoading(loading: Boolean?) { | |
| binding.swipeRefreshLayout.isRefreshing = loading ?: false | |
| } |
| get() = requireArguments().getSerializable(ARG_FOLDER) as CloudFolderModel | ||
| set(updatedFolder) { | ||
| arguments?.putSerializable(ARG_FOLDER, updatedFolder) | ||
| } |
There was a problem hiding this comment.
Ensure consistent use of requireArguments() in the folder property setter
The folder property's getter uses requireArguments(), but the setter uses arguments?. For consistency and to avoid potential NullPointerExceptions, consider using requireArguments() in the setter as well.
Apply this diff to align with the getter:
set(updatedFolder) {
- arguments?.putSerializable(ARG_FOLDER, updatedFolder)
+ requireArguments().putSerializable(ARG_FOLDER, updatedFolder)
}📝 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.
| get() = requireArguments().getSerializable(ARG_FOLDER) as CloudFolderModel | |
| set(updatedFolder) { | |
| arguments?.putSerializable(ARG_FOLDER, updatedFolder) | |
| } | |
| get() = requireArguments().getSerializable(ARG_FOLDER) as CloudFolderModel | |
| set(updatedFolder) { | |
| requireArguments().putSerializable(ARG_FOLDER, updatedFolder) | |
| } |
| triggerUpdateSelectedNodesNumberInfo() | ||
| } | ||
|
|
||
| abstract inner class UiStateTest(val isForFile: Boolean) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename 'UiStateTest' to 'UiState' for clarity
Consider renaming the abstract inner class UiStateTest to UiState to better reflect its purpose. The suffix Test might be misleading if the class is not intended solely for testing.
Apply this diff to rename the class:
- abstract inner class UiStateTest(val isForFile: Boolean) {
+ abstract inner class UiState(val isForFile: Boolean) {📝 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.
| abstract inner class UiStateTest(val isForFile: Boolean) { | |
| abstract inner class UiState(val isForFile: Boolean) { |
| fun filterNodes(nodes: List<CloudNodeModel<*>>?, filterText: String): List<CloudNodeModel<*>>? { | ||
| return if (filterText.isNotEmpty()) { | ||
| if (sharedPreferencesHandler.useGlobSearch()) { | ||
| nodes?.filter { cloudNode -> PatternMatcher(filterText, PatternMatcher.PATTERN_SIMPLE_GLOB).match(cloudNode.name) } | ||
| } else { | ||
| nodes?.filter { cloudNode -> cloudNode.name.contains(filterText, true) } | ||
| } | ||
| } else { | ||
| nodes | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the 'filterNodes' function
The filterNodes function can be simplified by using Kotlin's let and when expressions, enhancing readability.
Apply this diff to refactor the function:
fun filterNodes(nodes: List<CloudNodeModel<*>>?, filterText: String): List<CloudNodeModel<*>>? {
- return if (filterText.isNotEmpty()) {
- if (sharedPreferencesHandler.useGlobSearch()) {
- nodes?.filter { cloudNode -> PatternMatcher(filterText, PatternMatcher.PATTERN_SIMPLE_GLOB).match(cloudNode.name) }
- } else {
- nodes?.filter { cloudNode -> cloudNode.name.contains(filterText, true) }
- }
- } else {
- nodes
- }
+ return nodes?.let {
+ when {
+ filterText.isEmpty() -> it
+ sharedPreferencesHandler.useGlobSearch() ->
+ it.filter { cloudNode -> PatternMatcher(filterText, PatternMatcher.PATTERN_SIMPLE_GLOB).match(cloudNode.name) }
+ else -> it.filter { cloudNode -> cloudNode.name.contains(filterText, true) }
+ }
+ }
}📝 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.
| fun filterNodes(nodes: List<CloudNodeModel<*>>?, filterText: String): List<CloudNodeModel<*>>? { | |
| return if (filterText.isNotEmpty()) { | |
| if (sharedPreferencesHandler.useGlobSearch()) { | |
| nodes?.filter { cloudNode -> PatternMatcher(filterText, PatternMatcher.PATTERN_SIMPLE_GLOB).match(cloudNode.name) } | |
| } else { | |
| nodes?.filter { cloudNode -> cloudNode.name.contains(filterText, true) } | |
| } | |
| } else { | |
| nodes | |
| } | |
| } | |
| fun filterNodes(nodes: List<CloudNodeModel<*>>?, filterText: String): List<CloudNodeModel<*>>? { | |
| return nodes?.let { | |
| when { | |
| filterText.isEmpty() -> it | |
| sharedPreferencesHandler.useGlobSearch() -> | |
| it.filter { cloudNode -> PatternMatcher(filterText, PatternMatcher.PATTERN_SIMPLE_GLOB).match(cloudNode.name) } | |
| else -> it.filter { cloudNode -> cloudNode.name.contains(filterText, true) } | |
| } | |
| } | |
| } |
| if (node is CloudFileModel && isImageMediaType(node.name) && node.thumbnail != null) { | ||
| val bitmap = BitmapFactory.decodeFile(node.thumbnail!!.absolutePath) | ||
| binding.galleryCloudNodeImage.setImageBitmap(bitmap) | ||
| } else { | ||
| binding.galleryCloudNodeImage.setImageResource(bindCloudNodeImage(node)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential decoding errors when loading images
In the bindNodeImage function, when decoding the image file into a bitmap, consider handling potential exceptions that may occur if the file is corrupt or cannot be decoded. This will prevent the app from crashing due to unhandled exceptions.
Apply this diff to add exception handling:
if (node is CloudFileModel && isImageMediaType(node.name) && node.thumbnail != null) {
- val bitmap = BitmapFactory.decodeFile(node.thumbnail!!.absolutePath)
- binding.galleryCloudNodeImage.setImageBitmap(bitmap)
+ val bitmap = BitmapFactory.decodeFile(node.thumbnail!!.absolutePath)
+ if (bitmap != null) {
+ binding.galleryCloudNodeImage.setImageBitmap(bitmap)
+ } else {
+ binding.galleryCloudNodeImage.setImageResource(bindCloudNodeImage(node))
+ }
} else {
binding.galleryCloudNodeImage.setImageResource(bindCloudNodeImage(node))
}📝 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.
| if (node is CloudFileModel && isImageMediaType(node.name) && node.thumbnail != null) { | |
| val bitmap = BitmapFactory.decodeFile(node.thumbnail!!.absolutePath) | |
| binding.galleryCloudNodeImage.setImageBitmap(bitmap) | |
| } else { | |
| binding.galleryCloudNodeImage.setImageResource(bindCloudNodeImage(node)) | |
| } | |
| } | |
| if (node is CloudFileModel && isImageMediaType(node.name) && node.thumbnail != null) { | |
| val bitmap = BitmapFactory.decodeFile(node.thumbnail!!.absolutePath) | |
| if (bitmap != null) { | |
| binding.galleryCloudNodeImage.setImageBitmap(bitmap) | |
| } else { | |
| binding.galleryCloudNodeImage.setImageResource(bindCloudNodeImage(node)) | |
| } | |
| } else { | |
| binding.galleryCloudNodeImage.setImageResource(bindCloudNodeImage(node)) | |
| } | |
| } |
|
|
||
| private var uiState: UiStateTest? = null | ||
|
|
||
| private var currentProgressIcon: Int = 0 |
There was a problem hiding this comment.
Initialize 'currentProgressIcon' with a valid resource ID
The currentProgressIcon variable is initialized to 0, which may not correspond to a valid drawable resource. Consider initializing it with a default drawable resource ID to avoid potential issues when setting the image drawable.
Apply this diff to initialize with a valid resource ID:
- private var currentProgressIcon: Int = 0
+ private var currentProgressIcon: Int = R.drawable.default_progress_iconEnsure that R.drawable.default_progress_icon is a valid drawable resource in your project.
Committable suggestion was skipped due to low confidence.
| @@ -553,7 +562,7 @@ class BrowseFilesActivity : BaseActivity<ActivityLayoutBinding>(ActivityLayoutBi | |||
| browseFilesFragment().showLoading(loading) | |||
| } | |||
|
|
|||
| private fun browseFilesFragment(): BrowseFilesFragment = getCurrentFragment(R.id.fragment_container) as BrowseFilesFragment | |||
| private fun browseFilesFragment(): FilesFragmentInterface = getCurrentFragment(R.id.fragment_container) as FilesFragmentInterface | |||
There was a problem hiding this comment.
Potential risk of ClassCastException in browseFilesFragment()
Casting the fragment to FilesFragmentInterface without type checking could lead to a ClassCastException if a fragment not implementing this interface is present in R.id.fragment_container.
Consider adding a type check before casting to ensure safety:
private fun browseFilesFragment(): FilesFragmentInterface {
val fragment = getCurrentFragment(R.id.fragment_container)
if (fragment is FilesFragmentInterface) {
return fragment
} else {
throw IllegalStateException("Current fragment does not implement FilesFragmentInterface")
}
}This approach ensures that if the fragment does not implement FilesFragmentInterface, an appropriate exception is thrown, preventing unexpected crashes.
Hello guys 👋
This is our proposal for implementing a gallery view [/issues/372]
Me, @WheelyMcBones and @taglioIsCoding have made the following changes.
Referring to our previous PR [/pull/533], in order to create a new Fragment dedicated to the grid visualization, we added the FilesFragmentsInterface, which both the already existing BrowseFilesFragment and the new GalleryFragment now implements. For the visualization we used a GridLayout with custom Item and Adapter. The two Fragments should be interchangeable, but the GalleryFragment is automatically being chosen only for the vault and folder marked as AutoUpload in the settings. Since the automatic upload service is selecting only media files, we can do the assumption that won't be populated with other kind of files so we are not displaying any other information like filename, modification date, size, etc.