Skip to content

feat: file stays file mostly#8166

Closed
r10s wants to merge 3 commits intomainfrom
r10s/file-stays-file-mostly
Closed

feat: file stays file mostly#8166
r10s wants to merge 3 commits intomainfrom
r10s/file-stays-file-mostly

Conversation

@r10s
Copy link
Copy Markdown
Contributor

@r10s r10s commented Apr 24, 2026

this PR simplifies heuristics when attaching a file or and image:

  • convert File to Webxdc or Vcard, these types all may come from the file selector
  • convert Image to Audio/Video/Gif, the types all may come from the gallery selector
  • removed conversion from File to Image

in a subsequent PR, we should make sure, that incoming files are not concerted to images as well. we might need some flag for that (to know if an image is sent as image or as file). that way, the UI can render files all the same way, and show their size, name etc., making it also more clear that the image is sent as original file

@r10s r10s requested review from adbenitez and link2xt April 24, 2026 20:29
@r10s r10s force-pushed the r10s/file-stays-file-mostly branch from b4e505d to 949671b Compare April 24, 2026 20:36
@link2xt link2xt force-pushed the r10s/file-stays-file-mostly branch from 949671b to c68cfa3 Compare April 25, 2026 23:21
@link2xt
Copy link
Copy Markdown
Collaborator

link2xt commented Apr 25, 2026

Tweaked one python test in the first commit and rebased.

@r10s
Copy link
Copy Markdown
Contributor Author

r10s commented Apr 26, 2026

thanks a lot for taking care of the python test <3

@iequidoo iequidoo self-requested a review April 26, 2026 15:03
@iequidoo
Copy link
Copy Markdown
Collaborator

The second commit needs a test

@link2xt
Copy link
Copy Markdown
Collaborator

link2xt commented Apr 27, 2026

The second commit needs a test

I tried to add a test as a third commit, and it fails, Image is still converted to vCard when image with .vcf extension is sent. It logs "Cannot check/recode image, using original data: Unknown format.", sets viewtype to File, then File is converted to vCard anyway.

This test passes regardless of the second commit:

/// Tests that image sent with "File" viewtype keeps the viewtype
/// and is not converted into Image viewtype because of .png extension.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_send_image_as_file() -> Result<()> {
    let alice = &TestContext::new_alice().await;

    let bytes = include_bytes!("../../test-data/image/logo.png");
    let path = alice.get_blobdir().join("logo").with_extension("png");
    fs::write(&path, bytes)
        .await
        .context("Failed to write file")?;

    // Set image as file.
    let mut msg = Message::new(Viewtype::File);
    msg.set_file_and_deduplicate(alice, &path, None, None)?;
    let chat = alice.get_self_chat().await;
    let sent = alice.send_msg(chat.id, &mut msg).await;
    let msg = Message::load_from_db(alice, sent.sender_msg_id).await?;

    // Viewtype is still File, not converted to Image.
    assert_eq!(msg.get_viewtype(), Viewtype::File);

    Ok(())
}

This test fails regardless of the second commit:

/// Tests that vCard sent with "image" viewtype keeps the viewtype
/// and is not converted into vCard viewtype.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_send_vcard_as_image() -> Result<()> {
    let alice = &TestContext::new_alice().await;

    let vcard = contact::make_vcard(alice, &[ContactId::SELF]).await?;

    let path = alice.get_blobdir().join("alice").with_extension("vcf");
    fs::write(&path, &vcard)
        .await
        .context("Failed to write file")?;

    // Set vCard as image.
    let mut msg = Message::new(Viewtype::Image);
    msg.set_file_and_deduplicate(alice, &path, None, None)?;
    let chat = alice.get_self_chat().await;
    let sent = alice.send_msg(chat.id, &mut msg).await;
    let msg = Message::load_from_db(alice, sent.sender_msg_id).await?;

    // Viewtype is still Image, not converted to Vcard with heuristics.
    assert_eq!(msg.get_viewtype(), Viewtype::Image);

    Ok(())
}

@link2xt link2xt force-pushed the r10s/file-stays-file-mostly branch from 4694091 to c68cfa3 Compare April 27, 2026 17:50
@link2xt
Copy link
Copy Markdown
Collaborator

link2xt commented Apr 27, 2026

I think we can merge this as is anyway, but it is unclear if the second commit fixes anything.

@r10s
Copy link
Copy Markdown
Contributor Author

r10s commented Apr 27, 2026

hm, then maybe revert the second commit. i am totally fine with that.

i did the second commit to have a more understandable flow, but seem to fail at that :) so it seems better to leave the old logic and iterate over when it becomes and issue

@r10s
Copy link
Copy Markdown
Contributor Author

r10s commented Apr 27, 2026

wait, but we also do not want to convert from File to Video/Gif, so the second commit seems still needed

@link2xt
Copy link
Copy Markdown
Collaborator

link2xt commented Apr 27, 2026

wait, but we also do not want to convert from File to Video/Gif, so the second commit seems still needed

Somehow the first test where I tried to send .png as File already passed even without the second commit.

@link2xt link2xt force-pushed the r10s/file-stays-file-mostly branch from c68cfa3 to 2366326 Compare April 27, 2026 20:50
@link2xt
Copy link
Copy Markdown
Collaborator

link2xt commented Apr 27, 2026

Somehow the first test where I tried to send .png as File already passed even without the second commit.

I have pushed the test checking that .png File keeps viewtype as a separate commit. It works even without the third commit.

@link2xt link2xt force-pushed the r10s/file-stays-file-mostly branch from 2366326 to 3b1e11a Compare April 27, 2026 20:55
@r10s
Copy link
Copy Markdown
Contributor Author

r10s commented Apr 27, 2026

Somehow the first test where I tried to send .png as File already passed even without the second commit.

sending png as file is fixed in the first commit already. but the test test_send_image_as_file should fail on main.

if or if not vcard send as image keeps the image-viewtype, i am not even sure about that. but in practise, this is a non-issue, at least on mobile. still, i am wondering why the viewtype is converted and test_send_vcard_as_image fails. maybe there are other places doing some conversions.

@r10s r10s marked this pull request as draft April 28, 2026 15:39
@r10s
Copy link
Copy Markdown
Contributor Author

r10s commented Apr 28, 2026

i commented to @Hocuri's considerations at #8167 (comment) , let's not rush on merging this PR

@r10s
Copy link
Copy Markdown
Contributor Author

r10s commented Apr 28, 2026

k, this will cause too much noise just now, see linked issue for details. we can reconsider at a later point, but it is good to figure out what not to do :) thanks all for helping on figuring that out

@r10s r10s closed this Apr 28, 2026
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