Skip to content

Move RootPicker to filepicker module #114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mdrlzy wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Move RootPicker to filepicker module #114

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
99792cd
Move RootPicker to filepicker module
mdrlzy Dec 1, 2024
48caf42
Pin: check only for nested roots (under parent root we can make fav)
mdrlzy Dec 1, 2024
15b624d
Pin: better check for favorites (map relatives to full path)
mdrlzy Dec 1, 2024
beda289
Revert favorites check with full path, check with relatives endsWith
mdrlzy Dec 27, 2024
c17719b
ArkFilePickerViewModel: pinFile -> pinFolder
mdrlzy Dec 27, 2024
5f98d2d
Remove INTERNAL_STORAGE const from utils
mdrlzy Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion filepicker/src/main/java/dev/arkbuilders/components/filepicker/ArkFilePickerConfig.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import androidx.annotation.StyleRes
import dev.arkbuilders.components.filepicker.R
import java.nio.file.Path

class ArkFilePickerConfig(
data class ArkFilePickerConfig(
@StringRes
val titleStringId: Int = R.string.ark_file_picker_pick_title,
@StringRes
Expand Down
12 changes: 8 additions & 4 deletions filepicker/src/main/java/dev/arkbuilders/components/filepicker/ArkFilePickerViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import dev.arkbuilders.arklib.data.folders.FoldersRepo
import dev.arkbuilders.arklib.utils.DeviceStorageUtils
import dev.arkbuilders.arklib.utils.listChildren
import dev.arkbuilders.components.utils.hasNestedOrParentalRoot
import dev.arkbuilders.components.utils.hasNestedRoot
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext
import java.nio.file.Path
Expand Down Expand Up @@ -191,9 +192,8 @@ internal class ArkFilePickerViewModel(
val rootsWithFavorites = container.stateFlow.value.rootsWithFavs
Copy link
Member

@kirillt kirillt Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename the function pinFile into pinFolder for more clarity?

val roots = rootsWithFavorites.keys
val root = roots.find { root -> file.startsWith(root) }
val favorites = rootsWithFavorites[root]?.flatten()

val hasNestedRoot = file.hasNestedOrParentalRoot(roots)
val hasNestedRoot = file.hasNestedRoot(roots)
Copy link
Collaborator

@tuancoltech tuancoltech Dec 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 from hasNestedOrParentalRoot to hasNestedRoot, we're ignoring every root folder that might contain currently folder, and still allow pining the current folder.

Is that our expectation?

Copy link
Member Author

@mdrlzy mdrlzy Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (hasNestedRoot) {
postSideEffect(FilePickerSideEffect.NestedRootProhibited)
Expand All @@ -203,10 +203,14 @@ internal class ArkFilePickerViewModel(
val haveRoot = haveRoot()

root?.let {

//Make sure file isn't inside a root folder
if (root != file) {
val foundAsFavorite = favorites?.any { file.endsWith(it) } ?: false
val favorites = rootsWithFavorites.map { (root, relativeFavorites) ->
relativeFavorites.map {
root.resolve(it)
Copy link
Member

@kirillt kirillt Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why rootsWithFavorites.map contains absolute paths? We should save only relative paths on disk.

Copy link
Member Author

@mdrlzy mdrlzy Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Favorites in rootsWithFavorites are relative and I map them into absolute paths for check

Copy link
Collaborator

@tuancoltech tuancoltech Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member Author

@mdrlzy mdrlzy Dec 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}.flatten()
val foundAsFavorite = favorites.any { it == file }

if (!foundAsFavorite) {
addFavorite(file)
Expand Down
97 changes: 97 additions & 0 deletions filepicker/src/main/java/dev/arkbuilders/components/filepicker/ArkRootPickerFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package dev.arkbuilders.components.filepicker

import androidx.core.os.bundleOf
import androidx.fragment.app.setFragmentResult
import androidx.lifecycle.lifecycleScope
import dev.arkbuilders.arklib.data.folders.FoldersRepo
import dev.arkbuilders.components.utils.hasNestedRoot
import dev.arkbuilders.components.utils.isInternalStorage
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import java.nio.file.Path

class ArkRootPickerFragment : ArkFilePickerFragment() {
private var rootNotFavorite = false

override fun onFolderChanged(currentFolder: Path) {
lifecycleScope.launch {
val folders = FoldersRepo.instance.provideFolders()
val roots = folders.keys

if (currentFolder.isInternalStorage() || currentFolder.hasNestedRoot(roots)) {
Copy link
Collaborator

@tuancoltech tuancoltech Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we still allowing picking a root folder even if the folder is inside another root folder already?

Copy link
Member Author

@mdrlzy mdrlzy Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If folder has parental root, we should still be able to make it favorite

rootNotFavorite = true
binding.btnPick.text = getString(R.string.ark_file_picker_root)
binding.btnPick.isEnabled = false
return@launch
}

val root = roots.find { root -> currentFolder.startsWith(root) }
root?.let {
if (root == currentFolder) {
rootNotFavorite = true
binding.btnPick.text = getString(R.string.ark_file_picker_root)
binding.btnPick.isEnabled = false
} else {
val favorites = folders.map { (root, relativeFavorites) ->
relativeFavorites.map {
root.resolve(it)
}
}.flatten()
val foundAsFavorite = favorites.any { it == currentFolder }
rootNotFavorite = false
binding.btnPick.text = getString(R.string.ark_file_picker_favorite)
binding.btnPick.isEnabled = !foundAsFavorite
}
} ?: let {
rootNotFavorite = true
binding.btnPick.text = getString(R.string.ark_file_picker_root)
binding.btnPick.isEnabled = true
}
}
}

override fun onPick(pickedPath: Path) {
lifecycleScope.launch(Dispatchers.IO) {
addRootOrFavorite(pickedPath)
setFragmentResult(
ROOT_PICKED_REQUEST_KEY,
bundleOf().apply {
putString(PICKED_PATH_BUNDLE_KEY, pickedPath.toString())
putBoolean(ROOT_NOT_FAV_BUNDLE_KEY, rootNotFavorite)
}
)
}
}

private suspend fun addRootOrFavorite(pickedPath: Path) {
val folders = FoldersRepo.instance.provideFolders()
if (rootNotFavorite) {
FoldersRepo.instance.addRoot(pickedPath)
} else {
val root = folders.keys.find { pickedPath.startsWith(it) }
?: throw IllegalStateException(
"Can't add favorite if it's root is not added"
)
val favoriteRelativePath = root.relativize(pickedPath)
FoldersRepo.instance.addFavorite(root, favoriteRelativePath)
}
}

companion object {
const val ROOT_PICKED_REQUEST_KEY = "rootPicked"
const val PICKED_PATH_BUNDLE_KEY = "pickedPath"
const val ROOT_NOT_FAV_BUNDLE_KEY = "rootNotFav"

fun newInstance(
config: ArkFilePickerConfig = ArkFilePickerConfig()
) = ArkRootPickerFragment().apply {
setConfig(
config.copy(
showRoots = false,
mode = ArkFilePickerMode.FOLDER,
pathPickedRequestKey = "notUsed"
)
)
}
}
}
17 changes: 17 additions & 0 deletions filepicker/src/main/java/dev/arkbuilders/components/filepicker/FragmentManagerUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,21 @@ fun FragmentManager.onArkPathPicked(
)
)
}
}

fun FragmentManager.onArkRootOrFavPicked(
lifecycleOwner: LifecycleOwner,
listener: (path: Path, rootNotFavorite: Boolean) -> Unit
) {
setFragmentResultListener(
ArkRootPickerFragment.ROOT_PICKED_REQUEST_KEY,
lifecycleOwner
) { _, bundle ->
listener(
Path(
bundle.getString(ArkRootPickerFragment.PICKED_PATH_BUNDLE_KEY)!!
Copy link
Collaborator

@tuancoltech tuancoltech Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
bundle.getString(ArkRootPickerFragment.PICKED_PATH_BUNDLE_KEY)!!
bundle.getString(ArkFilePickerFragment.PATH_PICKED_PATH_BUNDLE_KEY, "")

Copy link
Member Author

@mdrlzy mdrlzy Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

),
bundle.getBoolean(ArkRootPickerFragment.ROOT_NOT_FAV_BUNDLE_KEY)
)
}
}
2 changes: 2 additions & 0 deletions filepicker/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
<string name="ark_file_picker_pin_folder_only">Only folder can be pinned.</string>
<string name="ark_file_nested_root_inside">There\'s already nested root(s) inside.</string>
<string name="ark_file_picker_pin">Pin</string>
<string name="ark_file_picker_root">Root</string>
<string name="ark_file_picker_favorite">Favorite</string>
<plurals name="ark_file_picker_items">
<item quantity="one">%d item</item>
<item quantity="other">%d items</item>
Expand Down
3 changes: 2 additions & 1 deletion sample/src/main/java/dev/arkbuilders/sample/MainActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import com.google.android.material.button.MaterialButton
import dev.arkbuilders.components.filepicker.ArkFilePickerConfig
import dev.arkbuilders.components.filepicker.ArkFilePickerFragment
import dev.arkbuilders.components.filepicker.ArkFilePickerMode
import dev.arkbuilders.components.filepicker.ArkRootPickerFragment
import dev.arkbuilders.components.filepicker.onArkPathPicked
import dev.arkbuilders.sample.about.AboutActivity
import dev.arkbuilders.sample.storage.StorageDemoFragment
Expand All @@ -39,7 +40,7 @@ class MainActivity : AppCompatActivity() {

findViewById<MaterialButton>(R.id.btn_root_picker).setOnClickListener {
resolvePermissions()
RootFavPickerDialog
ArkRootPickerFragment
.newInstance()
.show(supportFragmentManager, null)
}
Expand Down
58 changes: 0 additions & 58 deletions sample/src/main/java/dev/arkbuilders/sample/RootFavPickerDialog.kt
View file
Open in desktop

This file was deleted.

9 changes: 8 additions & 1 deletion utils/src/main/java/dev/arkbuilders/components/utils/PathExt.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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")
Copy link
Collaborator

@tuancoltech tuancoltech Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@mdrlzy mdrlzy Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


fun Path.hasNestedOrParentalRoot(roots: Iterable<Path>): Boolean {
val hasNestedRoot = roots.any { path ->
this.startsWith(path) || path.startsWith(this)
}
return hasNestedRoot
}
}

fun Path.hasNestedRoot(roots: Iterable<Path>) = roots.any { it.startsWith(this) }

fun Path.isInternalStorage() = this == INTERNAL_STORAGE