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

Switch to dc_msg_save_file() for exporting attachments #3091

Closed
iequidoo opened this issue May 24, 2024 · 6 comments
Closed

Switch to dc_msg_save_file() for exporting attachments #3091

iequidoo opened this issue May 24, 2024 · 6 comments
Labels
enhancement actually in development, user visible enhancement

Comments

@iequidoo
Copy link
Contributor

iequidoo commented May 24, 2024

IIUC currently the app implements copying of a file on its own. Also when two files with the same name are sent, the second one gets the name <filename>-1.<ext> and if it's exported then, this mangled name is used. It doesn't look very bad, but i'd suggest to use the original file name, at least when no such file yet exists in the target dir.

@r10s
Copy link
Member

r10s commented Nov 26, 2024

ftr, depending on how and if intents are used instead of "just" saving a file, it might be sufficient to use dc_msg_get_filename()

@Hocuri
Copy link
Collaborator

Hocuri commented Nov 28, 2024

It's not possible on Android to write to the Downloads folder using native code (native code is only allowed to write to our own private storage). But DC Android already correctly uses dc_get_filename() and exports the attachment with this name, so this is fine. The -1 gets added when sending (and is also added to Param::Filename then); fixing this will likely need a new core API, so there is nothing Android can do about this at the moment.

@Hocuri Hocuri closed this as completed Nov 28, 2024
@r10s
Copy link
Member

r10s commented Nov 28, 2024

instead of a new api: we should be able to write the file to send to a directory outside blobdir, and pass that to core. core will copy the files then to blobdir

i think it is even partly that way for some images from camera

videos-in-creation-to-be-recoded are a bit special, but there, the filename is often not meaningful anyways, so i would regard that separately (or maybe give up that whole on-creation state at some point)

@Hocuri
Copy link
Collaborator

Hocuri commented Nov 28, 2024

instead of a new api: we should be able to write the file to send to a directory outside blobdir, and pass that to core. core will copy the files then to blobdir

If the files then pile up in this directory outside blobdir, eventually there will be name collisions, forcing us to rename files again. And we can't delete them immediately since core only copies them when sending. The good news would be that as far as I can think, there will only ever be one file that needs to be kept there, so the Android code could simply delete the existing file as soon as it needs to pass a new file to core, but still, this seems rather complex to me.

or maybe give up that whole on-creation state at some point

+1

@r10s
Copy link
Member

r10s commented Nov 28, 2024

we could create a directory with a random name for every file exported, and delete them after sending (iirc core guarantees that the file is copied in time, needs to be checked). that could be straight forward

@iequidoo
Copy link
Contributor Author

iequidoo commented Nov 29, 2024

If deltachat/deltachat-core-rust#5495 is merged, the app can just create a blobdir subdirectory with a random name and put the file there (with the original name) and then pass it to the core. This doesn't require a new core API and that PR also doesn't change it, it only adds support for blobdir subdirs (storing files directly in the blobdir is still possible) and generates random names for these subdirs when copying blobs from external locations. Apps mayn't even follow the core's subdirs name pattern. Also merging that PR doesn't prevent implementing deltachat/deltachat-core-rust#6265 further (EDIT: Moreover, we can use file hashes for subdir names, maybe that will help in implementation). Though of course a huge apps testing is needed before merging, so it's unlikely that happens soon if at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement actually in development, user visible enhancement
Projects
None yet
Development

No branches or pull requests

3 participants