diff --git a/changelog/unreleased/enhancement-upload-improvements b/changelog/unreleased/enhancement-upload-improvements index ea8d82fbf8d..bd253ec0f50 100644 --- a/changelog/unreleased/enhancement-upload-improvements +++ b/changelog/unreleased/enhancement-upload-improvements @@ -1,12 +1,19 @@ Enhancement: Upload progress & overlay improvements -* Remove fetching of newly uploaded files to improve performance +* Remove fetching of newly uploaded files and folders to improve performance * Redesign the upload overlay * Show uploading files in the upload overlay * Immediately show the upload overlay when uploading folders to tell the user that the upload is starting * Only show top level folders in the upload overlay when uploading folders * Remove the Uppy StatusBar plugin +* Use `uppy.addFiles()` instead of calling `uppy.addFile()` for each file +* The upload overlay now shows a more proper time estimation when uploading many files +* The user feedback of the upload overlay during upload preparation has been improved +* Several router bugs have been fixed in the upload overlay +* Temporarily disabled `uploadDataDuringCreation` to prevent the local storage from getting to full +* Some more refactoring under the hood has been made https://github.com/owncloud/web/pull/7067 +https://github.com/owncloud/web/pull/7127 https://github.com/owncloud/web/issues/7066 https://github.com/owncloud/web/issues/7069 diff --git a/packages/web-app-files/src/components/AppBar/CreateAndUpload.vue b/packages/web-app-files/src/components/AppBar/CreateAndUpload.vue index d0f32e0bf80..5286d5f98e2 100644 --- a/packages/web-app-files/src/components/AppBar/CreateAndUpload.vue +++ b/packages/web-app-files/src/components/AppBar/CreateAndUpload.vue @@ -732,7 +732,12 @@ export default defineComponent({ this.$uppyService.publish('uploadStarted') await this.createDirectoryTree(files) this.$uppyService.publish('addedForUpload', files) - await this.updateStoreForCreatedFolders(files) + + const reloadRequired = !!files.find((f) => f.meta.currentFolder === this.currentPath) + if (reloadRequired) { + bus.publish('app.files.list.load') + } + this.$uppyService.uploadFiles(files) }, diff --git a/packages/web-app-files/src/composables/upload/useUploadHelpers.ts b/packages/web-app-files/src/composables/upload/useUploadHelpers.ts index 6221f0d0c08..5f40979cabb 100644 --- a/packages/web-app-files/src/composables/upload/useUploadHelpers.ts +++ b/packages/web-app-files/src/composables/upload/useUploadHelpers.ts @@ -1,10 +1,7 @@ import { Route } from 'vue-router' import { UppyResource } from 'web-runtime/src/composables/upload' -import { buildResource, buildWebDavFilesPath, buildWebDavSpacesPath } from '../../helpers/resources' +import { buildWebDavFilesPath, buildWebDavSpacesPath } from '../../helpers/resources' import { User } from '../../helpers/user' -import { DavProperties } from 'web-pkg/src/constants' -import { ClientService } from 'web-pkg/src/services' -import { Store } from 'vuex' import { useCapabilityShareJailEnabled, useClientService, @@ -16,10 +13,10 @@ import { isLocationPublicActive, isLocationSpacesActive } from '../../router' import { computed, onMounted, ref, Ref, unref } from '@vue/composition-api' import { SHARE_JAIL_ID } from '../../services/folder' import * as uuid from 'uuid' +import path from 'path' interface UploadHelpersResult { inputFilesToUppyFiles(inputFileOptions): UppyResource[] - updateStoreForCreatedFolders(files: UppyResource[]): void currentPath: Ref uploadPath: Ref personalDriveId: Ref @@ -38,7 +35,6 @@ export function useUploadHelpers(): UploadHelpersResult { const getToken = computed((): string => store.getters.getToken) const server = computed((): string => store.getters.configuration.server) const hasShareJail = useCapabilityShareJailEnabled() - const publicLinkPassword = computed((): string => store.getters['Files/publicLinkPassword']) const isPublicLocation = useActiveLocation(isLocationPublicActive, 'files-public-files') const isSpacesProjectLocation = useActiveLocation(isLocationSpacesActive, 'files-spaces-project') const isSpacesShareLocation = useActiveLocation(isLocationSpacesActive, 'files-spaces-share') @@ -103,12 +99,6 @@ export function useUploadHelpers(): UploadHelpersResult { currentPath, webDavBasePath }), - updateStoreForCreatedFolders: updateStoreForCreatedFolders({ - clientService, - store, - isPublicLocation, - publicLinkPassword - }), currentPath, uploadPath, personalDriveId @@ -125,52 +115,6 @@ const getPersonalDriveId = async (clientService, server: string, getToken: strin return drivesResponse.data.value[0].id } -const updateStoreForCreatedFolders = ({ - clientService, - store, - isPublicLocation, - publicLinkPassword -}: { - clientService: ClientService - store: Store - isPublicLocation: Ref - publicLinkPassword: Ref -}) => { - return async (files: UppyResource[]) => { - const { owncloudSdk: client } = clientService - const fetchedFolders = [] - for (const file of files) { - if (!file.meta.relativeFolder) { - continue - } - - const relativeFolder = `/${file.meta.relativeFolder.replace(/^\/+/, '')}` - // Only care about the root folders, no need to fetch nested folders - const rootFolder = relativeFolder.split('/').slice(0, 2).join('/') - const rootFolderPath = `${file.meta.webDavBasePath}/${rootFolder}` - - if (fetchedFolders.includes(rootFolderPath)) { - continue - } - - let resource - if (unref(isPublicLocation)) { - resource = await client.publicFiles.getFileInfo( - `${file.meta.currentFolder}${rootFolder}`, - unref(publicLinkPassword), - DavProperties.PublicLink - ) - } else { - resource = await client.files.fileInfo(rootFolderPath, DavProperties.Default) - } - - resource = buildResource(resource) - store.commit('Files/UPSERT_RESOURCE', resource) - fetchedFolders.push(rootFolderPath) - } - } -} - const inputFilesToUppyFiles = ({ route, uploadPath, @@ -180,33 +124,25 @@ const inputFilesToUppyFiles = ({ return (files: File[]): UppyResource[] => { const uppyFiles: UppyResource[] = [] - const { params } = unref(route) + const { name, params, query } = unref(route) const currentFolder = unref(currentPath) + const trimmedUploadPath = unref(uploadPath).replace(/\/+$/, '') const topLevelFolderIds = {} for (const file of files) { // Get the relative path of the file when the file was inside a directory on the client computer const relativeFilePath = file.webkitRelativePath || (file as any).relativePath || null // Directory without filename - const directory = relativeFilePath - ? relativeFilePath.substring(0, relativeFilePath.lastIndexOf('/')) - : '' + const directory = + !relativeFilePath || path.dirname(relativeFilePath) === '.' + ? '' + : path.dirname(relativeFilePath) // Build tus endpoint to dynamically set it on file upload. // Looks something like: https://localhost:9200/remote.php/dav/files/admin - let tusEndpoint - if (directory) { - tusEndpoint = `${unref(uploadPath).replace(/\/+$/, '')}/${directory.replace(/^\/+/, '')}` - } else { - tusEndpoint = unref(uploadPath) - } - - // Build the route object for this file. This is used later by the upload-info-box - const item = params.item ? `${params.item}/${directory}` : directory - const fileRoute = { - ...unref(route), - params: { ...params, item } - } + const tusEndpoint = directory + ? `${trimmedUploadPath}/${directory.replace(/^\/+/, '')}` + : unref(uploadPath) let topLevelFolderId if (relativeFilePath) { @@ -230,12 +166,13 @@ const inputFilesToUppyFiles = ({ webDavBasePath: unref(webDavBasePath), // WebDAV base path where the files will be uploaded to uploadId: uuid.v4(), topLevelFolderId, - routeName: fileRoute.name, - routeItem: fileRoute.params?.item || '', - routeShareName: (fileRoute.params as any)?.shareName || '', - routeShareId: (fileRoute.query as any)?.shareId || '', - routeStorage: (fileRoute.params as any)?.storage || '', - routeStorageId: (fileRoute.params as any)?.storageId || '' + routeName: name, + routeItem: params.item ? `${params.item}/${directory}` : directory, + routeShareName: (params as any)?.shareName || '', + routeShareId: (query as any)?.shareId || '', + routeStorage: (params as any)?.storage || '', + routeStorageId: (params as any)?.storageId || '', + routeParamName: (params as any)?.name || '' } }) } diff --git a/packages/web-app-files/tests/unit/composables/upload/useUploadHelpers.spec.ts b/packages/web-app-files/tests/unit/composables/upload/useUploadHelpers.spec.ts index f6c4ee04828..4dd93e8b6eb 100644 --- a/packages/web-app-files/tests/unit/composables/upload/useUploadHelpers.spec.ts +++ b/packages/web-app-files/tests/unit/composables/upload/useUploadHelpers.spec.ts @@ -45,50 +45,4 @@ describe('useUploadHelpers', () => { { currentItem: currentPathMock, fileUrl: uploadPathMock } ) }) - - it('should update the store for newly created folders', async () => { - const wrapper = createWrapper( - () => { - const { updateStoreForCreatedFolders } = useUploadHelpers() - return { updateStoreForCreatedFolders } - }, - { currentItem: currentPathMock, fileUrl: uploadPathMock } - ) - - const spyStoreCommit = jest.spyOn(wrapper.vm.$store, 'commit') - - const uppyResources = [ - { - source: 'source', - name: 'file1', - type: 'type', - data: new Blob(), - meta: { - currentFolder: 'currentFolder', - relativeFolder: 'l1/l2/l3', - relativePath: 'relativePath', - route: { name: 'files-personal' }, - tusEndpoint: 'tusEndpoint', - webDavBasePath: 'webDavBasePath' - } - }, - { - source: 'source', - name: 'file2', - type: 'type', - data: new Blob(), - meta: { - currentFolder: 'currentFolder', - relativeFolder: 'l1/l2/l3', - relativePath: 'relativePath', - route: { name: 'files-personal' }, - tusEndpoint: 'tusEndpoint', - webDavBasePath: 'webDavBasePath' - } - } - ] - - await wrapper.vm.updateStoreForCreatedFolders(uppyResources) - expect(spyStoreCommit).toHaveBeenCalledTimes(1) - }) }) diff --git a/packages/web-runtime/src/components/UploadInfo.vue b/packages/web-runtime/src/components/UploadInfo.vue index 79079fb1712..8a1687fabae 100644 --- a/packages/web-runtime/src/components/UploadInfo.vue +++ b/packages/web-runtime/src/components/UploadInfo.vue @@ -56,7 +56,7 @@ !f.isFolder).length for (const file of files) { + if (file.data?.size) { + this.bytesTotal += file.data.size + } + const { relativeFolder, uploadId, topLevelFolderId } = file.meta const isTopLevelItem = !relativeFolder // only add top level items to this.uploads because we only show those @@ -264,11 +269,11 @@ export default { this.$uppyService.subscribe('upload-progress', ({ file, progress }) => { if (!this.timeStarted) { this.timeStarted = new Date() + this.inPreparation = false } if (this.filesInEstimation[file.meta.uploadId] === undefined) { this.filesInEstimation[file.meta.uploadId] = 0 - this.bytesTotal += progress.bytesTotal } const byteIncrease = progress.bytesUploaded - this.filesInEstimation[file.meta.uploadId] @@ -418,6 +423,7 @@ export default { this.filesInEstimation = {} this.timeStarted = null this.remainingTime = undefined + this.inPreparation = true }, displayFileAsResource(file) { return !!file.targetRoute @@ -426,10 +432,10 @@ export default { return file.isFolder === true }, folderLink(file) { - return this.createFolderLink(file.path, file.storageId, file.targetRoute) + return this.createFolderLink(file.path, file.targetRoute) }, parentFolderLink(file) { - return this.createFolderLink(path.dirname(file.path), file.storageId, file.targetRoute) + return this.createFolderLink(path.dirname(file.path), file.targetRoute) }, buildRouteFromUppyResource(resource) { if (!resource.meta.routeName) { @@ -445,7 +451,8 @@ export default { item: resource.meta.routeItem, shareName: resource.meta.routeShareName, storage: resource.meta.routeStorage, - storageId: resource.meta.routeStorageId + storageId: resource.meta.routeStorageId, + name: resource.meta.routeParamName } } }, @@ -465,7 +472,7 @@ export default { } return this.hasShareJail ? this.$gettext('Personal') : this.$gettext('All files and folders') }, - createFolderLink(path, storageId, targetRoute) { + createFolderLink(path, targetRoute) { if (!targetRoute) { return {} } @@ -475,7 +482,8 @@ export default { name: targetRoute.name, query: targetRoute.query, params: { - ...(storageId && path && { storageId }), + ...(targetRoute.params?.storageId && + path && { storageId: targetRoute.params?.storageId }), ...(targetRoute.params?.storage && { storage: targetRoute.params?.storage }), ...(targetRoute.params?.shareName && { shareName: targetRoute.params?.shareName }) } diff --git a/packages/web-runtime/src/composables/upload/useUpload.ts b/packages/web-runtime/src/composables/upload/useUpload.ts index b10a7be439b..3afa1db6a1c 100644 --- a/packages/web-runtime/src/composables/upload/useUpload.ts +++ b/packages/web-runtime/src/composables/upload/useUpload.ts @@ -33,6 +33,7 @@ export interface UppyResource { routeShareId?: string routeStorage?: string routeStorageId?: string + routeParamName?: string } } @@ -163,8 +164,10 @@ const createDirectoryTree = ({ routeName: file.meta.routeName, routeItem: file.meta.routeItem, routeShareName: file.meta.routeShareName, + routeShareId: file.meta.routeShareId, routeStorage: file.meta.routeStorage, - routeStorageId: file.meta.routeStorageId + routeStorageId: file.meta.routeStorageId, + routeParamName: file.meta.routeParamName } } diff --git a/packages/web-runtime/src/services/uppyService.ts b/packages/web-runtime/src/services/uppyService.ts index d785e345386..35ab45b58e1 100644 --- a/packages/web-runtime/src/services/uppyService.ts +++ b/packages/web-runtime/src/services/uppyService.ts @@ -13,7 +13,6 @@ type UppyServiceTopics = | 'uploadRemoved' | 'uploadSuccess' | 'uploadError' - | 'fileAdded' | 'filesSelected' | 'progress' | 'addedForUpload' @@ -50,7 +49,8 @@ export class UppyService { removeFingerprintOnSuccess: true, overridePatchMethod: !!tusHttpMethodOverride, retryDelays: [0, 500, 1000], - uploadDataDuringCreation + // @TODO Use uploadDataDuringCreation once https://github.com/tus/tus-js-client/issues/397 is solved + uploadDataDuringCreation: false } const xhrPlugin = this.uppy.getPlugin('XHRUpload') @@ -161,7 +161,6 @@ export class UppyService { this.clearInputs() }) this.uppy.on('file-added', (file) => { - this.publish('fileAdded') const addedFile = file as unknown as UppyResource if (this.uppy.getPlugin('XHRUpload')) { const escapedName = encodeURIComponent(addedFile.name) @@ -192,20 +191,7 @@ export class UppyService { } uploadFiles(files: UppyResource[]) { - files.forEach((file) => { - try { - this.uppy.addFile(file) - } catch (err) { - console.error('error upload file:', file) - if (err.isRestriction) { - // handle restrictions - console.error('Restriction error:', err) - } else { - // handle other errors - console.error(err) - } - } - }) + this.uppy.addFiles(files) } retryAllUploads() { diff --git a/packages/web-runtime/tests/unit/components/UploadInfo.spec.js b/packages/web-runtime/tests/unit/components/UploadInfo.spec.js index 4d428db6179..216577ad3b1 100644 --- a/packages/web-runtime/tests/unit/components/UploadInfo.spec.js +++ b/packages/web-runtime/tests/unit/components/UploadInfo.spec.js @@ -67,6 +67,16 @@ describe('UploadInfo component', () => { const uploadTitle = wrapper.find('.upload-info-title p').text() expect(uploadTitle).toBe('Upload cancelled') }) + it('should show that an upload is preparing', () => { + const wrapper = getShallowWrapper({ + showInfo: true, + filesInProgressCount: 0, + runningUploads: 1, + inPreparation: true + }) + const uploadTitle = wrapper.find('.upload-info-title p').text() + expect(uploadTitle).toBe('Preparing upload...') + }) }) describe('progress bar', () => { it('should show the progress bar when an upload is in progress', () => { @@ -176,7 +186,8 @@ function getShallowWrapper({ runningUploads = 0, successful = [], errors = [], - uploadsCancelled = false + uploadsCancelled = false, + inPreparation = false } = {}) { return shallowMount(UploadInfo, { localVue, @@ -190,7 +201,8 @@ function getShallowWrapper({ runningUploads, successful, errors, - uploadsCancelled + uploadsCancelled, + inPreparation } }, mocks: {