-
Notifications
You must be signed in to change notification settings - Fork 25
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
base: main
Are you sure you want to change the base?
Conversation
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 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?
presage/src/manager.rs
Outdated
@@ -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) { |
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 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
.
presage/src/manager.rs
Outdated
archived: false, | ||
avatar: None, | ||
profile_key: profile_key.to_vec(), | ||
}; |
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 guess most of the fields here are default values to you could try to only set uuid
and add at the end
..Default::default()
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.
the trait std::default::Default
is not implemented for libsignal_service::models::Contact
.
presage/src/manager.rs
Outdated
|
||
let store = &mut state.config_store; | ||
|
||
match store.thread_metadata(&thread) { |
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.
weird that you can't do state.store.thread_metadata
?
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.
You are right i can use the state.config_store.thread_metadata
.
presage/src/manager.rs
Outdated
let store = &mut state.config_store; | ||
|
||
match store.thread_metadata(&thread) { | ||
Ok(metadata) => { |
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.
Ok(metadata) => { | |
Ok(None) => { |
For me it's perfectly fine if you take this as a base and rename it to Thread. Therefore should i solve the comments? |
I'll take care of the comments, you don't have to worry about it. Thanks for the initial push! |
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 🥳 |
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 |
That sounds nice :) I keep experimenting with this branch to find out if something is missing. And for example I need the |
@gferon any updates from your side? |
This adds a ThreadMetadata store and adds automatic contact creation for messages from unknown senders