From e4425ec43fa6143c6fe59bebc7883b3e67102e49 Mon Sep 17 00:00:00 2001 From: Andrew Baldwin Date: Thu, 16 May 2024 17:25:23 -0700 Subject: [PATCH] Remove client id from timestamp-based `chathistory`. Other simplifications and clarifications suggested by review. --- data/src/client.rs | 132 +++++++++++++++++++--------------------- data/src/history.rs | 20 ++---- data/src/isupport.rs | 8 +-- data/src/message.rs | 17 +----- irc/proto/src/lib.rs | 8 +-- src/main.rs | 80 ++++++++---------------- src/screen/dashboard.rs | 10 +-- 7 files changed, 108 insertions(+), 167 deletions(-) diff --git a/data/src/client.rs b/data/src/client.rs index 631e2584..0a7c2885 100644 --- a/data/src/client.rs +++ b/data/src/client.rs @@ -312,7 +312,7 @@ impl Client { if matches!( message_reference, - MessageReference::Timestamp(_, _) + MessageReference::Timestamp(_) ) { finished .events @@ -346,68 +346,70 @@ impl Client { return None; } _ if batch_tag.is_some() => { - let events = if let Some(target) = batch_tag - .as_ref() - .and_then(|batch| self.batches.get(batch)) - .and_then(|batch| batch.chathistory_target.clone()) - { - if let Some(ChatHistoryRequest { + let events = if let Some(( + ChatHistoryRequest { subcommand, message_reference, .. - }) = self.chathistory_requests.get(&target) - { - if Some(User::from(Nick::from("HistServ"))) == message.user() { - // HistServ provides event-playback without event-playback - // which would require client-side parsing to map appropriately. - // Avoid that complexity by only providing that functionality - // via event-playback. - vec![] - } else { - match &message.command { - Command::NICK(_) => { - let target = message::Target::Channel { - channel: target, - source: source::Source::Server(None), - }; - - vec![Event::ChatHistoryWithTarget( - message, - self.nickname().to_owned(), - target, - subcommand.clone(), - message_reference.clone(), - )] - } - Command::QUIT(_) => { - let target = message::Target::Channel { - channel: target, - source: source::Source::Server(Some(source::Server::new( - source::server::Kind::Quit, - message - .user() - .map(|user| Nick::from(user.nickname().as_ref())), - ))), - }; - - vec![Event::ChatHistoryWithTarget( - message, - self.nickname().to_owned(), - target, - subcommand.clone(), - message_reference.clone(), - )] - } - _ => vec![Event::ChatHistorySingle( + }, + target, + )) = batch_tag + .as_ref() + .and_then(|batch| self.batches.get(batch)) + .and_then(|batch| batch.chathistory_target.clone()) + .and_then(|target| { + self.chathistory_requests + .get(&target) + .map(|chathistory_request| (chathistory_request, target)) + }) { + if Some(User::from(Nick::from("HistServ"))) == message.user() { + // HistServ provides event-playback without event-playback + // which would require client-side parsing to map appropriately. + // Avoid that complexity by only providing that functionality + // via event-playback. + vec![] + } else { + match &message.command { + Command::NICK(_) => { + let target = message::Target::Channel { + channel: target, + source: source::Source::Server(None), + }; + + vec![Event::ChatHistoryWithTarget( + message, + self.nickname().to_owned(), + target, + subcommand.clone(), + message_reference.clone(), + )] + } + Command::QUIT(_) => { + let target = message::Target::Channel { + channel: target, + source: source::Source::Server(Some(source::Server::new( + source::server::Kind::Quit, + message + .user() + .map(|user| Nick::from(user.nickname().as_ref())), + ))), + }; + + vec![Event::ChatHistoryWithTarget( message, self.nickname().to_owned(), + target, subcommand.clone(), message_reference.clone(), - )], + )] } + _ => vec![Event::ChatHistorySingle( + message, + self.nickname().to_owned(), + subcommand.clone(), + message_reference.clone(), + )], } - } else { - self.handle(message, context)? } } else { self.handle(message, context)? @@ -485,10 +487,10 @@ impl Client { requested.push("batch"); // We require batch for our chathistory support - let requesting_chathistory = if contains_starting_with("chathistory") { + let requesting_chathistory = if contains("chathistory") { requested.push("chathistory"); true - } else if contains_starting_with("draft/chathistory") { + } else if contains("draft/chathistory") { requested.push("draft/chathistory"); true } else { @@ -578,9 +580,6 @@ impl Client { let newly_contains = |s| new_caps.iter().any(|cap| cap == s); - let newly_contains_starting_with = - |s| new_caps.iter().any(|cap| cap.starts_with(s)); - let contains = |s| self.listed_caps.iter().any(|cap| cap == s); if newly_contains("invite-notify") { @@ -604,10 +603,10 @@ impl Client { } // We require batch for our chathistory support - let requesting_chathistory = if newly_contains_starting_with("chathistory") { + let requesting_chathistory = if newly_contains("chathistory") { requested.push("chathistory"); true - } else if newly_contains_starting_with("draft/chathistory") { + } else if newly_contains("draft/chathistory") { requested.push("draft/chathistory"); true } else { @@ -1252,14 +1251,14 @@ impl Client { match subcommand { ChatHistorySubcommand::Latest(_) => { let command_message_reference = match message_reference { - MessageReference::Timestamp(server_time, _) => { + MessageReference::Timestamp(server_time) => { if let Some(fuzzed_server_time) = TimeDelta::try_seconds(isupport::CHATHISTORY_FUZZ_SECONDS) .and_then(|time_delta| { server_time.checked_sub_signed(time_delta) }) { - MessageReference::Timestamp(fuzzed_server_time, ":".to_string()) + MessageReference::Timestamp(fuzzed_server_time) } else { message_reference } @@ -1282,17 +1281,14 @@ impl Client { } ChatHistorySubcommand::Before => { let command_message_reference = match message_reference { - MessageReference::Timestamp(reference_timestamp, _) => { + MessageReference::Timestamp(reference_timestamp) => { if let Some(fuzzed_reference_timestamp) = TimeDelta::try_seconds(isupport::CHATHISTORY_FUZZ_SECONDS) .and_then(|time_delta| { reference_timestamp.checked_add_signed(time_delta) }) { - MessageReference::Timestamp( - fuzzed_reference_timestamp, - ":".to_string(), - ) + MessageReference::Timestamp(fuzzed_reference_timestamp) } else { message_reference } diff --git a/data/src/history.rs b/data/src/history.rs index 2b3a3bf5..886f1835 100644 --- a/data/src/history.rs +++ b/data/src/history.rs @@ -485,21 +485,13 @@ fn is_referenceable_message( ) -> bool { if matches!(message.target.source(), message::Source::Internal(_)) { false + } else if matches!( + message_reference_type, + Some(isupport::MessageReferenceType::MessageId) + ) { + message.id.is_some() } else { - match message_reference_type { - Some(isupport::MessageReferenceType::MessageId) => message - .id - .as_ref() - .is_some_and(|message_id| !message_id.starts_with(':')), - Some(isupport::MessageReferenceType::Timestamp) => message - .id - .as_ref() - .is_some_and(|message_id| message_id.starts_with(':') && message_id != ":"), - None => message - .id - .as_ref() - .is_some_and(|message_id| message_id != ":"), - } + true } } diff --git a/data/src/isupport.rs b/data/src/isupport.rs index 8ab0c937..366dc03e 100644 --- a/data/src/isupport.rs +++ b/data/src/isupport.rs @@ -650,7 +650,7 @@ pub struct CommandTargetLimit { #[derive(Clone, Debug)] pub enum MessageReference { - Timestamp(DateTime, String), + Timestamp(DateTime), MessageId(String), None, } @@ -658,7 +658,7 @@ pub enum MessageReference { impl fmt::Display for MessageReference { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - MessageReference::Timestamp(server_time, _) => write!( + MessageReference::Timestamp(server_time) => write!( f, "timestamp={}", server_time.to_rfc3339_opts(SecondsFormat::Millis, true) @@ -672,9 +672,7 @@ impl fmt::Display for MessageReference { impl PartialEq for MessageReference { fn eq(&self, other: &Message) -> bool { match self { - MessageReference::Timestamp(server_time, client_id) => { - other.server_time == *server_time && other.id.as_deref() == Some(client_id.as_str()) - } + MessageReference::Timestamp(server_time) => other.server_time == *server_time, MessageReference::MessageId(id) => other.id.as_deref() == Some(id.as_str()), MessageReference::None => false, } diff --git a/data/src/message.rs b/data/src/message.rs index 3aaaf332..5224a940 100644 --- a/data/src/message.rs +++ b/data/src/message.rs @@ -1,9 +1,7 @@ use chrono::{DateTime, Utc}; use irc::proto; use irc::proto::Command; -use seahash::SeaHasher; use serde::{Deserialize, Serialize}; -use std::hash::{Hash, Hasher}; pub use self::source::Source; use crate::time::{self, Posix}; @@ -15,7 +13,7 @@ pub type Channel = String; pub(crate) mod broadcast; pub mod source; -#[derive(Debug, Clone, Hash)] +#[derive(Debug, Clone)] pub struct Encoded(proto::Message); impl Encoded { @@ -99,10 +97,9 @@ impl Message { our_nick: Nick, config: &Config, resolve_attributes: impl Fn(&User, &str) -> Option, - generate_missing_id: bool, ) -> Option { let server_time = server_time(&encoded); - let id = message_id(&encoded).or(generate_missing_id.then_some(client_id(&encoded))); + let id = message_id(&encoded); let text = text(&encoded, &our_nick, config, &resolve_attributes)?; let target = target(encoded, &our_nick, &resolve_attributes)?; @@ -322,16 +319,6 @@ fn target( } } -pub fn client_id(message: &Encoded) -> String { - let mut hasher = SeaHasher::new(); - - message.hash(&mut hasher); - - // Prefix hash with ':' fort client id, since that character - // is not allowed in message ids provided by servers - format!(":{:x}", hasher.finish()) -} - pub fn message_id(message: &Encoded) -> Option { message .tags diff --git a/irc/proto/src/lib.rs b/irc/proto/src/lib.rs index 0fea9c98..f3f7abda 100644 --- a/irc/proto/src/lib.rs +++ b/irc/proto/src/lib.rs @@ -4,7 +4,7 @@ pub mod command; pub mod format; pub mod parse; -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Message { pub tags: Vec, pub source: Option, @@ -21,19 +21,19 @@ impl From for Message { } } -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Tag { pub key: String, pub value: Option, } -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, Eq)] pub enum Source { Server(String), User(User), } -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct User { pub nickname: String, pub username: Option, diff --git a/src/main.rs b/src/main.rs index fbe628c7..10516c36 100644 --- a/src/main.rs +++ b/src/main.rs @@ -428,26 +428,20 @@ impl Application for Halloy { .flat_map(|message| { let mut commands = vec![]; - let supports_chathistory = self.clients.get_server_supports_chathistory(&server); - let mut events = self.clients.receive(&server, message); if let Some(data::client::Event::ChatHistoryBatchFilter) = events.first() { if let Some(reference_position) = events.iter().position(|event| match event { data::client::Event::ChatHistorySingle(encoded, _, _, message_reference) | data::client::Event::ChatHistoryWithTarget(encoded, _, _, _, message_reference) => { - if let MessageReference::Timestamp(reference_server_time, reference_client_id) = - message_reference - { + if let MessageReference::Timestamp(reference_server_time) = message_reference { log::trace!( "{:?} message_reference {:?} encoded {:?}", - data::message::server_time(encoded) == *reference_server_time - && data::message::client_id(encoded) == *reference_client_id, - message_reference, - encoded - ); + data::message::server_time(encoded) == *reference_server_time, + message_reference, + encoded, + ); data::message::server_time(encoded) == *reference_server_time - && data::message::client_id(encoded) == *reference_client_id } else { false } @@ -481,24 +475,16 @@ impl Application for Halloy { match event { data::client::Event::Single(encoded, our_nick) => { - if let Some(message) = data::Message::received( - encoded, - our_nick, - &self.config, - resolve_user_attributes, - supports_chathistory, - ) { + if let Some(message) = + data::Message::received(encoded, our_nick, &self.config, resolve_user_attributes) + { dashboard.record_message(&server, message); } } data::client::Event::WithTarget(encoded, our_nick, target) => { - if let Some(message) = data::Message::received( - encoded, - our_nick, - &self.config, - resolve_user_attributes, - supports_chathistory, - ) { + if let Some(message) = + data::Message::received(encoded, our_nick, &self.config, resolve_user_attributes) + { dashboard.record_message(&server, message.with_target(target)); } } @@ -549,13 +535,9 @@ impl Application for Halloy { } }, data::client::Event::Notification(encoded, our_nick, notification) => { - if let Some(message) = data::Message::received( - encoded, - our_nick, - &self.config, - resolve_user_attributes, - supports_chathistory, - ) { + if let Some(message) = + data::Message::received(encoded, our_nick, &self.config, resolve_user_attributes) + { dashboard.record_message(&server, message); } @@ -619,13 +601,9 @@ impl Application for Halloy { subcommand, message_reference, ) => { - if let Some(message) = data::Message::received( - encoded, - our_nick, - &self.config, - resolve_user_attributes, - true, - ) { + if let Some(message) = + data::Message::received(encoded, our_nick, &self.config, resolve_user_attributes) + { dashboard.record_chathistory_message( &server, message, @@ -641,13 +619,9 @@ impl Application for Halloy { subcommand, message_reference, ) => { - if let Some(message) = data::Message::received( - encoded, - our_nick, - &self.config, - resolve_user_attributes, - true, - ) { + if let Some(message) = + data::Message::received(encoded, our_nick, &self.config, resolve_user_attributes) + { dashboard.record_chathistory_message( &server, message.with_target(target), @@ -656,17 +630,17 @@ impl Application for Halloy { ); } } - data::client::Event::ChatHistoryBatchFinished( - subcommand, - target, - message_reference, - ) => { + data::client::Event::ChatHistoryBatchFinished(subcommand, target, message_reference) => { match subcommand { ChatHistorySubcommand::Latest(_) => { - log::debug!("[{server}] received latest messages in {target} since {message_reference}",) + log::debug!( + "[{server}] received latest messages in {target} since {message_reference}", + ) } ChatHistorySubcommand::Before => { - log::debug!("[{server}] received messages in {target} before {message_reference}",) + log::debug!( + "[{server}] received messages in {target} before {message_reference}", + ) } } diff --git a/src/screen/dashboard.rs b/src/screen/dashboard.rs index f06970d5..afb1ec15 100644 --- a/src/screen/dashboard.rs +++ b/src/screen/dashboard.rs @@ -973,10 +973,7 @@ impl Dashboard { } } isupport::MessageReferenceType::Timestamp => { - return MessageReference::Timestamp( - latest_message.server_time, - latest_message.id.clone().unwrap_or(":".to_string()), - ); + return MessageReference::Timestamp(latest_message.server_time); } } } @@ -1008,10 +1005,7 @@ impl Dashboard { } } isupport::MessageReferenceType::Timestamp => { - return MessageReference::Timestamp( - oldest_message.server_time, - oldest_message.id.clone().unwrap_or(":".to_string()), - ); + return MessageReference::Timestamp(oldest_message.server_time); } } }