Move RootPicker to filepicker module #114
Conversation
| @@ -191,9 +192,8 @@ internal class ArkFilePickerViewModel( | |||
| val rootsWithFavorites = container.stateFlow.value.rootsWithFavs | |||
There was a problem hiding this comment.
Maybe rename the function pinFile into pinFolder for more clarity?
| val favorites = rootsWithFavorites.map { (root, relativeFavorites) -> | ||
| relativeFavorites.map { | ||
| root.resolve(it) |
There was a problem hiding this comment.
But why rootsWithFavorites.map contains absolute paths? We should save only relative paths on disk.
There was a problem hiding this comment.
Favorites in rootsWithFavorites are relative and I map them into absolute paths for check
There was a problem hiding this comment.
@mdrlzy I'm curious why do we have to resolve relative path into absolute path just for checking if the current being pinned folder is already a favorite, while we still can do it with the original relative paths?
There was a problem hiding this comment.
I thought that we check all favorites and not just under specific root. I didn't like the check via endsWith. But in fact everything was ok, I returned it as it was.
|
|
||
| val hasNestedRoot = file.hasNestedOrParentalRoot(roots) | ||
| val hasNestedRoot = file.hasNestedRoot(roots) |
There was a problem hiding this comment.
@mdrlzy By checking if a folder hasNestedOrParentalRoot, we are prohibiting pinning current folder if
- it contains root(s)
or - it is being contained by root(s)
By changing fromhasNestedOrParentalRoottohasNestedRoot, we're ignoring every root folder that mightcontaincurrently folder, and still allow pining the current folder.
Is that our expectation?
There was a problem hiding this comment.
If folder has parental root, we should still be able to pin it as favorite. Now this is not possible, check the main branch yourself.
Sorry, my bad, I forgot about it when I was reviewing your PR
| @@ -1,10 +1,17 @@ | |||
| package dev.arkbuilders.components.utils | |||
|
|
|||
| import java.nio.file.Path | |||
| import kotlin.io.path.Path | |||
|
|
|||
| val INTERNAL_STORAGE = Path("/storage/emulated/0") | |||
There was a problem hiding this comment.
Firstly, this path actually denotes external storage, not internal storage, so you might want to update the value and the function's name. Also, the intention is to check if a path is the root external storage (not every external storage path is a root one).
So, the name isInternalStorage is quite confusing.
What about this name? isRootExternalStorage?
Secondly, comparison using this hard-coded path is unreliable because the path is not always exactly the same on every device.
Instead, what do you think about the other approach as below?
fun File.isRootExternalStorageFolder(): Boolean {
// Root of external storage
val rootExternalStorage = Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS).parentFile
return this.canonicalPath == rootExternalStorage?.canonicalPath
}
I guess this will work on every device regardless of the actually path value of the external storage.
There was a problem hiding this comment.
Internal storage is meant in relation to sd card, for example there is device and sd card, the device will be internal.
Naming taken from there
With arklib we can get list of storage paths (device, sd card)
https://github.com/ARK-Builders/arklib-android/blob/main/lib/src/main/java/dev/arkbuilders/arklib/utils/FileUtils.kt
/storage/emulated/0 is needed only to show meaningful Internal storage instead of full path. User will not be able to go to /storage/emulated/, /storage/, /
Hardcode is bad, but it was simple option that covers 95% of cases
User doesn't know about internal folder of Android apps :)
How can I understand with your code whether this path belongs to device or sd card?
Actually INTERNAL_STORAGE constant is not needed here, it is already in arklib.
And I'll delete isInternalStorage, it is really confusing.
I did some research, there is an api that seems to allow to find out storage name:
https://developer.android.com/reference/android/os/storage/StorageVolume
https://github.com/zhanghai/MaterialFiles/blob/master/app/src/main/java/me/zhanghai/android/files/storage/DeviceStorage.kt
We can create a task to refactor this constant and get names through normal api.
@kirillt what do you think?
| ) { _, bundle -> | ||
| listener( | ||
| Path( | ||
| bundle.getString(ArkRootPickerFragment.PICKED_PATH_BUNDLE_KEY)!! |
There was a problem hiding this comment.
Instead of asserting the returned string value, what about giving it a non-null fallback value?
It will never crash the app even if the value cannot be found from this bundle.
| bundle.getString(ArkRootPickerFragment.PICKED_PATH_BUNDLE_KEY)!! | |
| bundle.getString(ArkFilePickerFragment.PATH_PICKED_PATH_BUNDLE_KEY, "") |
There was a problem hiding this comment.
This is key for selected path from dialog. If we return fragment result bundle without selected path, then something goes terribly wrong. We want to crash in this case.
| val folders = FoldersRepo.instance.provideFolders() | ||
| val roots = folders.keys | ||
|
|
||
| if (currentFolder.isInternalStorage() || currentFolder.hasNestedRoot(roots)) { |
There was a problem hiding this comment.
Are we still allowing picking a root folder even if the folder is inside another root folder already?
There was a problem hiding this comment.
If folder has parental root, we should still be able to make it favorite
#68
Also refactor filepicker pin feature. Check only for nested roots, we can make favorites under parent root.