feat: Resend the last 10 messages to new broadcast member (#7678)#7854
feat: Resend the last 10 messages to new broadcast member (#7678)#7854
Conversation
Messages are sent and encrypted only to the new member. This way we at least postpone spreading the information that the new member joined: even if the server operator is a broadcast member, they can't know that immediately.
iequidoo
force-pushed
the
iequidoo/resend-last-broadcast-msgs
branch
from
324b160 to
d7b3549
Compare
| pub info_only: bool, | ||
| /// If `Some(i)`, return only info (`i` is true) or non-info messages. However, non-info system | ||
| /// messages are only returned for `None`. | ||
| pub info: Option<bool>, |
There was a problem hiding this comment.
Option<bool> always introduces quite some cognitive complexity, i.e. is non-trivial to understand, even if it makes the code shorter.
Instead, this should be an enum:
enum MessageFilter {
/// Return all messages
All,
/// Return only info messages
Info,
/// Return only non-info messages
NonInfo,
}| /// Actual Message ID, if set. This is to avoid updating the `msgs` row, in which case `id` is | ||
| /// fake (`u32::MAX`); | ||
| pub(crate) row_id: MsgId, |
There was a problem hiding this comment.
Why do we need to avoid updating the msgs row when resending? And, can we just add a parameter to create_send_msg_jobs() for this, rather than a new field on an often-used struct?
| /// Chat message list request options. | ||
| #[derive(Debug)] | ||
| #[derive(Debug, Default)] | ||
| pub struct MessageListOptions { |
There was a problem hiding this comment.
It might be worth it to use a specific SQL query that gets the last 10 non-info messages in a chat, rather than using get_chat_msgs_ex(). But I didn't try out how this would actually look in the code - it would probably be more changed lines and a small bit of code repetition, but the added complexity would be more self-contained. So, just an idea, idk if it would be worth it.
| pub(crate) async fn resend_msgs_ex( | ||
| context: &Context, | ||
| msg_ids: &[MsgId], | ||
| to_fpr: Option<Fingerprint>, |
There was a problem hiding this comment.
Usually, we don't abbreviate fingerprint in variables -> should be renamed for consistency
to_fingerprint: Option<Fingerprint>,| // note(treefit): only matters if it is the last chat message (but probably too expensive to | ||
| // check, debounce also solves it) |
There was a problem hiding this comment.
Since we're touching this line anyway:
| // note(treefit): only matters if it is the last chat message (but probably too expensive to | |
| // check, debounce also solves it) | |
| // The event only matters if the message is last in the chat. | |
| // But it's probably too expensive check, and UIs anyways need to debounce |
| ) { | ||
| Some(Err(format_err!("Missing removed/added member"))) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
I do think we should check whether this is a SystemMessage::MemberRemovedFromGroup | SystemMessage::MemberAddedToGroup, and only then return Param::Arg4. Otherwise, this code may silently become incorrect if someone adds another meaning to Arg4 (not that the multiple meanings of Arg* params are great, but we can solve that at another time)
2 participants
Messages are sent and encrypted only to the new member. This way we at least postpone spreading the information that the new member joined: even if the server operator is a broadcast member, they can't know that immediately.
Close #7678