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.

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

Remote file paths #4537

Merged
merged 8 commits into from Jul 13, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@uppy/google-drive/src/GoogleDrive.jsx
Expand Up @@ -71,7 +71,7 @@ export default class GoogleDrive extends UIPlugin {
onFirstRender () {
return Promise.all([
this.provider.fetchPreAuthToken(),
this.view.getFolder('root', '/'),
this.view.getFolder('root'),
])
}

Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/provider-views/README.md
Expand Up @@ -26,7 +26,7 @@ class GoogleDrive extends UIPlugin {
onFirstRender () {
return Promise.all([
this.provider.fetchPreAuthToken(),
this.view.getFolder('root', '/'),
this.view.getFolder('root'),
])
}

Expand Down
10 changes: 5 additions & 5 deletions packages/@uppy/provider-views/src/Breadcrumbs.jsx
Expand Up @@ -18,18 +18,18 @@ const Breadcrumb = (props) => {
}

export default (props) => {
const { getFolder, title, breadcrumbsIcon, directories } = props
const { getFolder, title, breadcrumbsIcon, directoryStack } = props
mifi marked this conversation as resolved.
Show resolved Hide resolved

return (
<div className="uppy-Provider-breadcrumbs">
<div className="uppy-Provider-breadcrumbsIcon">{breadcrumbsIcon}</div>
{
directories.map((directory, i) => (
directoryStack.map((directory, i) => (
<Breadcrumb
key={directory.id}
getFolder={() => getFolder(directory.id)}
title={i === 0 ? title : directory.title}
isLast={i + 1 === directories.length}
getFolder={() => getFolder(directory.requestPath)}
title={i === 0 ? title : directory.name}
isLast={i + 1 === directoryStack.length}
/>
))
}
Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/provider-views/src/ProviderView/Header.jsx
Expand Up @@ -6,7 +6,7 @@ export default (props) => {
if (props.showBreadcrumbs) {
components.push(Breadcrumbs({
getFolder: props.getFolder,
directories: props.directories,
directoryStack: props.directoryStack,
breadcrumbsIcon: props.pluginIcon && props.pluginIcon(),
title: props.title,
}))
Expand Down
128 changes: 89 additions & 39 deletions packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx
Expand Up @@ -32,6 +32,15 @@ function isOriginAllowed (origin, allowedOrigin) {
.some((pattern) => pattern?.test(origin) || pattern?.test(`${origin}/`)) // allowing for trailing '/'
}

function formatDirectoryStack (directoryStack) {
return directoryStack.slice(1).map((directory) => directory.name).join('/')
}

function addPath (path, component) {
mifi marked this conversation as resolved.
Show resolved Hide resolved
if (!path) return component
return `${path}/${component}`
}

/**
* Class to easily generate generic views for Provider plugins
*/
Expand Down Expand Up @@ -73,7 +82,7 @@ export default class ProviderView extends View {
authenticated: false,
files: [],
folders: [],
directories: [],
directoryStack: [],
filterInput: '',
isSearchVisible: false,
currentSelection: [],
Expand All @@ -85,9 +94,30 @@ export default class ProviderView extends View {
// Nothing.
}

#updateFilesAndFolders (res, files, folders) {
this.nextPagePath = res.nextPagePath
res.items.forEach((item) => {
async #list (requestPath, absDirPath) {
const { username, nextPagePath, items } = await this.provider.list(requestPath)
this.username = username || this.username

return {
items: items.map((item) => ({
...item,
absDirPath,
})),
nextPagePath,
}
}

async #updateFilesAndFolders (requestPath, directoryStack, filesIn, foldersIn) {
const absDirPath = formatDirectoryStack(directoryStack)

const { items, nextPagePath } = await this.#list(requestPath, absDirPath)

this.nextPagePath = nextPagePath

const files = filesIn ? [...filesIn] : []
const folders = foldersIn ? [...foldersIn] : []

items.forEach((item) => {
if (item.isFolder) {
folders.push(item)
} else {
Expand All @@ -99,32 +129,34 @@ export default class ProviderView extends View {
}

/**
* Based on folder ID, fetch a new folder and update it to state
* Select a folder based on its id: fetches the folder and then updates state with its contents
* TODO rename to something better like selectFolder or navigateToFolder (breaking change?)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this todo? Name is not that bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getX sounds to me like pure function (no side effects) that simply returns something. but this function does a whole lot more than that: it changes the breadcrumbs, it lists folders/files and then updates the state

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, almost everything in Uppy is state and almost all functions perform things on state. But yes since it's a public function I don't think we can change it now.

*
* @param {string} id Folder id
* @param {string} requestPath
* the path we need to use when sending list request to companion (for some providers it's different from ID)
* @param {string} name used in the UI and to build the absDirPath
* @returns {Promise} Folders/files in folder
*/
async getFolder (id, name) {
async getFolder (requestPath, name) {
this.setLoading(true)
try {
const res = await this.provider.list(id)
const folders = []
const files = []
let updatedDirectories
this.lastCheckbox = undefined

const state = this.plugin.getPluginState()
const index = state.directories.findIndex((dir) => id === dir.id)
let { directoryStack } = this.plugin.getPluginState()

const index = directoryStack.findIndex((dir) => requestPath === dir.requestPath)

if (index !== -1) {
updatedDirectories = state.directories.slice(0, index + 1)
// means we navigated back to a known directory (already in the stack), so cut the stack off there
directoryStack = directoryStack.slice(0, index + 1)
} else {
updatedDirectories = state.directories.concat([{ id, title: name }])
// we have navigated into a new (unknown) folder, add it to the stack
directoryStack = [...directoryStack, { requestPath, name }]
}

this.username = res.username || this.username
this.#updateFilesAndFolders(res, files, folders)
this.plugin.setPluginState({ directories: updatedDirectories, filterInput: '' })
this.lastCheckbox = undefined
this.plugin.setPluginState({ directoryStack, filterInput: '' })

await this.#updateFilesAndFolders(requestPath, directoryStack)
} catch (err) {
this.handleError(err)
} finally {
Expand Down Expand Up @@ -161,7 +193,7 @@ export default class ProviderView extends View {
authenticated: false,
files: [],
folders: [],
directories: [],
directoryStack: [],
filterInput: '',
}
this.plugin.setPluginState(newState)
Expand Down Expand Up @@ -220,16 +252,14 @@ export default class ProviderView extends View {
}

async handleScroll (event) {
const path = this.nextPagePath || null
const requestPath = this.nextPagePath || null

if (this.shouldHandleScroll(event) && path) {
if (this.shouldHandleScroll(event) && requestPath) {
this.isHandlingScroll = true

try {
const response = await this.provider.list(path)
const { files, folders } = this.plugin.getPluginState()

this.#updateFilesAndFolders(response, files, folders)
const { directoryStack, files, folders } = this.plugin.getPluginState()
await this.#updateFilesAndFolders(requestPath, directoryStack, files, folders)
} catch (error) {
this.handleError(error)
} finally {
Expand All @@ -238,11 +268,11 @@ export default class ProviderView extends View {
}
}

async recursivelyListAllFiles (path, queue, onFiles) {
let curPath = path
async #recursivelyListAllFiles ({ requestPath, absDirPath, relDirPath, queue, onFiles }) {
let curPath = requestPath

while (curPath) {
const res = await this.provider.list(curPath)
const res = await this.#list(curPath, absDirPath)
curPath = res.nextPagePath

const files = res.items.filter((item) => !item.isFolder)
Expand All @@ -252,7 +282,13 @@ export default class ProviderView extends View {

// recursively queue call to self for each folder
const promises = folders.map(async (folder) => queue.add(async () => (
this.recursivelyListAllFiles(folder.requestPath, queue, onFiles)
this.#recursivelyListAllFiles({
requestPath: folder.requestPath,
absDirPath: addPath(absDirPath, folder.name),
relDirPath: addPath(relDirPath, folder.name),
queue,
onFiles,
})
)))
await Promise.all(promises) // in case we get an error
}
Expand All @@ -266,9 +302,17 @@ export default class ProviderView extends View {
const messages = []
const newFiles = []

for (const file of currentSelection) {
if (file.isFolder) {
const { requestPath, name } = file
for (const selectedItem of currentSelection) {
const { requestPath } = selectedItem

const withRelDirPath = (newItem) => ({
...newItem,
// calculate the file's path relative to the user's selected item's path
// see https://github.com/transloadit/uppy/pull/4537#issuecomment-1614236655
relDirPath: newItem.absDirPath.replace(selectedItem.absDirPath, '').replace(/^\//, ''),
Copy link
Member

Choose a reason for hiding this comment

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

since you added absDirPath yourself, can't we handle this in addPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how that would work

})

if (selectedItem.isFolder) {
let isEmpty = true
let numNewFiles = 0

Expand All @@ -283,36 +327,42 @@ export default class ProviderView extends View {
// the folder was already added. This checks if all files are duplicate,
// if that's the case, we don't add the files.
if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) {
newFiles.push(newFile)
newFiles.push(withRelDirPath(newFile))
numNewFiles++
this.setLoading(this.plugin.uppy.i18n('addedNumFiles', { numFiles: numNewFiles }))
}
isEmpty = false
}
}

await this.recursivelyListAllFiles(requestPath, queue, onFiles)
await this.#recursivelyListAllFiles({
requestPath,
absDirPath: addPath(selectedItem.absDirPath, selectedItem.name),
relDirPath: selectedItem.name,
queue,
onFiles,
})
await queue.onIdle()

let message
if (isEmpty) {
message = this.plugin.uppy.i18n('emptyFolderAdded')
} else if (numNewFiles === 0) {
message = this.plugin.uppy.i18n('folderAlreadyAdded', {
folder: name,
folder: selectedItem.name,
})
} else {
// TODO we don't really know at this point whether any files were actually added
// (only later after addFiles has been called) so we should probably rewrite this.
// Example: If all files fail to add due to restriction error, it will still say "Added 100 files from folder"
message = this.plugin.uppy.i18n('folderAdded', {
smart_count: numNewFiles, folder: name,
smart_count: numNewFiles, folder: selectedItem.name,
})
}

messages.push(message)
} else {
newFiles.push(file)
newFiles.push(withRelDirPath(selectedItem))
}
}

Expand Down Expand Up @@ -350,7 +400,7 @@ export default class ProviderView extends View {
const headerProps = {
showBreadcrumbs: targetViewOptions.showBreadcrumbs,
getFolder: this.getFolder,
directories: this.plugin.getPluginState().directories,
directoryStack: this.plugin.getPluginState().directoryStack,
pluginIcon: this.plugin.icon,
title: this.plugin.title,
logout: this.logout,
Expand Down
Expand Up @@ -46,7 +46,7 @@ export default class SearchProviderView extends View {
isInputMode: true,
files: [],
folders: [],
directories: [],
directoryStack: [],
filterInput: '',
currentSelection: [],
searchTerm: null,
Expand Down
5 changes: 5 additions & 0 deletions packages/@uppy/provider-views/src/View.js
Expand Up @@ -92,6 +92,11 @@ export default class View {
if (file.author.url) tagFile.meta.authorUrl = file.author.url
}

// add relativePath similar to non-remote files: https://github.com/transloadit/uppy/pull/4486#issuecomment-1579203717
if (file.relDirPath != null) tagFile.meta.relativePath = file.relDirPath ? `${file.relDirPath}/${tagFile.name}` : tagFile.name
Copy link
Member

Choose a reason for hiding this comment

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

we currently change paths in three places: in addPath, mutation in recursivelyListAllFiles, and here. Would it be possible to have it centralised?

Copy link
Contributor

Choose a reason for hiding this comment

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

should be possible to set the paths in recursivelyListAllFiles - c.f. owncloud@9f111c5

I'm not handling absolute paths, but in @mifi 's implementation the absPath is available in recursivelyListAllFiles too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to set paths in multiple places, because not all files are loaded by recursivelyListAllFiles

// and absolutePath (with leading slash) https://github.com/transloadit/uppy/pull/4537#issuecomment-1614236655
if (file.absDirPath != null) tagFile.meta.absolutePath = file.absDirPath ? `/${file.absDirPath}/${tagFile.name}` : `/${tagFile.name}`

return tagFile
}

Expand Down
Expand Up @@ -27,17 +27,19 @@ function getAsFileSystemHandleFromEntry (entry, logDropError) {
}

async function* createPromiseToAddFileOrParseDirectory (entry, relativePath, lastResortFile = undefined) {
const relativePathWithName = relativePath ? `${relativePath}/${entry.name}` : null

// For each dropped item, - make sure it's a file/directory, and start deepening in!
if (entry.kind === 'file') {
const file = await entry.getFile()
if (file != null) {
file.relativePath = relativePath ? `${relativePath}/${entry.name}` : null
file.relativePath = relativePathWithName
yield file
} else if (lastResortFile != null) yield lastResortFile
} else if (entry.kind === 'directory') {
for await (const handle of entry.values()) {
// Recurse on the directory, appending the dir name to the relative path
yield* createPromiseToAddFileOrParseDirectory(handle, `${relativePath}/${entry.name}`)
yield* createPromiseToAddFileOrParseDirectory(handle, relativePathWithName ?? entry.name)
}
} else if (lastResortFile != null) yield lastResortFile
}
Expand Down