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

Mark parent directory as viewed when all files are viewed #33958

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

Conversation

kerwin612
Copy link
Member

@kerwin612 kerwin612 commented Mar 21, 2025

Fix #25644

before:
before1
before1

after:
after1
after1

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 21, 2025
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 21, 2025
@lunny lunny added this to the 1.24.0 milestone Mar 21, 2025
@lunny lunny added the type/enhancement An improvement of existing functionality label Mar 21, 2025
@lunny lunny changed the title Mark parent directory as viewed when all files are viewed (fixes #25644) Mark parent directory as viewed when all files are viewed Mar 21, 2025
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 25, 2025
@@ -83,3 +83,7 @@ export function mergeChildIfOnlyOneDir(nodes: Item[]): void {
}
}
}

export function fileIsViewed(item: Item): boolean {
return item.isFile ? item.file.IsViewed : (item as DirItem).children.every(fileIsViewed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this algorithm performant?

Suppose the tree it list this: a/b/c/d and there are 100 files in the deepest dir d.

Then you need to call fileIsViewed about 4 * 100 = 400 times? The performance will become worse when there are more parent directories and more files.

Copy link
Member Author

Choose a reason for hiding this comment

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

This approach should be acceptable for now, though I can't think of a more optimal algorithm at the moment. Not sure what else to try, haha.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because there are already some UI performance problems.

When using complex algorithm in Reactivity APIs, the problem becomes more complex and hard to handle.

I just want to make sure there the performance is overall good.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, interesting choice with the top-down check.
I would have used a bottom-up algorithm:
When a file changes its state, the parent sets its state to are all of my direct children viewed?.
This can propagate recursively up to the root node.
That way there shouldn't be a performance concern unless a directory has an enormous number of children.
In that case, some slowdown is to be expected however.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would have used a bottom-up algorithm:
When a file changes its state, the parent sets its state to are all of my direct children viewed?.

Yup, that's also my intuition

Copy link
Member Author

Choose a reason for hiding this comment

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

I've adopted a different implementation approach where child components propagate their state upwards to the parent. This helps eliminate redundant computation. Could you experts please review this approach? While my local tests confirm it achieves the same results, I'd like to get your professional opinions. @wxiaoguang @delvh

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 27, 2025
@kerwin612 kerwin612 requested review from delvh and wxiaoguang March 27, 2025 01:53
* - Directories: Compare children count with viewed count
*/
const isViewed = computed(() => {
return props.item.isFile ? props.item.file.IsViewed : (props.item as DirItem).children.length === count.value;
Copy link
Member

Choose a reason for hiding this comment

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

Batching the update sounds like over-engineering to me.
After all, if I read the code correctly, there's a chance of flickering for a frame.
Why not simply

Suggested change
return props.item.isFile ? props.item.file.IsViewed : (props.item as DirItem).children.length === count.value;
return props.item.isFile ? props.item.file.IsViewed : (props.item as DirItem).children.every(child => child.file.IsViewed);

?

Copy link
Member Author

Choose a reason for hiding this comment

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

The code(props.item.isFile ? props.item.file.IsViewed : (props.item as DirItem).children.every(child => child.file.IsViewed);) fails to accurately retrieve folder statuses in deeply nested folder hierarchies, as the isViewed state isn't backend-maintained but dynamically computed on the fly.

This explains why I initially implemented it this way:

export function fileIsViewed(item: Item): boolean {
  return item.isFile ? item.file.IsViewed : (item as DirItem).children.every(fileIsViewed);
}

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right.
However, that difference should be mainly for historical reasons:
Prior to this PR, the Viewed state didn't make sense on directories, so DirItem didn't have the attribute.
Now, it does make sense.
So we should be able to move the attribute one layer up into FileItem and DirItem and convert it from computed on demand into an actual attribute we update.
Then, every tree item has this attribute and we can use this approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed, moving the folder status calculation logic to the backend would require:

​1. Backend complexity increase: Implementing similar computational logic as in this PR, adding architectural overhead
2. ​Frontend adaptation: Introducing new logic to synchronize folder read status updates

In contrast, the current PR's approach:

​1. Zero backend changes: Maintains existing backend architecture
​2. Frontend-driven computation: Handles folder status calculation entirely on the client-side

The biggest difference between the two is that the front - end needs to add a new logic of "reading the folder status from the back - end". The reason why the file status doesn't need to be read separately is that the interaction is initiated by the front - end, so the front - end can directly read its own status. However, if the folder status is calculated by the back - end, a new logic has to be added for reading.

I’m not sure if I’ve made myself clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/frontend size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark the containing directory as viewed in the file tree when all its files are viewed
5 participants