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: Add import_vcard() (#5202) #5582

Merged
merged 5 commits into from
May 21, 2024
Merged

feat: Add import_vcard() (#5202) #5582

merged 5 commits into from
May 21, 2024

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented May 17, 2024

Add a function importing contacts from the given vCard.

Close #5202

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.

Looks very good already! Lots of small comments, most of them pretty unimportant, feel free to ignore those.

src/contact.rs Outdated Show resolved Hide resolved
src/contact.rs Outdated Show resolved Hide resolved
src/contact.rs Outdated Show resolved Hide resolved
src/contact.rs Outdated Show resolved Hide resolved
src/contact.rs Outdated Show resolved Hide resolved
src/contact.rs Outdated Show resolved Hide resolved
src/contact.rs Outdated Show resolved Hide resolved
src/contact.rs Outdated Show resolved Hide resolved
src/contact.rs Outdated Show resolved Hide resolved
@iequidoo iequidoo force-pushed the iequidoo/vcard branch 4 times, most recently from a948302 to b9cefff Compare May 17, 2024 22:01
@iequidoo iequidoo marked this pull request as ready for review May 17, 2024 22:07
@iequidoo iequidoo requested a review from Hocuri May 17, 2024 22:07
@iequidoo iequidoo requested a review from adbenitez May 17, 2024 23:46
@iequidoo iequidoo force-pushed the iequidoo/vcard branch 3 times, most recently from 853d29d to a08e643 Compare May 18, 2024 23:14
@adbenitez adbenitez requested a review from link2xt May 19, 2024 16:07
src/contact.rs Outdated Show resolved Hide resolved
src/contact.rs Outdated Show resolved Hide resolved
@iequidoo iequidoo force-pushed the iequidoo/vcard branch 2 times, most recently from b855038 to 9924e0b Compare May 19, 2024 23:36
@iequidoo iequidoo requested a review from link2xt May 20, 2024 02:21
src/contact.rs Outdated Show resolved Hide resolved
async fn import_vcard(&self, account_id: u32, path: String) -> Result<Vec<u32>> {
let ctx = self.get_context(account_id).await?;
let mut f = fs::File::open(&path).await?;
let mtime = f
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is better to use current time as a timestamp instead of mtime and not pass the timestamp around as arguments. mtime is always tricky, e.g. if you receive a vCard and then transfer a backup, second device may have have newer timestamp for the same vCard than the first device. Also regarding the time of contact modification, I think it makes more sense to store the date contact was modified if we merge with the changes that come from the network.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For me using the current time is even worse as it changes every time the function is called, while file mtime usually doesn't. But this timestamp anyway was used as a fallback if we don't have timestamps in the vCard. DC doesn't create such vCards anyway. So i decided to use 0 (i.e. UNIX epoch) for vCards w/o timestamps. This means that contacts are created from such vCards, but if a contact already exists and has a gossip key, it is not updated

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only exception if one such vCards is imported after another, then the gossip key is updated because the timestamp is 0 for both

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also regarding the time of contact modification, I think it makes more sense to store the date contact was modified if we merge with the changes that come from the network.

Agree, but currently we don't have a place to store the contact modification time. last_seen isn't suited for this. Probably need to add another one contacts table column

src/contact.rs Outdated Show resolved Hide resolved
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.

Ta-daaaaaa!!

@link2xt link2xt merged commit ff60605 into main May 21, 2024
37 checks passed
@link2xt link2xt deleted the iequidoo/vcard branch May 21, 2024 17:40
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.

vCard-based address book import API
3 participants