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: Display vCard contact name in the message summary #5619

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

iequidoo
Copy link
Collaborator

No description provided.

@iequidoo

This comment was marked as resolved.

@iequidoo iequidoo marked this pull request as ready for review May 22, 2024 22:28
@iequidoo iequidoo requested review from Hocuri and r10s May 22, 2024 22:28
@Hocuri Hocuri mentioned this pull request May 25, 2024
3 tasks
Copy link
Collaborator

@Hocuri Hocuri left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned that this makes Viewtype::Vcard very special code-wise because set_file() changes the viewtype to Viewtype::File or sets a param for only this one viewtype, which could produce subtle bugs in the future.

Would it be feasible put the code calling try_set_vcard() into do_set_draft() and prepare_msg_blob() instead of set_file() and set_file_from_bytes() (and keep the code in mimeparser.rs)?

Note to myself, I need to test this on an Android device - for [vcards created with DC, vcards created with Thunderbird, vcards created with Thunderbird that contain too many and should be shown as a file]:

  • send .vcf file (sender & receiver side)
  • draft file
  • forward file
  • share file

src/mimeparser.rs Show resolved Hide resolved
@iequidoo
Copy link
Collaborator Author

I'm a bit concerned that this makes Viewtype::Vcard very special code-wise because set_file() changes the viewtype to Viewtype::File or sets a param for only this one viewtype, which could produce subtle bugs in the future.

Would it be feasible put the code calling try_set_vcard() into do_set_draft() and prepare_msg_blob() instead of set_file() and set_file_from_bytes() (and keep the code in mimeparser.rs)?

It also was a question for me where it's better to parse a vCard on the sender side (and fall back to File if it can't be parsed). Pros of doing this in set_file*() are:

  • set_file_from_bytes() already has data in memory, no need to open the file later.
  • test_get_summary_text() checks that the summary is correct already after set_file_from_bytes(), no need to make vCards a special case.

Otoh it can be unexpected that set_file*() may change the message viewtype. Btw, also if the file content changes after the set_file*() call, the summary will be wrong.

Wrt the code in mimeparser.rs, in fact i just moved the check for vCard validity from get_mime_type() to do_add_single_file_part(), this way we don't need to parse a vCard twice -- the first time to check its validity and the second to set the message summary.

@iequidoo iequidoo force-pushed the iequidoo/vcard branch 2 times, most recently from f9a91ee to 6959fb9 Compare May 25, 2024 18:31
@iequidoo

This comment was marked as resolved.

@Hocuri
Copy link
Collaborator

Hocuri commented May 26, 2024

set_file_from_bytes() already has data in memory, no need to open the file later.

Except for Webxdcs, set_file_from_bytes() is only used in tests where performance doesn't matter as much, though.

test_get_summary_text() checks that the summary is correct already after set_file_from_bytes(), no need to make vCards a special case.

I think it's better to special-case in a test than to special-case set_file*() to do something extra just for a single viewtype.

We even can remove StockMessage::Contact now, it should never be displayed

Decided not to deprecate it yet, it may be useful in the future, but added a comment that it's unused currently

I do think we should remove it, if we add stock strings just in case we may need it in the future, we will end up with a lot of unused stock strings at some point. And in most cases when one thinks "it may be useful in the future", one ends up never needing it.

@iequidoo
Copy link
Collaborator Author

I think it's better to special-case in a test than to special-case set_file*() to do something extra just for a single viewtype.

Moved updating the message summary to prepare_msg_blob() and do_set_draft().

I do think we should remove it, if we add stock strings just in case we may need it in the future, we will end up with a lot of unused stock strings at some point. And in most cases when one thinks "it may be useful in the future", one ends up never needing it.

Removed.

@iequidoo
Copy link
Collaborator Author

DC_STR_CONTACT can't be just removed so i deprecated it.

Copy link
Collaborator

@Hocuri Hocuri left a comment

Choose a reason for hiding this comment

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

Nice! I have 3 small, non-blocking comments. The only thing left to approve is me testing that this works, as mentioned above.

src/message.rs Outdated Show resolved Hide resolved
src/mimeparser.rs Outdated Show resolved Hide resolved
src/summary.rs Outdated Show resolved Hide resolved
@iequidoo iequidoo force-pushed the iequidoo/vcard branch 4 times, most recently from 216394c to a794845 Compare May 28, 2024 00:17
@iequidoo iequidoo changed the title feat: Display vCard contact name in message summary feat: Display vCard contact name in the message summary May 28, 2024
@Hocuri
Copy link
Collaborator

Hocuri commented May 29, 2024

Status of this PR: When testing on a real Android device, I found that this doesn't work on the sending side and the summary is only "👤". @iequidoo and me texted in private about this, but we don't know why it happens (and I still have somewhat limited capacity to debug it).

@iequidoo
Copy link
Collaborator Author

Status of this PR: When testing on a real Android device, I found that this doesn't work on the sending side and the summary is only "👤". @iequidoo and me texted in private about this, but we don't know why it happens (and I still have somewhat limited capacity to debug it).

I tried to debug it on my Android yesterday, but had a problem with the file system and unfortunately lost my build env :) But i have reproduced the problem before that. Now building it again.

@iequidoo
Copy link
Collaborator Author

iequidoo commented May 29, 2024

I tried to debug it on my Android yesterday, but had a problem with the file system and unfortunately lost my build env :) But i have reproduced the problem before that. Now building it again.

Now it works on my Android, so the last commit is really a fix. EDIT: the test is coming. EDIT: Done.

@iequidoo
Copy link
Collaborator Author

I've fixed it for drafts, added the corresponding test and verified on my Android

src/chat.rs Outdated Show resolved Hide resolved
src/chat.rs Show resolved Hide resolved
src/mimeparser.rs Outdated Show resolved Hide resolved
@iequidoo iequidoo merged commit 37831f8 into main Jun 12, 2024
37 checks passed
@iequidoo iequidoo deleted the iequidoo/vcard branch June 12, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants