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

fix(files): allow triggering a Files reload directly #50354

Closed
wants to merge 1 commit into from
+42 −49
Closed

fix(files): allow triggering a Files reload directly #50354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions apps/files/src/actions/convertAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

import AutoRenewSvg from '@mdi/svg/svg/autorenew.svg?raw'

import { convertFile, convertFiles, getParentFolder } from './convertUtils'
import { convertFile, convertFiles } from './convertUtils'

type ConversionsProvider = {
from: string,
Expand All @@ -33,17 +33,17 @@
return nodes.every(node => from === node.mime)
},

async exec(node: Node, view: View, dir: string) {

Check failure on line 36 in apps/files/src/actions/convertAction.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

'view' is defined but never used

Check failure on line 36 in apps/files/src/actions/convertAction.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

'dir' is defined but never used
// If we're here, we know that the node has a fileid
convertFile(node.fileid as number, to, getParentFolder(view, dir))
convertFile(node.fileid as number, to)

// Silently terminate, we'll handle the UI in the background
return null
},

async execBatch(nodes: Node[], view: View, dir: string) {

Check failure on line 44 in apps/files/src/actions/convertAction.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

'view' is defined but never used

Check failure on line 44 in apps/files/src/actions/convertAction.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

'dir' is defined but never used
const fileIds = nodes.map(node => node.fileid).filter(Boolean) as number[]
convertFiles(fileIds, to, getParentFolder(view, dir))
convertFiles(fileIds, to)

// Silently terminate, we'll handle the UI in the background
return Array(nodes.length).fill(null)
Expand Down
78 changes: 32 additions & 46 deletions apps/files/src/actions/convertUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,45 +3,56 @@
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
import type { AxiosResponse } from '@nextcloud/axios'
import type { Folder, View } from '@nextcloud/files'
import type { OCSResponse } from '@nextcloud/typings/ocs'

import { emit } from '@nextcloud/event-bus'
import { AxiosError } from 'axios'

Check failure on line 8 in apps/files/src/actions/convertUtils.ts

View workflow job for this annotation

GitHub Actions / NPM lint 

"axios" is extraneous
import { generateOcsUrl } from '@nextcloud/router'
import { showError, showLoading, showSuccess } from '@nextcloud/dialogs'
import { t } from '@nextcloud/l10n'
import axios from '@nextcloud/axios'
import PQueue from 'p-queue'

import logger from '../logger'
import { useFilesStore } from '../store/files'
import { getPinia } from '../store'
import { usePathsStore } from '../store/paths'

const queue = new PQueue({ concurrency: 5 })

const requestConversion = function(fileId: number, targetMimeType: string): Promise<AxiosResponse> {
type ConversionResponse = {
path: string
fileId: number
}

interface PromiseRejectedResult<T> {
status: 'rejected'
reason: T
}

type PromiseSettledResult<T, E> = PromiseFulfilledResult<T> | PromiseRejectedResult<E>;
type ConversionSuccess = AxiosResponse<OCSResponse<ConversionResponse>>
type ConversionError = AxiosError<OCSResponse<ConversionResponse>>

const requestConversion = function(fileId: number, targetMimeType: string): Promise<AxiosResponse<OCSResponse<ConversionResponse>>> {
return axios.post(generateOcsUrl('/apps/files/api/v1/convert'), {
fileId,
targetMimeType,
})
}

export const convertFiles = async function(fileIds: number[], targetMimeType: string, parentFolder: Folder | null) {
export const convertFiles = async function(fileIds: number[], targetMimeType: string) {
const conversions = fileIds.map(fileId => queue.add(() => requestConversion(fileId, targetMimeType)))

// Start conversion
const toast = showLoading(t('files', 'Converting files…'))

// Handle results
try {
const results = await Promise.allSettled(conversions)
const failed = results.filter(result => result.status === 'rejected')
const results = await Promise.allSettled(conversions) as PromiseSettledResult<ConversionSuccess, ConversionError>[]
const failed = results.filter(result => result.status === 'rejected') as PromiseRejectedResult<ConversionError>[]
if (failed.length > 0) {
const messages = failed.map(result => result.reason?.response?.data?.ocs?.meta?.message) as string[]
const messages = failed.map(result => result.reason?.response?.data?.ocs?.meta?.message)
logger.error('Failed to convert files', { fileIds, targetMimeType, messages })

// If all failed files have the same error message, show it
if (new Set(messages).size === 1) {
if (new Set(messages).size === 1 && typeof messages[0] === 'string') {
showError(t('files', 'Failed to convert files: {message}', { message: messages[0] }))
return
}
Expand Down Expand Up @@ -75,14 +86,14 @@
showSuccess(t('files', 'Files successfully converted'))

// Trigger a reload of the file list
if (parentFolder) {
emit('files:node:updated', parentFolder)
}
await window.OCP?.Files?.App?.reload?.()

// Find the first file that is within the current folder

// Switch to the new files
const firstSuccess = results[0] as PromiseFulfilledResult<AxiosResponse>
const firstSuccess = results[0] as PromiseFulfilledResult<ConversionSuccess>
const newFileId = firstSuccess.value.data.ocs.data.fileId
window.OCP.Files.Router.goToRoute(null, { ...window.OCP.Files.Router.params, fileid: newFileId }, window.OCP.Files.Router.query)
window.OCP.Files.Router.goToRoute(null, { ...window.OCP.Files.Router.params, fileid: newFileId.toString() }, window.OCP.Files.Router.query)
} catch (error) {
// Should not happen as we use allSettled and handle errors above
showError(t('files', 'Failed to convert files'))
Expand All @@ -93,24 +104,22 @@
}
}

export const convertFile = async function(fileId: number, targetMimeType: string, parentFolder: Folder | null) {
export const convertFile = async function(fileId: number, targetMimeType: string) {
const toast = showLoading(t('files', 'Converting file…'))

try {
const result = await queue.add(() => requestConversion(fileId, targetMimeType)) as AxiosResponse
const result = await queue.add(() => requestConversion(fileId, targetMimeType)) as AxiosResponse<OCSResponse<ConversionResponse>>
showSuccess(t('files', 'File successfully converted'))

// Trigger a reload of the file list
if (parentFolder) {
emit('files:node:updated', parentFolder)
}
await window.OCP?.Files?.App?.reload?.()
skjnldsv marked this conversation as resolved.
Show resolved Hide resolved

// Switch to the new file
const newFileId = result.data.ocs.data.fileId
window.OCP.Files.Router.goToRoute(null, { ...window.OCP.Files.Router.params, fileid: newFileId }, window.OCP.Files.Router.query)
window.OCP.Files.Router.goToRoute(null, { ...window.OCP.Files.Router.params, fileid: newFileId.toString() }, window.OCP.Files.Router.query)
} catch (error) {
// If the server returned an error message, show it
if (error.response?.data?.ocs?.meta?.message) {
if (error instanceof AxiosError && error?.response?.data?.ocs?.meta?.message) {
showError(t('files', 'Failed to convert file: {message}', { message: error.response.data.ocs.meta.message }))
return
}
Expand All @@ -122,26 +131,3 @@
toast.hideToast()
}
}

/**
* Get the parent folder of a path
*
* TODO: replace by the parent node straight away when we
* update the Files actions api accordingly.
*
* @param view The current view
* @param path The path to the file
* @returns The parent folder
*/
export const getParentFolder = function(view: View, path: string): Folder | null {
const filesStore = useFilesStore(getPinia())
const pathsStore = usePathsStore(getPinia())

const parentSource = pathsStore.getPath(view.id, path)
if (!parentSource) {
return null
}

const parentFolder = filesStore.getNode(parentSource) as Folder | undefined
return parentFolder ?? null
}
7 changes: 7 additions & 0 deletions apps/files/src/views/FilesList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,13 @@ export default defineComponent({

// reload on settings change
subscribe('files:config:updated', this.fetchContent)

// Init the global OCP.Files.App object
// OCP.Files is already initialized at this point
if (!window?.OCP?.Files?.App) {
window.OCP.Files.App = {}
}
Object.assign(window.OCP.Files.App, { ...window.OCP.Files.App, reload: this.fetchContent })
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not add anything new to window.OCP, this should die IMHO.

What is the general use case? Files got converted? Can we not just emit afiles:node:created?
Because we currently already have events for node deletion / creation / update, so everything in the current directory could be modified and changing the directory will cause a reload anyways.

But if we need a reload mechanism, why not provide a reload method in the file router, could fit there. OCP.Files.Router.reload() or maybe just an event files:reload.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the general use case? Files got converted? Can we not just emit afiles:node:created? Because we currently already have events for node deletion / creation / update, so everything in the current directory could be modified and changing the directory will cause a reload anyways.

That would be the smartest approach. But could be overly complicated.
Thanks for reminding me to not take shortcuts, I'll do the following:

  1. if the current folder matches one of the converted files
  2. then we fetch each files
  3. and we trigger a files:node:created

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly what I needed for feedback @susnux ! ❤️ 🤗

},

unmounted() {
Expand Down
Loading