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

Add ThreadMetadata store #189

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

nanu-c
Copy link
Contributor

@nanu-c nanu-c commented Sep 20, 2023

This adds a ThreadMetadata store and adds automatic contact creation for messages from unknown senders

Copy link
Collaborator

@gferon gferon left a comment

Choose a reason for hiding this comment

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

Nice addition! I have a bunch of comments. Would you mind if I play around with this PR and potentially integrate the metadata inside of something that would be called Thread (we would rename the current one into ThreadKey) and contains a MessagesIter as well?

@@ -953,6 +959,65 @@ impl<C: Store> Manager<C, Registered> {
log::trace!("{group:?}");
}
}
let uuid = content.metadata.sender.uuid.clone();
match state.config_store.contact_by_id(uuid) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like the feature in itself is a good addition, but I would maybe do this with a function like upsert_contact similar to upsert_group.

archived: false,
avatar: None,
profile_key: profile_key.to_vec(),
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess most of the fields here are default values to you could try to only set uuid and add at the end

..Default::default()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the trait std::default::Default is not implemented for libsignal_service::models::Contact.


let store = &mut state.config_store;

match store.thread_metadata(&thread) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

weird that you can't do state.store.thread_metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right i can use the state.config_store.thread_metadata.

let store = &mut state.config_store;

match store.thread_metadata(&thread) {
Ok(metadata) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Ok(metadata) => {
Ok(None) => {

@nanu-c
Copy link
Contributor Author

nanu-c commented Sep 24, 2023

For me it's perfectly fine if you take this as a base and rename it to Thread. Therefore should i solve the comments?

@gferon
Copy link
Collaborator

gferon commented Sep 25, 2023

I'll take care of the comments, you don't have to worry about it. Thanks for the initial push!

@gferon
Copy link
Collaborator

gferon commented Oct 1, 2023

Just wanted to drop a comment to say I've not forgotten this PR and started to work on it. It involves some clean up of the profiles and contacts structs to do things properly. This also means we'll be able to add a bit more data inside the store, such as avatars for groups and contacts 🥳

@nanu-c
Copy link
Contributor Author

nanu-c commented Oct 3, 2023

Is there something i can do to help you?

@gferon
Copy link
Collaborator

gferon commented Oct 14, 2023

Is there something i can do to help you?

I'll pick it back up next week :-) I was attending Eurorust with @rubdos in the past few days

@nanu-c
Copy link
Contributor Author

nanu-c commented Oct 14, 2023

I'll pick it back up next week :-) I was attending Eurorust with @rubdos in the past few days

That sounds nice :) I keep experimenting with this branch to find out if something is missing. And for example I need the expire_timer, too.

@darkdragon-001
Copy link

@gferon any updates from your side?

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.

3 participants