-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Prohibit nested roots #83
Conversation
492b448
to
44f246b
Compare
72662ce
to
f967e06
Compare
2416977
to
b63d055
Compare
b63d055
to
235daea
Compare
235daea
to
b7fb1e6
Compare
val hasNestedRoot = roots.contains(file) | ||
|| (roots.indexOfFirst { path -> | ||
val index = path.toString().indexOf(file.toString()) | ||
(index >= 0) && (path.toString()[index] == '/') } >= 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make second condition as separate variable?
Second condition is hard to understand, maybe root.startsWith(file) || file.startsWith(root)
startsWith
checks if this is child of argument path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also extract this check as separate method to the utils
module?
We will need this check in Navigator app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems you don't check if selected folder is parent of root.
We can't allow /Internal/Music
root and /Internal/Music/A
selected folder to pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdrlzy The changes in this PR are doing exactly what you think is missing.
That means, if /Internal/Music/A
is root, pinning (in consequence, making root) /Internal/Music
is disallowed.
Why do we have to avoid making the parent folder root when a child is already root? I thought by nested it was meant that once a folder is made root every folder inside it cannot be made root again. By this definition a parent to that folder can be made root by dropping all the children that are roots within it (root means ark folder) |
Good point. I think since we are moving core to JNI, transfer of root should be part of Rust ark-core api. It's easier to disallow it now. |
Later would be great to implement "moving the root up", i.e. marking a parent as root, which would automatically mark all chilren-roots as favorites. But this requires some manipulation with indexes. Yes, it will be implemented in Rust. Right now, it's better to disable nested roots. Technically, we can allow them, it would just mean that same files are tracked by different indexes. But indexing these separate indexes (1 in parent and 1 in child) would be independent so it can lead to waste of time for users, when they open folder /A/B, then /A or /A/B/C. Favorite folders delegate indexing to its parent root, so better to explicit about it right now. |
Right now we are denying tracking files in /A when /A/B is root, right? |
@shubertm Correct. |
utils/src/main/java/dev/arkbuilders/components/utils/PathExt.kt
Outdated
Show resolved
Hide resolved
utils/src/main/java/dev/arkbuilders/components/utils/PathExt.kt
Outdated
Show resolved
Hide resolved
utils/src/main/java/dev/arkbuilders/components/utils/PathExt.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is good. Just resolve current comments please
880b38f
to
8c65ce7
Compare
8c65ce7
to
0ae7b99
Compare
Description:
Prohibit nested roots: #68
For the duplicated logic removal of RootPickerDialogFragment, or to be exact,
FoldersViewModel
, it should be done inNavigator
repository.An in-progress work is here: ARK-Builders/ARK-Navigator#441
Before
Screen.Recording.2024-10-29.at.22.25.00.mov
After
Screen.Recording.2024-10-29.at.22.23.17.mov