Skip to content
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

Merged
merged 2 commits into from
Nov 11, 2024
Merged

Prohibit nested roots #83

merged 2 commits into from
Nov 11, 2024

Conversation

tuancoltech
Copy link
Collaborator

@tuancoltech tuancoltech commented Mar 18, 2024

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/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

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

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

Copy link
Member

@mdrlzy mdrlzy Oct 29, 2024

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
Member

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
Collaborator 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
Copy link
Member

shubertm commented Nov 2, 2024

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
Copy link
Member

mdrlzy commented Nov 2, 2024

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
Copy link
Member

kirillt commented Nov 4, 2024

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
Copy link
Member

shubertm commented Nov 4, 2024

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

@tuancoltech
Copy link
Collaborator Author

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

@shubertm Correct.

Copy link
Member

@shubertm shubertm left a 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

@tuancoltech tuancoltech force-pushed the bug/prohibit_nested_roots branch from 8c65ce7 to 0ae7b99 Compare November 10, 2024 16:20
@tuancoltech tuancoltech merged commit cf05c1f into main Nov 11, 2024
2 checks passed
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