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

api: Add Viewtype::Vcard and parse_vcard() #5536

Merged
merged 5 commits into from May 16, 2024
Merged

api: Add Viewtype::Vcard and parse_vcard() #5536

merged 5 commits into from May 16, 2024

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented May 6, 2024

Part of #5422
Closes #5204

src/summary.rs Show resolved Hide resolved
src/summary.rs Outdated Show resolved Hide resolved
src/mimeparser.rs Outdated Show resolved Hide resolved
@Hocuri Hocuri force-pushed the iequidoo/vcard branch 3 times, most recently from 62b17f8 to 2454eaa Compare May 8, 2024 13:43
@Hocuri Hocuri marked this pull request as ready for review May 8, 2024 13:59
@Hocuri Hocuri requested a review from link2xt May 8, 2024 14:03
@Hocuri Hocuri changed the title feat: Add Viewtype::Vcard (#5202) api: Add Viewtype::Vcard and parse_vcard() May 8, 2024
Copy link
Collaborator Author

@iequidoo iequidoo left a comment

Choose a reason for hiding this comment

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

Approved!

deltachat-jsonrpc/Cargo.toml Outdated Show resolved Hide resolved
@Hocuri Hocuri force-pushed the iequidoo/vcard branch 2 times, most recently from adc6e54 to 6ac9757 Compare May 9, 2024 15:41
src/mimeparser.rs Outdated Show resolved Hide resolved
deltachat-ffi/deltachat.h Outdated Show resolved Hide resolved
@iequidoo
Copy link
Collaborator Author

So let it be called make_vcard(), it's shorter than create_vcard() and also "create" usually means "create smth new/empty"

Copy link
Member

@adbenitez adbenitez left a 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?

@adbenitez
Copy link
Member

So let it be called make_vcard(), it's shorter than create_vcard() and also "create" usually means "create smth new/empty"

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

@adbenitez
Copy link
Member

I think we can merge this? or is something missing?

(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)

@iequidoo
Copy link
Collaborator Author

I think we can merge this? or is something missing?

(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)

  • Removed the commit adding make_vcard(), going to create a separate PR.
  • Rebased on top of main.
  • Squashed commits, please check i did it properly.

Now it can be merged. I think it's better to merge what is already working, at least to avoid further conflicts

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.

About the commit messages:

  • Probably (not completely sure) the prefix should be api:, instead of feat: 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 or api(jsonrpc): Add parse_vcard() or so

src/mimeparser.rs Outdated Show resolved Hide resolved
Comment on lines 47 to 49
/// 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,
Copy link
Collaborator

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 authnames 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.

Copy link
Collaborator Author

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

iequidoo and others added 2 commits May 15, 2024 20:38
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>
Hocuri and others added 3 commits May 15, 2024 20:38
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.
@Christianpaciel
Copy link

Dale fotto. Seguro. Mo. Es tuyo

@Christianpaciel
Copy link

Forro segui. Con tus chat Forro

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.

Add vCard viewtype
4 participants