-
Notifications
You must be signed in to change notification settings - Fork 498
[QPOv2] Add list view to assets sidepanel #7737
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
Changes from all commits
94884d7
5ddea4e
a0dad31
3d0c0d1
dc53cbe
2fd9a73
0c6ea56
5b14568
da48899
f614914
85c6825
627db67
afa4664
f128c61
55db4fe
5ed4e5f
9e477d4
e9d675e
ede934a
114ece5
f9eb1c4
7c82218
9c4f808
72a9571
ab09e7a
a8fa3e3
434ee52
578b8fb
0f02b9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,203 @@ | ||
| <template> | ||
| <div class="flex h-full flex-col"> | ||
| <div | ||
| v-if="activeJobItems.length" | ||
| class="flex max-h-[50%] flex-col gap-2 overflow-y-auto px-2" | ||
| > | ||
| <AssetsListItem | ||
| v-for="job in activeJobItems" | ||
| :key="job.id" | ||
| :class=" | ||
| cn( | ||
| 'w-full shrink-0 text-text-primary transition-colors hover:bg-secondary-background-hover', | ||
| 'cursor-default' | ||
| ) | ||
| " | ||
| :preview-url="job.iconImageUrl" | ||
| :preview-alt="job.title" | ||
| :icon-name="job.iconName" | ||
| :icon-class="getJobIconClass(job)" | ||
| :primary-text="job.title" | ||
| :secondary-text="job.meta" | ||
| :progress-total-percent="job.progressTotalPercent" | ||
| :progress-current-percent="job.progressCurrentPercent" | ||
| @mouseenter="onJobEnter(job.id)" | ||
| @mouseleave="onJobLeave(job.id)" | ||
christian-byrne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| @click.stop | ||
| > | ||
| <template v-if="hoveredJobId === job.id" #actions> | ||
| <Button | ||
| v-if="canCancelJob" | ||
| :variant="cancelAction.variant" | ||
| size="icon" | ||
| :aria-label="cancelAction.label" | ||
| @click.stop="runCancelJob()" | ||
| > | ||
| <i :class="cancelAction.icon" class="size-4" /> | ||
| </Button> | ||
christian-byrne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| </template> | ||
| </AssetsListItem> | ||
| </div> | ||
|
|
||
| <div | ||
| v-if="assets.length" | ||
| :class="cn('px-2', activeJobItems.length && 'mt-2')" | ||
| > | ||
| <div | ||
| class="flex items-center py-2 text-sm font-normal leading-normal text-muted-foreground font-inter" | ||
| > | ||
| {{ t('sideToolbar.generatedAssetsHeader') }} | ||
| </div> | ||
| </div> | ||
|
|
||
| <VirtualGrid | ||
| class="flex-1" | ||
| :items="assetItems" | ||
| :grid-style="listGridStyle" | ||
| @approach-end="emit('approach-end')" | ||
| > | ||
| <template #item="{ item }"> | ||
| <AssetsListItem | ||
| role="button" | ||
| tabindex="0" | ||
| :aria-label=" | ||
| t('assetBrowser.ariaLabel.assetCard', { | ||
| name: item.asset.name, | ||
| type: getMediaTypeFromFilename(item.asset.name) | ||
| }) | ||
| " | ||
| :class="getAssetCardClass(isSelected(item.asset.id))" | ||
| :preview-url="item.asset.preview_url" | ||
| :preview-alt="item.asset.name" | ||
| :icon-name=" | ||
| iconForMediaType(getMediaTypeFromFilename(item.asset.name)) | ||
| " | ||
| :primary-text="getAssetPrimaryText(item.asset)" | ||
| :secondary-text="getAssetSecondaryText(item.asset)" | ||
| @click.stop="emit('select-asset', item.asset)" | ||
| /> | ||
| </template> | ||
| </VirtualGrid> | ||
| </div> | ||
| </template> | ||
christian-byrne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| <script setup lang="ts"> | ||
| import { computed, ref } from 'vue' | ||
| import { useI18n } from 'vue-i18n' | ||
|
|
||
| import VirtualGrid from '@/components/common/VirtualGrid.vue' | ||
| import Button from '@/components/ui/button/Button.vue' | ||
| import { useJobActions } from '@/composables/queue/useJobActions' | ||
| import type { JobListItem } from '@/composables/queue/useJobList' | ||
| import { useJobList } from '@/composables/queue/useJobList' | ||
| import AssetsListItem from '@/platform/assets/components/AssetsListItem.vue' | ||
| import { getOutputAssetMetadata } from '@/platform/assets/schemas/assetMetadataSchema' | ||
| import type { AssetItem } from '@/platform/assets/schemas/assetSchema' | ||
| import { iconForMediaType } from '@/platform/assets/utils/mediaIconUtil' | ||
| import type { JobState } from '@/types/queue' | ||
| import { | ||
| formatDuration, | ||
| formatSize, | ||
| getMediaTypeFromFilename, | ||
| truncateFilename | ||
| } from '@/utils/formatUtil' | ||
| import { iconForJobState } from '@/utils/queueDisplay' | ||
| import { cn } from '@/utils/tailwindUtil' | ||
|
|
||
| const { assets, isSelected } = defineProps<{ | ||
| assets: AssetItem[] | ||
| isSelected: (assetId: string) => boolean | ||
| }>() | ||
|
|
||
| const emit = defineEmits<{ | ||
| (e: 'select-asset', asset: AssetItem): void | ||
| (e: 'approach-end'): void | ||
| }>() | ||
|
|
||
| const { t } = useI18n() | ||
| const { jobItems } = useJobList() | ||
| const hoveredJobId = ref<string | null>(null) | ||
christian-byrne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| type AssetListItem = { key: string; asset: AssetItem } | ||
|
|
||
| const activeJobItems = computed(() => | ||
| jobItems.value.filter((item) => isActiveJobState(item.state)) | ||
| ) | ||
| const hoveredJob = computed(() => | ||
| hoveredJobId.value | ||
| ? (activeJobItems.value.find((job) => job.id === hoveredJobId.value) ?? | ||
| null) | ||
| : null | ||
| ) | ||
| const { cancelAction, canCancelJob, runCancelJob } = useJobActions(hoveredJob) | ||
|
|
||
| const assetItems = computed<AssetListItem[]>(() => | ||
| assets.map((asset) => ({ | ||
| key: `asset-${asset.id}`, | ||
| asset | ||
| })) | ||
| ) | ||
|
|
||
| const listGridStyle = { | ||
| display: 'grid', | ||
| gridTemplateColumns: 'minmax(0, 1fr)', | ||
| padding: '0 0.5rem', | ||
| gap: '0.5rem' | ||
| } | ||
christian-byrne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| function isActiveJobState(state: JobState): boolean { | ||
| return ( | ||
| state === 'pending' || state === 'initialization' || state === 'running' | ||
| ) | ||
| } | ||
|
Comment on lines
+148
to
+152
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Eliminate code duplication with useJobActions. The ♻️ Proposed refactorOption 1: Export a shared constant from a common location Create or use an existing constants file (e.g., // src/types/queue.ts or src/constants/queue.ts
export const ACTIVE_JOB_STATES: JobState[] = [
'pending',
'initialization',
'running'
] as constThen import and use in both files: In +import { ACTIVE_JOB_STATES } from '@/types/queue'
+
export function useJobActions(
job: MaybeRefOrGetter<JobListItem | null | undefined>
) {
// ...
return (
currentJob.showClear !== false &&
- cancellableStates.includes(currentJob.state)
+ ACTIVE_JOB_STATES.includes(currentJob.state)
)
}In +import { ACTIVE_JOB_STATES } from '@/types/queue'
+
function isActiveJobState(state: JobState): boolean {
- return (
- state === 'pending' || state === 'initialization' || state === 'running'
- )
+ return ACTIVE_JOB_STATES.includes(state)
}Option 2: Import directly from useJobActions (if you export the constant) This centralizes the definition in the composable that owns the logic. This refactor improves maintainability by ensuring the definition of active/cancellable job states is managed in one place.
🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai make a tracking issue for this in the future
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 57 @christian-byrne I've created issue #7947 to track this refactoring. The issue documents the code duplication between 🧠 Learnings used |
||
|
|
||
| function getAssetPrimaryText(asset: AssetItem): string { | ||
| return truncateFilename(asset.name) | ||
| } | ||
|
|
||
| function getAssetSecondaryText(asset: AssetItem): string { | ||
| const metadata = getOutputAssetMetadata(asset.user_metadata) | ||
| if (typeof metadata?.executionTimeInSeconds === 'number') { | ||
| return `${metadata.executionTimeInSeconds.toFixed(2)}s` | ||
| } | ||
|
|
||
| const duration = asset.user_metadata?.duration | ||
| if (typeof duration === 'number') { | ||
| return formatDuration(duration) | ||
| } | ||
|
|
||
| if (typeof asset.size === 'number') { | ||
| return formatSize(asset.size) | ||
| } | ||
|
|
||
| return '' | ||
| } | ||
|
|
||
| function getAssetCardClass(selected: boolean): string { | ||
| return cn( | ||
| 'w-full text-text-primary transition-colors hover:bg-secondary-background-hover', | ||
| 'cursor-pointer', | ||
| selected && | ||
| 'bg-secondary-background-hover ring-1 ring-inset ring-modal-card-border-highlighted' | ||
| ) | ||
| } | ||
|
|
||
| function onJobEnter(jobId: string) { | ||
| hoveredJobId.value = jobId | ||
| } | ||
|
|
||
| function onJobLeave(jobId: string) { | ||
| if (hoveredJobId.value === jobId) { | ||
| hoveredJobId.value = null | ||
| } | ||
| } | ||
|
|
||
| function getJobIconClass(job: JobListItem): string | undefined { | ||
| const classes = [] | ||
| const iconName = job.iconName ?? iconForJobState(job.state) | ||
| if (!job.iconImageUrl && iconName === iconForJobState('pending')) { | ||
| classes.push('animate-spin') | ||
| } | ||
| return classes.length ? classes.join(' ') : undefined | ||
| } | ||
christian-byrne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| </script> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,10 +79,10 @@ | |
| <Divider v-else type="dashed" class="my-2" /> | ||
| </template> | ||
| <template #body> | ||
| <div v-if="loading && !displayAssets.length"> | ||
| <div v-if="showLoadingState"> | ||
| <ProgressSpinner class="absolute left-1/2 w-[50px] -translate-x-1/2" /> | ||
| </div> | ||
| <div v-else-if="!loading && !displayAssets.length"> | ||
| <div v-else-if="showEmptyState"> | ||
| <NoResultsPlaceholder | ||
| icon="pi pi-info-circle" | ||
| :title=" | ||
|
|
@@ -96,7 +96,15 @@ | |
| /> | ||
| </div> | ||
| <div v-else class="relative size-full" @click="handleEmptySpaceClick"> | ||
| <AssetsSidebarListView | ||
| v-if="isListView" | ||
| :assets="displayAssets" | ||
| :is-selected="isSelected" | ||
| @select-asset="handleAssetSelect" | ||
| @approach-end="handleApproachEnd" | ||
| /> | ||
christian-byrne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| <VirtualGrid | ||
| v-else | ||
| :items="mediaAssetsWithKey" | ||
| :grid-style="{ | ||
| display: 'grid', | ||
|
|
@@ -198,6 +206,7 @@ import { useI18n } from 'vue-i18n' | |
| import NoResultsPlaceholder from '@/components/common/NoResultsPlaceholder.vue' | ||
| import VirtualGrid from '@/components/common/VirtualGrid.vue' | ||
| import Load3dViewerContent from '@/components/load3d/Load3dViewerContent.vue' | ||
| import AssetsSidebarListView from '@/components/sidebar/tabs/AssetsSidebarListView.vue' | ||
| import SidebarTabTemplate from '@/components/sidebar/tabs/SidebarTabTemplate.vue' | ||
| import ResultGallery from '@/components/sidebar/tabs/queue/ResultGallery.vue' | ||
| import Tab from '@/components/tab/Tab.vue' | ||
|
|
@@ -232,6 +241,9 @@ const viewMode = ref<'list' | 'grid'>('grid') | |
| const isQueuePanelV2Enabled = computed(() => | ||
| settingStore.get('Comfy.Queue.QPOV2') | ||
| ) | ||
| const isListView = computed( | ||
| () => isQueuePanelV2Enabled.value && viewMode.value === 'list' | ||
| ) | ||
|
|
||
| // Track which asset's context menu is open (for single-instance context menu management) | ||
| const openContextMenuId = ref<string | null>(null) | ||
|
|
@@ -347,6 +359,20 @@ const displayAssets = computed(() => { | |
| return filteredAssets.value | ||
| }) | ||
|
|
||
| const showLoadingState = computed( | ||
| () => | ||
| loading.value && | ||
| displayAssets.value.length === 0 && | ||
| (!isListView.value || activeJobsCount.value === 0) | ||
| ) | ||
|
|
||
| const showEmptyState = computed( | ||
| () => | ||
| !loading.value && | ||
| displayAssets.value.length === 0 && | ||
| (!isListView.value || activeJobsCount.value === 0) | ||
| ) | ||
|
|
||
|
Comment on lines
+362
to
+375
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change was made because now the view can show even when there are no generated assets, because running jobs were added to this view |
||
| watch(displayAssets, (newAssets) => { | ||
| if (currentGalleryAssetId.value && galleryActiveIndex.value !== -1) { | ||
| const newIndex = newAssets.findIndex( | ||
|
|
||
christian-byrne marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import { computed, toValue } from 'vue' | ||
| import type { MaybeRefOrGetter } from 'vue' | ||
| import { useI18n } from 'vue-i18n' | ||
|
|
||
| import { useErrorHandling } from '@/composables/useErrorHandling' | ||
| import type { JobListItem } from '@/composables/queue/useJobList' | ||
| import { useJobMenu } from '@/composables/queue/useJobMenu' | ||
| import type { JobState } from '@/types/queue' | ||
|
|
||
| type JobAction = { | ||
| icon: string | ||
| label: string | ||
| variant: 'destructive' | 'secondary' | 'textonly' | ||
| } | ||
|
|
||
| export function useJobActions( | ||
| job: MaybeRefOrGetter<JobListItem | null | undefined> | ||
| ) { | ||
| const { t } = useI18n() | ||
| const { wrapWithErrorHandlingAsync } = useErrorHandling() | ||
| const { cancelJob } = useJobMenu() | ||
|
|
||
| const cancelAction: JobAction = { | ||
| icon: 'icon-[lucide--x]', | ||
| label: t('sideToolbar.queueProgressOverlay.cancelJobTooltip'), | ||
| variant: 'destructive' | ||
| } | ||
|
|
||
| const cancellableStates: JobState[] = ['pending', 'initialization', 'running'] | ||
|
Comment on lines
+23
to
+29
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Move static data outside the function for better performance. Both ♻️ Proposed refactor+const CANCELLABLE_STATES: JobState[] = [
+ 'pending',
+ 'initialization',
+ 'running'
+]
+
export function useJobActions(
job: MaybeRefOrGetter<JobListItem | null | undefined>
) {
const { t } = useI18n()
const { wrapWithErrorHandlingAsync } = useErrorHandling()
const { cancelJob } = useJobMenu()
- const cancelAction: JobAction = {
+ const cancelAction: JobAction = {
icon: 'icon-[lucide--x]',
label: t('sideToolbar.queueProgressOverlay.cancelJobTooltip'),
variant: 'destructive'
}
- const cancellableStates: JobState[] = ['pending', 'initialization', 'running']
-
const jobRef = computed(() => toValue(job) ?? null)
const canCancelJob = computed(() => {
const currentJob = jobRef.value
if (!currentJob) {
return false
}
return (
currentJob.showClear !== false &&
- cancellableStates.includes(currentJob.state)
+ CANCELLABLE_STATES.includes(currentJob.state)
)
})Note: Based on coding guidelines emphasizing immutability and minimal mutable state.
🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai make a tracking issue for this in the future
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 57 @christian-byrne, I've created tracking issue #7946 for the refactoring suggestion about moving static data outside the The issue has been assigned to you and includes the context, specific refactoring details, benefits, and backlinks to this PR and review comment. |
||
|
|
||
| const jobRef = computed(() => toValue(job) ?? null) | ||
|
|
||
| const canCancelJob = computed(() => { | ||
| const currentJob = jobRef.value | ||
| if (!currentJob) { | ||
| return false | ||
| } | ||
|
|
||
| return ( | ||
| currentJob.showClear !== false && | ||
| cancellableStates.includes(currentJob.state) | ||
| ) | ||
| }) | ||
|
|
||
| const runCancelJob = wrapWithErrorHandlingAsync(async () => { | ||
| const currentJob = jobRef.value | ||
| if (!currentJob) { | ||
| return | ||
| } | ||
|
|
||
| await cancelJob(currentJob) | ||
| }) | ||
|
|
||
| return { | ||
| cancelAction, | ||
| canCancelJob, | ||
| runCancelJob | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.