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
api: Add Viewtype::Vcard and parse_vcard() #5536
Conversation
62b17f8
to
2454eaa
Compare
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.
Approved!
adc6e54
to
6ac9757
Compare
So let it be called |
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 think we can merge this? or is something missing?
I think I prefer create_vcard over make_vcard, but it doesn't matter much, if you want to really think deeply about the name then probably something like get_vcard_for_contact() is a better name than the other two names |
(never mind, I was thinking by the name of this PR it was only about adding vcard view type and displaying but now import/export api is also being added in this same PR) |
Now it can be merged. I think it's better to merge what is already working, at least to avoid further conflicts |
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.
About the commit messages:
- Probably (not completely sure) the prefix should be
api:
, instead offeat:
because these two commits not only add a new feature but even a new api - "feat(jsonrpc): Add a function parsing a vCard file at the given path": This should mention the name of the added function, i.e. something like
api(jsonrpc): Add parse_vcard() that can parse the file attached to a Viewtype::Vcard
orapi(jsonrpc): Add parse_vcard()
or so
deltachat-contact-tools/src/lib.rs
Outdated
/// Name authorized by the contact itself, vcard property `fn`. Can be empty, one should use | ||
/// `display_name()` to obtain the display name. | ||
pub authname: String, |
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.
Just a note, nothing blocking: It's not actually "authorized by the contact itself", because vCards are sent by a third party (even though AFAIK it's true that the authname
in contact.rs
has the same semantics). And the code handling authname
s in contact.rs
is a bit far away, while this crate here is only concerned with parsing vCards.
Since i agree that display_name
is something else in DeltaChat land, I would slightly be in favor of calling this field simply name
, but feel free to ignore this comment.
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'd avoid using name
/display_name
to protect from occasional sharing of locally given names. With authname
the code in the deltachat core looks just straightforward. But public_name
/shared_name
would be also ok. Anyway i agree that the comment should be fixed
9e6728e
to
eec0c0f
Compare
Add a function parsing a vCard file at the given path. Co-authored-by: Hocuri <hocuri@gmx.de> Co-authored-by: Asiel Díaz Benítez <asieldbenitez@gmail.com>
Co-authored-by: Hocuri <hocuri@gmx.de>
It's actually `deltachat::contact::Contact::authname` by semantics. `display_name`/`name` shouldn't be shared, it's the name given by the user locally.
- u64 only adds unnecessary conversions. - `Contact::last_seen` is also `i64`, so make timestamps such everywhere.
Dale fotto. Seguro. Mo. Es tuyo |
Forro segui. Con tus chat Forro |
Part of #5422
Closes #5204