From c7ba85ab647ef1278cbb611abfcb30ea6746327b Mon Sep 17 00:00:00 2001 From: "Jan C. Borchardt" <925062+jancborchardt@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:46:11 +0200 Subject: [PATCH] feat(ConflictPicker): simplify the file conflict dialog Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Jan C. Borchardt <925062+jancborchardt@users.noreply.github.com> --- .../ConflictPicker/ConflictPicker.vue | 145 +++++++++---- .../ConflictPicker/ConflictPickerCard.vue | 142 +++++++++++++ .../ConflictPicker/ConflictPickerEntry.vue | 184 +++++++--------- lib/conflict-picker.ts | 2 +- tests/components/ConflictPicker.spec.ts | 199 +++++++++--------- 5 files changed, 415 insertions(+), 257 deletions(-) create mode 100644 lib/components/ConflictPicker/ConflictPickerCard.vue diff --git a/lib/components/ConflictPicker/ConflictPicker.vue b/lib/components/ConflictPicker/ConflictPicker.vue index 6f78df9df..1e3bf2eb3 100644 --- a/lib/components/ConflictPicker/ConflictPicker.vue +++ b/lib/components/ConflictPicker/ConflictPicker.vue @@ -7,7 +7,8 @@ import type { INode } from '@nextcloud/files' import type { ConflictInput, ConflictResolutionResult } from '../../conflict-picker.ts' -import { mdiArrowRight, mdiClose } from '@mdi/js' +import { mdiArrowRight } from '@mdi/js' +import { basename } from '@nextcloud/paths' import { computed, shallowRef, useTemplateRef } from 'vue' import NcButton from '@nextcloud/vue/components/NcButton' import NcCheckboxRadioSwitch from '@nextcloud/vue/components/NcCheckboxRadioSwitch' @@ -25,9 +26,9 @@ const props = defineProps<{ container?: string | undefined /** - * Directory/context file name + * Directory with the conflicts, its name is shown in the description */ - dirname: string + dirname?: string /** * The existing nodes (same names as the conflicts) @@ -59,7 +60,10 @@ const blockedTitle = t('You need to select at least one version of each file to const formElement = useTemplateRef('form') const conflictEntries = useTemplateRef('conflictEntry') -const newSelected = shallowRef([]) +const isSingle = computed(() => props.incoming.length === 1) + +// Preselect the new files so the user can continue right away. +const newSelected = shallowRef([...props.incoming]) const oldSelected = shallowRef([]) const isNoNewSelected = computed(() => newSelected.value.length === 0) @@ -77,9 +81,21 @@ const areConflictsResolved = computed(() => { return true }) -const dialogName = computed(() => props.dirname?.trim() !== '' - ? n('%n file conflict in {dirname}', '%n file conflicts in {dirname}', props.incoming.length, { dirname: props.dirname }) - : n('%n file conflict', '%n files conflict', props.incoming.length)) +const dialogName = computed(() => isSingle.value + ? t('Select file to keep') + : t('Select files to keep')) + +// Only the folder name, the root folder is called "All files" in the Files app +const folderName = computed(() => basename(props.dirname ?? '') || t('All files')) + +// Split on the placeholder so the folder name can be shown in bold +const descriptionParts = computed(() => { + const message = isSingle.value + ? t('An item with the same name already exists in {dirname}.') + : t('Items with the same name already exist in {dirname}, select which to keep.') + const [before, after = ''] = message.split('{dirname}') + return { before, after } +}) /** * Cancel the conflic resolution (closing the dialog) @@ -88,6 +104,28 @@ function onCancel() { emit('close', null) } +/** + * Single-file: replace the existing file with the new one. + */ +function onReplace() { + emit('close', { + selected: [...props.incoming], + renamed: [], + skipped: [], + }) +} + +/** + * Single-file: keep both, the new file is uploaded with a renamed copy. + */ +function onKeepBoth() { + emit('close', { + selected: [], + renamed: [...props.incoming], + skipped: [], + }) +} + /** * Skip all conflicts (no upload will be performed) */ @@ -185,8 +223,10 @@ function onSubmit() {

- {{ t('Which files do you want to keep?') }}
- {{ t('If you select both versions, the incoming file will have a number added to its name.') }}
+ {{ descriptionParts.before }}{{ folderName }}{{ descriptionParts.after }}
+ @@ -205,22 +245,23 @@ function onSubmit() { aria-labelledby="conflict-picker-description" :class="$style.pickerForm" @submit.prevent.stop="onSubmit"> - -

+ +
{{ t('Select all checkboxes') }} - - {{ t('Select all new files') }} - - {{ t('Select all existing files') }} + {{ t('Existing files') }} + + + + {{ t('New files') }}
@@ -229,6 +270,7 @@ function onSubmit() { v-for="(node, index) in existing" ref="conflictEntry" :key="node.fileid" + :single="isSingle" :incoming="incoming[index]!" :existing="node" :incomingSelected="newSelected.includes(incoming[index]!)" @@ -245,39 +287,46 @@ function onSubmit() { data-cy-conflict-picker-cancel variant="tertiary" @click="onCancel"> - {{ t('Cancel') }} - - - {{ incoming.length === 1 ? t('Skip this file') : n('Skip %n file', 'Skip %n files', incoming.length) }} - - - - {{ t('Continue') }} - {{ blockedTitle }} - + + + + + @@ -286,6 +335,8 @@ function onSubmit() { .picker { --margin: 36px; --secondary-margin: 18px; + // shared width of the middle arrow column, so headings and entries line up + --arrow-column: 44px; } .pickerHeader { @@ -315,11 +366,13 @@ function onSubmit() { width: 100%; margin-top: calc(var(--secondary-margin) * 1.5); padding-bottom: var(--secondary-margin); - grid-template-columns: 1fr 1fr; + grid-template-columns: 1fr var(--arrow-column) 1fr; + align-items: center; legend { display: flex; align-items: center; + grid-column: 1 / -1; width: 100%; margin-bottom: calc(var(--secondary-margin) / 2); } diff --git a/lib/components/ConflictPicker/ConflictPickerCard.vue b/lib/components/ConflictPicker/ConflictPickerCard.vue new file mode 100644 index 000000000..677dcea5a --- /dev/null +++ b/lib/components/ConflictPicker/ConflictPickerCard.vue @@ -0,0 +1,142 @@ + + + + + + + diff --git a/lib/components/ConflictPicker/ConflictPickerEntry.vue b/lib/components/ConflictPicker/ConflictPickerEntry.vue index 4a75b190c..ccd05db24 100644 --- a/lib/components/ConflictPicker/ConflictPickerEntry.vue +++ b/lib/components/ConflictPicker/ConflictPickerEntry.vue @@ -7,12 +7,12 @@ import type { INode } from '@nextcloud/files' import type { ConflictInput } from '../../conflict-picker.ts' -import { mdiFile, mdiFolder } from '@mdi/js' +import { mdiArrowRight } from '@mdi/js' import { FileType } from '@nextcloud/files' import { computed, ref, watch } from 'vue' import NcCheckboxRadioSwitch from '@nextcloud/vue/components/NcCheckboxRadioSwitch' -import NcDateTime from '@nextcloud/vue/components/NcDateTime' import NcIconSvgWrapper from '@nextcloud/vue/components/NcIconSvgWrapper' +import ConflictPickerCard from './ConflictPickerCard.vue' import { getPreviewURL } from '../../composables/preview.ts' import { t } from '../../utils/l10n.ts' @@ -36,6 +36,12 @@ const props = defineProps<{ * The new incoming node */ incoming: ConflictInput + + /** + * Single-file mode: render the two versions without checkboxes. + * The parent then offers "Keep both"/"Replace" buttons instead. + */ + single?: boolean }>() defineExpose({ validate }) @@ -68,6 +74,12 @@ const isIncomingFolder = computed(() => { return props.incoming.type === FileType.Folder }) +// Bold whichever value differs to ease comparison: the newer date and the larger size. +const incomingNewer = computed(() => !!incomingMTime.value && !!existingMTime.value && incomingMTime.value > existingMTime.value) +const existingNewer = computed(() => !!incomingMTime.value && !!existingMTime.value && existingMTime.value > incomingMTime.value) +const incomingLarger = computed(() => incomingSize.value !== undefined && existingSize.value !== undefined && incomingSize.value > existingSize.value) +const existingLarger = computed(() => incomingSize.value !== undefined && existingSize.value !== undefined && existingSize.value > incomingSize.value) + watch(() => props.existing, async () => { existingMTime.value = getMTime(props.existing) existingSize.value = getSize(props.existing) @@ -179,133 +191,91 @@ function validate() {
{{ existing.displayname }} - + - - - - - - - - - {{ t('New version') }} - - - - {{ t('Last modified date unknown') }} - - - {{ incomingSize }} - - - + - - + + + + + + - - - - - - - - - {{ t('Existing version') }} - - - - {{ t('Last modified date unknown') }} - - - {{ existingSize }} - - - + +
diff --git a/lib/conflict-picker.ts b/lib/conflict-picker.ts index 5929f70fe..46f114f4d 100644 --- a/lib/conflict-picker.ts +++ b/lib/conflict-picker.ts @@ -54,7 +54,7 @@ export interface ConflictPickerOptions { * Given the current content of the directory and the conflicting new nodes, * this will ask the user for resolving the conflicts and return the conflict resolution. * - * @param dirname - The directory name with conflicts (used for the dialog title) + * @param dirname - The directory name with conflicts (its name is shown in the description) * @param conflicts - The incoming nodes * @param content - The current content of the directory (existing nodes) * @param options Optional settings for the conflict picker diff --git a/tests/components/ConflictPicker.spec.ts b/tests/components/ConflictPicker.spec.ts index bccf15b8d..b8a8ae344 100644 --- a/tests/components/ConflictPicker.spec.ts +++ b/tests/components/ConflictPicker.spec.ts @@ -6,7 +6,7 @@ import type { ConflictInput, ConflictResolutionResult } from '../../lib/conflict-picker.ts' import { File as NcFile } from '@nextcloud/files' -import { cleanup, findByText, fireEvent, getAllByRole, getByRole, render } from '@testing-library/vue' +import { cleanup, findByText, fireEvent, getAllByRole, getByRole, queryAllByRole, render } from '@testing-library/vue' import { readFile } from 'fs/promises' import { join } from 'path' import { afterEach, beforeAll, describe, expect, test } from 'vitest' @@ -14,7 +14,7 @@ import ConflictPicker from '../../lib/components/ConflictPicker/ConflictPicker.v afterEach(cleanup) -test('Renders default ConflictPicker', async () => { +describe('Single file conflict', () => { const oldImage = new NcFile({ id: 1, root: '/files/user', @@ -25,25 +25,83 @@ test('Renders default ConflictPicker', async () => { mtime: new Date('2021-01-01T00:00:00.000Z'), }) - const image = new File([], 'image.jpg') - render(ConflictPicker, { - props: { + test('Renders without checkboxes and with keep both / replace buttons', async () => { + const image = new File([], 'image.jpg') + render(ConflictPicker, { + props: { + container: getContainer(), + dirname: 'Pictures', + existing: [oldImage], + incoming: [image], + }, + }) + + const dialog = getByRole(document.body, 'dialog', { name: 'Select file to keep' }) + expect(dialog).toBeInstanceOf(HTMLElement) + + // No checkboxes for a single file + expect(queryAllByRole(dialog, 'checkbox')).toHaveLength(0) + + expect(getByRole(dialog, 'button', { name: 'Cancel' })).toBeInstanceOf(HTMLElement) + expect(getByRole(dialog, 'button', { name: 'Keep both' })).toBeInstanceOf(HTMLElement) + expect(getByRole(dialog, 'button', { name: 'Replace' })).toBeInstanceOf(HTMLElement) + }) + + test('Shows only the folder name, the root is called "All files"', async () => { + const image = new File([], 'image.jpg') + const props = { container: getContainer(), - dirname: 'Pictures', existing: [oldImage], incoming: [image], - }, + } + + const component = render(ConflictPicker, { props: { ...props, dirname: '/Photos/Sub folder' } }) + const dialog = getByRole(document.body, 'dialog') + expect(dialog.querySelector('strong')!.textContent).toBe('Sub folder') + + await component.rerender({ ...props, dirname: '/' }) + expect(dialog.querySelector('strong')!.textContent).toBe('All files') }) - const dialog = getByRole(document.body, 'dialog') - expect(dialog).toBeInstanceOf(HTMLElement) - expect(getByRole(document.body, 'dialog', { name: /1 file conflict in Pictures/ })).toBeInstanceOf(HTMLElement) + test('Replace keeps only the new file', async () => { + const image = new File([], 'image.jpg') + const component = render(ConflictPicker, { + props: { + container: getContainer(), + dirname: 'Pictures', + existing: [oldImage], + incoming: [image], + }, + }) + + const dialog = getByRole(document.body, 'dialog') + await fireEvent(getByRole(dialog, 'button', { name: 'Replace' }), new MouseEvent('click', { bubbles: true })) + + const [result] = component.emitted('close')[0]! as [ConflictResolutionResult] + expect(result.selected).toEqual([image]) + expect(result.renamed).toHaveLength(0) + expect(result.skipped).toHaveLength(0) + }) + + test('Keep both renames the new file', async () => { + const image = new File([], 'image.jpg') + const component = render(ConflictPicker, { + props: { + container: getContainer(), + dirname: 'Pictures', + existing: [oldImage], + incoming: [image], + }, + }) - expect(getByRole(dialog, 'form')).toBeInstanceOf(HTMLElement) - expect(getAllByRole(dialog, 'checkbox')).toHaveLength(4) + const dialog = getByRole(document.body, 'dialog') + await fireEvent(getByRole(dialog, 'button', { name: 'Keep both' }), new MouseEvent('click', { bubbles: true })) - expect(getByRole(dialog, 'checkbox', { name: 'Select all new files' })).toBeInstanceOf(HTMLElement) - expect(getByRole(dialog, 'checkbox', { name: 'Select all existing files' })).toBeInstanceOf(HTMLElement) + const [result] = component.emitted('close')[0]! as [ConflictResolutionResult] + expect(result.renamed).toEqual([image]) + expect(result.selected).toHaveLength(0) + expect(result.skipped).toHaveLength(0) + }) }) describe('ConflictPicker resolving', () => { @@ -109,7 +167,7 @@ describe('ConflictPicker resolving', () => { await expect(findByText(dialog, /folder is selected, the content is written into the existing folder/)).resolves.not.toThrow() }) - test('Pick all incoming files', async () => { + test('New files are preselected so the user can continue right away', async () => { const component = render(ConflictPicker, { props: { container: getContainer(), @@ -120,34 +178,22 @@ describe('ConflictPicker resolving', () => { }) const dialog = getByRole(document.body, 'dialog') - expect(dialog).toBeInstanceOf(HTMLElement) - const checkbox: HTMLInputElement = getByRole(dialog, 'checkbox', { name: /Select all new files/ }) - expect(checkbox.checked).toBe(false) + const selectAllNew: HTMLInputElement = getByRole(dialog, 'checkbox', { name: 'New files' }) + expect(selectAllNew.checked).toBe(true) const individualCheckboxes: HTMLInputElement[] = getAllByRole(dialog, 'checkbox', { name: /New version/ }) expect(individualCheckboxes).toHaveLength(2) - for (const box of individualCheckboxes) { - expect(box.checked).toBe(false) - } - - await fireEvent(checkbox, new MouseEvent('click', { bubbles: true })) - expect(checkbox.checked).toBe(true) for (const box of individualCheckboxes) { expect(box.checked).toBe(true) } const submit = getByRole(dialog, 'button', { name: 'Continue' }) - expect(submit).toBeInstanceOf(HTMLButtonElement) await fireEvent(submit, new MouseEvent('click', { bubbles: true })) - const events: [ConflictResolutionResult][] = component.emitted('close') - expect(events).toHaveLength(1) - const [result] = events[0]! - + const [result] = component.emitted('close')[0]! as [ConflictResolutionResult] expect(result.renamed).toHaveLength(0) expect(result.skipped).toHaveLength(0) - expect(result.selected).toHaveLength(2) expect(result.selected).toEqual([...images]) }) @@ -162,34 +208,20 @@ describe('ConflictPicker resolving', () => { }) const dialog = getByRole(document.body, 'dialog') - expect(dialog).toBeInstanceOf(HTMLElement) - const checkbox: HTMLInputElement = getByRole(dialog, 'checkbox', { name: /Select all existing files/ }) - expect(checkbox.checked).toBe(false) + const selectAllNew: HTMLInputElement = getByRole(dialog, 'checkbox', { name: 'New files' }) + const selectAllExisting: HTMLInputElement = getByRole(dialog, 'checkbox', { name: 'Existing files' }) - const individualCheckboxes: HTMLInputElement[] = getAllByRole(dialog, 'checkbox', { name: /Existing version/ }) - expect(individualCheckboxes).toHaveLength(2) - for (const box of individualCheckboxes) { - expect(box.checked).toBe(false) - } - - await fireEvent(checkbox, new MouseEvent('click', { bubbles: true })) - expect(checkbox.checked).toBe(true) - for (const box of individualCheckboxes) { - expect(box.checked).toBe(true) - } + // Deselect the (preselected) new files and select the existing ones instead + await fireEvent(selectAllNew, new MouseEvent('click', { bubbles: true })) + await fireEvent(selectAllExisting, new MouseEvent('click', { bubbles: true })) const submit = getByRole(dialog, 'button', { name: 'Continue' }) - expect(submit).toBeInstanceOf(HTMLButtonElement) await fireEvent(submit, new MouseEvent('click', { bubbles: true })) - const events: [ConflictResolutionResult][] = component.emitted('close') - expect(events).toHaveLength(1) - const [result] = events[0]! - + const [result] = component.emitted('close')[0]! as [ConflictResolutionResult] expect(result.renamed).toHaveLength(0) expect(result.selected).toHaveLength(0) - expect(result.skipped).toHaveLength(2) expect(result.skipped).toEqual([...images]) }) @@ -204,33 +236,17 @@ describe('ConflictPicker resolving', () => { }) const dialog = getByRole(document.body, 'dialog') - expect(dialog).toBeInstanceOf(HTMLElement) - const checkbox1: HTMLInputElement = getByRole(dialog, 'checkbox', { name: /Select all existing files/ }) - const checkbox2: HTMLInputElement = getByRole(dialog, 'checkbox', { name: /Select all new files/ }) - const individualCheckboxes: HTMLInputElement[] = getAllByRole(dialog, 'checkbox', { name: /(Existing|New) version/ }) - expect(individualCheckboxes).toHaveLength(4) - for (const box of individualCheckboxes) { - expect(box.checked).toBe(false) - } - - await fireEvent(checkbox1, new MouseEvent('click', { bubbles: true })) - await fireEvent(checkbox2, new MouseEvent('click', { bubbles: true })) - for (const box of individualCheckboxes) { - expect(box.checked).toBe(true) - } + // New files are preselected, additionally select the existing ones + const selectAllExisting: HTMLInputElement = getByRole(dialog, 'checkbox', { name: 'Existing files' }) + await fireEvent(selectAllExisting, new MouseEvent('click', { bubbles: true })) const submit = getByRole(dialog, 'button', { name: 'Continue' }) - expect(submit).toBeInstanceOf(HTMLButtonElement) await fireEvent(submit, new MouseEvent('click', { bubbles: true })) - const events: [ConflictResolutionResult][] = component.emitted('close') - expect(events).toHaveLength(1) - const [result] = events[0]! - + const [result] = component.emitted('close')[0]! as [ConflictResolutionResult] expect(result.selected).toHaveLength(0) expect(result.skipped).toHaveLength(0) - expect(result.renamed).toHaveLength(2) expect(result.renamed).toEqual([...images]) }) @@ -245,28 +261,20 @@ describe('ConflictPicker resolving', () => { }) const dialog = getByRole(document.body, 'dialog') - expect(dialog).toBeInstanceOf(HTMLElement) const group1 = getByRole(dialog, 'group', { name: 'image1.jpg' }) - const group2 = getByRole(dialog, 'group', { name: 'image2.jpg' }) - const existing: HTMLInputElement = getByRole(group1, 'checkbox', { name: /Existing version/ }) - const incoming: HTMLInputElement = getByRole(group2, 'checkbox', { name: /New version/ }) - expect(existing.checked).toBe(false) - expect(incoming.checked).toBe(false) + const existing1: HTMLInputElement = getByRole(group1, 'checkbox', { name: /Existing version/ }) + const incoming1: HTMLInputElement = getByRole(group1, 'checkbox', { name: /New version/ }) - await fireEvent(existing, new MouseEvent('click', { bubbles: true })) - await fireEvent(incoming, new MouseEvent('click', { bubbles: true })) - expect(existing.checked).toBe(true) - expect(incoming.checked).toBe(true) + // For image1 keep the existing file instead of the (preselected) new one + await fireEvent(incoming1, new MouseEvent('click', { bubbles: true })) + await fireEvent(existing1, new MouseEvent('click', { bubbles: true })) + // image2 keeps its preselected new file const submit = getByRole(dialog, 'button', { name: 'Continue' }) - expect(submit).toBeInstanceOf(HTMLButtonElement) await fireEvent(submit, new MouseEvent('click', { bubbles: true })) - const events: [ConflictResolutionResult][] = component.emitted('close') - expect(events).toHaveLength(1) - const [result] = events[0]! - + const [result] = component.emitted('close')[0]! as [ConflictResolutionResult] expect(result.renamed).toHaveLength(0) expect(result.skipped).toHaveLength(1) expect(result.skipped[0]!.name).toBe('image1.jpg') @@ -285,16 +293,11 @@ describe('ConflictPicker resolving', () => { }) const dialog = getByRole(document.body, 'dialog') - expect(dialog).toBeInstanceOf(HTMLElement) const submit = getByRole(dialog, 'button', { name: 'Skip 2 files' }) - expect(submit).toBeInstanceOf(HTMLButtonElement) await fireEvent(submit, new MouseEvent('click', { bubbles: true })) - const events: [ConflictResolutionResult][] = component.emitted('close') - expect(events).toHaveLength(1) - const [result] = events[0]! - + const [result] = component.emitted('close')[0]! as [ConflictResolutionResult] expect(result.renamed).toHaveLength(0) expect(result.selected).toHaveLength(0) expect(result.skipped).toHaveLength(2) @@ -312,16 +315,11 @@ describe('ConflictPicker resolving', () => { }) const dialog = getByRole(document.body, 'dialog') - expect(dialog).toBeInstanceOf(HTMLElement) const submit = getByRole(dialog, 'button', { name: 'Cancel' }) - expect(submit).toBeInstanceOf(HTMLButtonElement) await fireEvent(submit, new MouseEvent('click', { bubbles: true })) - const events: [ConflictResolutionResult][] = component.emitted('close') - expect(events).toHaveLength(1) - const [result] = events[0]! - + const [result] = component.emitted('close')[0]! as [ConflictResolutionResult | null] expect(result).toBeNull() }) @@ -336,16 +334,11 @@ describe('ConflictPicker resolving', () => { }) const dialog = getByRole(document.body, 'dialog') - expect(dialog).toBeInstanceOf(HTMLElement) const submit = getByRole(dialog, 'button', { name: 'Close' }) - expect(submit).toBeInstanceOf(HTMLButtonElement) await fireEvent(submit, new MouseEvent('click', { bubbles: true })) - const events: [ConflictResolutionResult][] = component.emitted('close') - expect(events).toHaveLength(1) - const [result] = events[0]! - + const [result] = component.emitted('close')[0]! as [ConflictResolutionResult | null] expect(result).toBeNull() }) })