-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
feat(files): add batch support to copy-move #42124
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this on my local dev setup and it works 👌 Only thing I would change is the redundant if statements with using the MOVE_OR_COPY
in this file, but that's more of a nitpick than anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this works on my dev setup, just needs the changes requested 👌
60129d2
to
fbc3e34
Compare
async execBatch(nodes: Node[], view: View, dir: string) { | ||
const action = getActionForNodes(nodes) | ||
const result = await openFilePickerForAction(action, dir, nodes) | ||
const promises = nodes.map(async node => { | ||
try { | ||
await handleCopyMoveNodeTo(node, result.destination, result.action) | ||
return true | ||
} catch (error) { | ||
logger.error(`Failed to ${result.action} node`, { node, error }) | ||
return false | ||
} | ||
}) | ||
|
||
// We need to keep the selection on error! | ||
// So we do not return null, and for batch action | ||
// we let the front handle the error. | ||
return await Promise.all(promises) | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muuuuuuuuuch cleaner!
Now we don't mix picking the folder AND executing the action :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Really nice changes : D
}) | ||
.setMimeTypeFilter([]) | ||
.setMultiSelect(false) | ||
.startAt(dir) | ||
|
||
return new Promise((resolve, reject) => { | ||
filePicker.setButtonFactory((nodes: Node[], path: string) => { | ||
filePicker.setButtonFactory((_selection, path: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why removing the typing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because unused :)
/compile / |
/backport to stable28 |
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
03268f6
to
5d74f1a
Compare
/backport to stable28 |
Backport in #42279 |
Fix #41661
Later
There are room for improvements, especially on how we handle promises-chaining 🤔Also, we should either refresh the view, or, if the destination is within the current displayed dir, refresh its content.