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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
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.