Skip to content

Prohibit nested roots#83

Merged
tuancoltech merged 2 commits into
mainfrom
bug/prohibit_nested_roots
Nov 11, 2024
Merged

Prohibit nested roots#83
tuancoltech merged 2 commits into
mainfrom
bug/prohibit_nested_roots

Conversation

@tuancoltech

@tuancoltech tuancoltech commented Mar 18, 2024

Copy link
Copy Markdown
Contributor

Description:

Prohibit nested roots: #68

For the duplicated logic removal of RootPickerDialogFragment, or to be exact, FoldersViewModel, it should be done in Navigator repository.
An in-progress work is here: ARK-Builders/Navigator#441

Before

Screen.Recording.2024-10-29.at.22.25.00.mov

After

Screen.Recording.2024-10-29.at.22.23.17.mov

@tuancoltech tuancoltech added bug Something isn't working enhancement New feature or request do not merge labels Mar 18, 2024
@tuancoltech tuancoltech self-assigned this Mar 18, 2024
@tuancoltech tuancoltech force-pushed the bug/prohibit_nested_roots branch from 492b448 to 44f246b Compare March 18, 2024 05:45
@tuancoltech tuancoltech force-pushed the enhance/restructure_repository branch from 72662ce to f967e06 Compare March 23, 2024 15:07
@tuancoltech tuancoltech force-pushed the bug/prohibit_nested_roots branch 3 times, most recently from 2416977 to b63d055 Compare March 24, 2024 08:51
@tuancoltech tuancoltech changed the title WIP: Prohibit nested roots Prohibit nested roots Mar 24, 2024
@tuancoltech tuancoltech requested review from kirillt and mdrlzy March 24, 2024 08:51
@tuancoltech tuancoltech changed the base branch from enhance/restructure_repository to main March 27, 2024 05:43
@tuancoltech tuancoltech force-pushed the bug/prohibit_nested_roots branch from b63d055 to 235daea Compare March 27, 2024 05:45
Comment on lines +195 to +198
val hasNestedRoot = roots.contains(file)
|| (roots.indexOfFirst { path ->
val index = path.toString().indexOf(file.toString())
(index >= 0) && (path.toString()[index] == '/') } >= 0)

Copy link
Copy Markdown
Contributor

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

@mdrlzy mdrlzy Oct 29, 2024

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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.

@shubertm

shubertm commented Nov 2, 2024

Copy link
Copy Markdown

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)
@kirillt @mdrlzy @tuancoltech @hieuwu

@mdrlzy

mdrlzy commented Nov 2, 2024

Copy link
Copy Markdown
Contributor

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) @kirillt @mdrlzy @tuancoltech @hieuwu

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.

@kirillt

kirillt commented Nov 4, 2024

Copy link
Copy Markdown
Member

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.

@shubertm

shubertm commented Nov 4, 2024

Copy link
Copy Markdown

Right now we are denying tracking files in /A when /A/B is root, right?

@tuancoltech

Copy link
Copy Markdown
Contributor Author

Right now we are denying tracking files in /A when /A/B is root, right?

@shubertm Correct.

Comment thread utils/src/main/java/dev/arkbuilders/components/utils/PathExt.kt Outdated
Comment thread utils/src/main/java/dev/arkbuilders/components/utils/PathExt.kt Outdated
Comment thread utils/src/main/java/dev/arkbuilders/components/utils/PathExt.kt Outdated

@shubertm shubertm left a comment

Copy link
Copy Markdown

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants