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

feat: Make dc_msg_get_filename() return the original attachment filename (#4309) #4555

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

iequidoo
Copy link
Collaborator

No description provided.

@iequidoo iequidoo force-pushed the iequidoo/blob-random-filename branch from bee8703 to 20bb6d7 Compare July 18, 2023 23:03
@iequidoo iequidoo marked this pull request as ready for review July 18, 2023 23:30
@iequidoo iequidoo requested a review from link2xt July 18, 2023 23:31
@link2xt
Copy link
Collaborator

link2xt commented Jul 19, 2023

This does not look good for usablity, because these filenames are visible in the UI when you save the file.

See discussion at related issue #3817.

@iequidoo iequidoo force-pushed the iequidoo/blob-random-filename branch from 20bb6d7 to c211f3a Compare July 25, 2023 03:14
@iequidoo iequidoo changed the title feat: Use random filename suffixes for blobstorage (#4309) api: Add dc_msg_save_file() which saves file copy at the provided path (#4309) Jul 25, 2023
Copy link
Collaborator

@link2xt link2xt left a comment

Choose a reason for hiding this comment

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

This branch now mixes new API and adding random filenames which makes things worse for Desktop if we merge this as is without modifying the UI code.

@iequidoo Could you create a separate branch/PR on top of stable that does only this:

  1. Save original attachment filename to the param.
  2. Display this original filename in the message info.
  3. Change Message.get_filename() to return the original filename rather than the basename of the file stored in the blobdir. This will be automatically accessible in JSON-RPC via this field so there is no need to change anything on the JSON-RPC side:
  4. Add C API dc_msg_get_filename, Python API and a Python test that checks that when the same file is sent twice, two messages arrive and both of them have the original unmangled attachment filename accessible via this API.

These changes do not require any research and discussion and would be easy to merge into the stable branch, but combined with deltachat/deltachat-desktop#3330 will already improve the situation with a common use case of saving PDFs, .xdcs through the Save As dialog and similar documents which need to be sorted out rather than opened in a viewer.

/**
* Find out full path, file name and extension of the file associated with a
* message.
*
* Typically files are associated with images, videos, audios, documents.
* Plain text messages do not have a file.
*
* @deprecated Deprecated 2023-07-22, use dc_msg_save_file() instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure yet that we can get rid of this API on Android. Android UI offers an "Export attachments" action, which creates an external storage filename (actually an Android.net.Uri) using Java API and then copies the file contents from the path provided by dc_msg_get_file to the new path. Android storage is complicated with many possibilities and compatibility questions: https://developer.android.com/training/data-storage/shared

For now I would say this is not deprecated, this API is still good for providing access to the file contents, but the first line of description can be changed to remove "file name and extension of the file" and say that filename with extension should be queried using a new API dc_msg_get_filename that returns an email attachment filename rather than a filesystem path. So C API needs a dc_msg_get_fliename as well, then on Android it will be a minor change to use this API to get the filename and extension and only use dc_msg_get_file to access the file contents.

cc @r10s, also would be nice if you can look into iOS code and check what kind of API is needed there.

Copy link
Member

@r10s r10s Jul 25, 2023

Choose a reason for hiding this comment

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

to me, the gist of this pr is not totally clear, however, i also did not follow all recent discussions around it.

yip, get_file() is used on iOS as well - and probably also for deltatouch and desktop. i do not see how save_file() can be a good replacement for it, eg. most files are images where you do not even see the filename, it does not make sense to copy them around all the time, it is totally fine to access them directly.

the files returned from get_file should also provide the correct extensions, eg. there are internal viewers etc., i am pretty sure we will break things, if we have extensionless blobs here and try to access them. the filename, however, can be random, and yes, we should not use get_file() function to get correct filename and extension. (just to clarify, i assume, this was what @link2xt meant)

things are only different for "documents", where you want to display the correct filename, so a get_filename() makes sense. if a save_file() is really helpful then, not sure, these things are pretty OS-specific and copying a file from get_file() to "somepath" + get_filename() is easy, if needed that way - in practise, OS will require more complicated export things

Copy link
Collaborator Author

@iequidoo iequidoo Jul 26, 2023

Choose a reason for hiding this comment

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

At least it simplifies the code a bit in deltachat-jsonrpc/src/api/mod.rs:misc_save_sticker(). Removed from this PR, will add later in the master, then we can rediscuss it

deltachat-ffi/deltachat.h Outdated Show resolved Hide resolved
deltachat-ffi/src/lib.rs Outdated Show resolved Hide resolved
deltachat-ffi/src/lib.rs Outdated Show resolved Hide resolved
deltachat-ffi/src/lib.rs Outdated Show resolved Hide resolved
deltachat-ffi/src/lib.rs Outdated Show resolved Hide resolved
deltachat-jsonrpc/src/api/mod.rs Outdated Show resolved Hide resolved
@iequidoo iequidoo force-pushed the iequidoo/blob-random-filename branch from c211f3a to 8eb3f3b Compare July 26, 2023 18:24
@iequidoo iequidoo changed the base branch from master to stable July 26, 2023 18:25
@iequidoo iequidoo changed the title api: Add dc_msg_save_file() which saves file copy at the provided path (#4309) feat: Make dc_msg_get_filename() return the original attachment filename (#4309) Jul 26, 2023
@iequidoo iequidoo force-pushed the iequidoo/blob-random-filename branch from 8eb3f3b to d163779 Compare July 26, 2023 18:42
@iequidoo
Copy link
Collaborator Author

@iequidoo Could you create a separate branch/PR on top of stable that does only this:

1. Save original attachment filename to the param.

2. Display this original filename in the message info.

3. Change `Message.get_filename()` to return the original filename rather than the basename of the file stored in the blobdir. This will be automatically accessible in JSON-RPC via this field so there is no need to change anything on the JSON-RPC side: https://github.com/deltachat/deltachat-core-rust/blob/f930576fd1a2feef981cbaa2e691f52bb66d2582/deltachat-jsonrpc/src/api/types/message.rs#L79

4. Add C API `dc_msg_get_filename`, Python API and a Python test that checks that when the same file is sent twice, two messages arrive and both of them have the original unmangled attachment filename accessible via this API.

These changes do not require any research and discussion and would be easy to merge into the stable branch, but combined with deltachat/deltachat-desktop#3330 will already improve the situation with a common use case of saving PDFs, .xdcs through the Save As dialog and similar documents which need to be sorted out rather than opened in a viewer.

@link2xt, done but instead of adding a Python test i just extended test_long_filenames(). dc_msg_get_filename() maps 1-1 to the corresponding Rust function, so i think having a Rust test is sufficient.

@iequidoo iequidoo requested review from link2xt and r10s July 26, 2023 18:59
deltachat-ffi/deltachat.h Show resolved Hide resolved
@@ -446,7 +446,8 @@ describe('Offline Tests with unconfigured account', function () {
context.setChatProfileImage(chatId, imagePath)
const blobPath = context.getChat(chatId).getProfileImage()
expect(blobPath.startsWith(blobs)).to.be.true
expect(blobPath.endsWith(image)).to.be.true
expect(blobPath.includes('image')).to.be.true
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes and most of the discussion on the PR are unrelated now, this is why I suggested opening a new branch and PR instead, but ok.

src/message.rs Outdated Show resolved Hide resolved
…ame (#4309)

It can be used e.g. as a default in the file saving dialog. Also display the original filename in
the message info. For these purposes add Param::Filename in addition to Param::File and use it as an
attachment filename in sent emails.
@iequidoo iequidoo force-pushed the iequidoo/blob-random-filename branch from d163779 to b146337 Compare July 27, 2023 17:05
@iequidoo iequidoo merged commit 55aaec7 into stable Jul 27, 2023
33 checks passed
@iequidoo iequidoo deleted the iequidoo/blob-random-filename branch July 27, 2023 17:31
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.

3 participants