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

improvements to selecting multiple files to send #4278

Merged

Conversation

Simon-Laux
Copy link
Member

@Simon-Laux Simon-Laux commented Oct 27, 2024

  • Dropping multiple files onto deltachat now sends images as compressed images instead of uncompressed files
  • Added support for selecting multiple files in the attachment file picker.
  • fix compression of images when added with Image option from attachment menu

closes #4219

@Simon-Laux Simon-Laux requested review from nicodh, r10s and WofWca October 27, 2024 12:25
Copy link
Collaborator

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

I did not test the browser version, but the Electron one looks alright, apart from removeTempFile().

I tested it on a surface level, and looked at the code somewhat thoroughly. It appears to only affect the places where multiSelections is used, otherwise the behavior of showOpenFileDialog is the same.

In addition, I would assume we don't want to release it with 1.48?

viewtype: msgViewType,
}).then(() => {
// start sending other files, don't wait until last file is sent
runtime.removeTempFile(filePath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this is copy-pasted from

openDialog(ConfirmSendingFiles, {
sanitizedFileList,
chatName: chat.name,
onClick: async (isConfirmed: boolean) => {
if (!isConfirmed) {
return
}
for (const file of sanitizedFileList) {
const path = await writeTempFileFromFile(file)
const msgViewType: Viewtype = file.type.startsWith('image')
? 'Image'
: 'File'
sendMessage(accountId, chat.id, {
file: path,
viewtype: msgViewType,
}).then(() => {
// start sending other files, don't wait until last file is sent
runtime.removeTempFile(path)
})
}
},
})

but in that code we also do writeTempFileFromFile. Why do we have to do that there, but not here.

And, most importantly, if we decide to not create temp files, we should not runtime.removeTempFile() here. Currently in Electron this simply gives an error of "Path is outside of the temp folder". I did not check what the browser version does, but it does not look good.

I think it'd be nice to add a docstring to removeTempFile, or somehow make it safer in general.

Copy link
Member Author

Choose a reason for hiding this comment

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

but in that code we also do writeTempFileFromFile. Why do we have to do that there, but not here.

because here we get valid file paths.
everything would be cleaner if we could pass files directly to core without these temp files, but that api doesn't exist yet.

For drag and drop I needed to change it, because it was an electron specific api and also did not always work #3595.

but it does not look good.

I know, needs to be improved, I plan to move the used temp folder to a different place, I already added a todo item to #4232 - in that process we can make the path check even safer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know, needs to be improved

I'm interested in a sense that it wouldn't work great if you pass normal, non-temporary files there 😅

@Simon-Laux
Copy link
Member Author

Simon-Laux commented Oct 27, 2024

In addition, I would assume we don't want to release it with 1.48?

works, fixes bugs and brings a usecase back that would otherwise have been removed (send multiple images files uncompressed in one action). So I would release it with 1.48, we will have a few more test releases before it anyway.

I did not test the browser version

I tested it in the browser and on electron.

@Simon-Laux Simon-Laux force-pushed the simon/improvements-to-selecting-multiple-files-to-send branch from ea31ef7 to 4c32b17 Compare October 27, 2024 17:22
…ker.

And fix compression of images when added with Image option from attachment menu.
@Simon-Laux Simon-Laux force-pushed the simon/improvements-to-selecting-multiple-files-to-send branch from 4c32b17 to 9e1b8b7 Compare October 27, 2024 17:23
Copy link
Contributor

@nicodh nicodh left a comment

Choose a reason for hiding this comment

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

I missed before that this includes a bugfix - so if we skip the wrong removeTempFile line I would vote to include it in the release

viewtype: msgViewType,
}).then(() => {
// start sending other files, don't wait until last file is sent
runtime.removeTempFile(filePath)
Copy link
Contributor

Choose a reason for hiding this comment

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

The line
runtime.removeTempFile(filePath)
is definitely wrong since as Simon says, filePath is the original file path

"Why do we have to do that there," ?
The reason is that drag & drop only provides DataTransfer files which have no path and we need to pass a local path as file attachement to core

Copy link
Member Author

@Simon-Laux Simon-Laux Nov 1, 2024

Choose a reason for hiding this comment

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

it is needed for browser, for electron the call just failed because it can not delete non-temp files.
I put a if-clause around it to only run it in browser target.

@Simon-Laux Simon-Laux requested review from nicodh and WofWca November 1, 2024 13:52
@Simon-Laux Simon-Laux merged commit f9362a9 into main Nov 1, 2024
6 of 7 checks passed
@Simon-Laux Simon-Laux deleted the simon/improvements-to-selecting-multiple-files-to-send branch November 1, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dragging images partly sent as files, unexpectedly huge
3 participants