-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this 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
It also was a question for me where it's better to parse a vCard on the sender side (and fall back to
Otoh it can be unexpected that Wrt the code in |
f9a91ee
to
6959fb9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Except for Webxdcs,
I think it's better to special-case in a test than to special-case
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. |
Moved updating the message summary to
Removed. |
|
There was a problem hiding this 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.
216394c
to
a794845
Compare
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. |
Now it works on my Android, so the last commit is really a fix. EDIT: the test is coming. EDIT: Done. |
I've fixed it for drafts, added the corresponding test and verified on my Android |
c059208
to
5c8c3cb
Compare
No description provided.