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.

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

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
facb60b
Mark parent directory as viewed when all files are viewed (fixes #25644)
kerwin612 Mar 21, 2025
1df14bf
Update DiffFileTreeItem.vue
kerwin612 Mar 26, 2025
927a6a1
Merge branch 'main' into feat/mark-viewed-directory
kerwin612 Mar 27, 2025
c3baa0b
fix
kerwin612 Mar 27, 2025
9344407
fix
kerwin612 Mar 27, 2025
07715a2
Merge branch 'main' into feat/mark-viewed-directory
kerwin612 Apr 3, 2025
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
83 changes: 75 additions & 8 deletions web_src/js/components/DiffFileTreeItem.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,81 @@
<script lang="ts" setup>
import {SvgIcon, type SvgName} from '../svg.ts';
import {diffTreeStore} from '../modules/stores.ts';
import {ref} from 'vue';
import type {Item, File, FileStatus} from '../utils/filetree.ts';
import {computed, nextTick, ref, watch} from 'vue';
import type {Item, DirItem, File, FileStatus} from '../utils/filetree.ts';

defineProps<{
// ------------------------------------------------------------
// Component Props
// ------------------------------------------------------------
const props = defineProps<{
item: Item,
setViewed?:(val: boolean) => void,
}>();

// ------------------------------------------------------------
// Reactive State & Refs
// ------------------------------------------------------------
const store = diffTreeStore();
const collapsed = ref(false);
const count = ref(0);
let pendingUpdate = 0;
let pendingTimer: Promise<void> | undefined;
// ------------------------------------------------------------
// Batch Update Mechanism
// ------------------------------------------------------------
/**
* Handles batched updates to count value
* Prevents multiple updates within the same tick
*/
const setCount = (isViewed: boolean) => {
pendingUpdate += (isViewed ? 1 : -1);

if (pendingTimer === undefined) {
pendingTimer = nextTick(() => {
count.value = Math.max(0, count.value + pendingUpdate);
pendingUpdate = 0;
pendingTimer = undefined;
});
}
};

// ------------------------------------------------------------
// Computed Properties
// ------------------------------------------------------------
/**
* Determines viewed state based on item type:
* - Files: Directly use IsViewed property
* - 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.

});
// ------------------------------------------------------------
// Watchers & Side Effects
// ------------------------------------------------------------
/**
* Propagate viewed state changes to parent component
* Using post flush to ensure DOM updates are complete
*/
watch(
() => isViewed.value,
(newVal) => {
if (props.setViewed) {
props.setViewed(newVal);
}
},
{immediate: true, flush: 'post'},
);

// ------------------------------------------------------------
// Collapse Behavior Documentation
// ------------------------------------------------------------
/**
* Collapse behavior rules:
* - Viewed folders start collapsed initially
* - Manual expand/collapse takes precedence over automatic behavior
* - State persists after manual interaction
*/
const collapsed = ref(isViewed.value);

function getIconForDiffStatus(pType: FileStatus) {
const diffTypes: Record<FileStatus, { name: SvgName, classes: Array<string> }> = {
Expand All @@ -35,7 +101,7 @@ function fileIcon(file: File) {
<!--title instead of tooltip above as the tooltip needs too much work with the current methods, i.e. not being loaded or staying open for "too long"-->
<a
v-if="item.isFile" class="item-file"
:class="{ 'selected': store.selectedItem === '#diff-' + item.file.NameHash, 'viewed': item.file.IsViewed }"
:class="{ 'selected': store.selectedItem === '#diff-' + item.file.NameHash, 'viewed': isViewed }"
:title="item.name" :href="'#diff-' + item.file.NameHash"
>
<!-- file -->
Expand All @@ -48,7 +114,7 @@ function fileIcon(file: File) {
</a>

<template v-else-if="item.isFile === false">
<div class="item-directory" :title="item.name" @click.stop="collapsed = !collapsed">
<div class="item-directory" :class="{ 'viewed': isViewed }" :title="item.name" @click.stop="collapsed = !collapsed">
<!-- directory -->
<SvgIcon :name="collapsed ? 'octicon-chevron-right' : 'octicon-chevron-down'"/>
<SvgIcon
Expand All @@ -59,7 +125,7 @@ function fileIcon(file: File) {
</div>

<div v-show="!collapsed" class="sub-items">
<DiffFileTreeItem v-for="childItem in item.children" :key="childItem.name" :item="childItem"/>
<DiffFileTreeItem v-for="childItem in item.children" :key="childItem.name" :item="childItem" :set-viewed="setCount"/>
</div>
</template>
</template>
Expand Down Expand Up @@ -88,7 +154,8 @@ a:hover {
border-radius: 4px;
}

.item-file.viewed {
.item-file.viewed,
.item-directory.viewed {
color: var(--color-text-light-3);
}

Expand Down
2 changes: 1 addition & 1 deletion web_src/js/utils/filetree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export type File = {
IsSubmodule: boolean;
}

type DirItem = {
export type DirItem = {
isFile: false;
name: string;
path: string;
Expand Down