From a3562c5940b078f8028bd281d7ed27b934f8d7d9 Mon Sep 17 00:00:00 2001 From: link2xt Date: Sat, 9 Oct 2021 10:39:44 +0000 Subject: [PATCH] Remove `get_summarytext_by_raw` Use `Summary.truncated_text()` instead. Co-Authored-By: Floris Bruynooghe --- deltachat-ffi/src/lib.rs | 17 +- src/lot.rs | 3 +- src/message.rs | 22 +-- src/mimefactory.rs | 6 +- src/summary.rs | 347 +++++++++++++++------------------------ 5 files changed, 158 insertions(+), 237 deletions(-) diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index c000a174c..54cd1b269 100644 --- a/deltachat-ffi/src/lib.rs +++ b/deltachat-ffi/src/lib.rs @@ -15,7 +15,7 @@ extern crate num_traits; extern crate serde_json; use std::collections::BTreeMap; -use std::convert::TryInto; +use std::convert::TryFrom; use std::fmt::Write; use std::ops::Deref; use std::ptr; @@ -3047,7 +3047,7 @@ pub unsafe extern "C" fn dc_msg_get_summary( let ffi_msg = &mut *msg; let ctx = &*ffi_msg.context; - let summary = block_on(async move { ffi_msg.message.get_summary(ctx, maybe_chat).await }) + let summary = block_on(ffi_msg.message.get_summary(ctx, maybe_chat)) .log_err(ctx, "dc_msg_get_summary failed") .unwrap_or_default(); Box::into_raw(Box::new(summary.into())) @@ -3065,12 +3065,13 @@ pub unsafe extern "C" fn dc_msg_get_summarytext( let ffi_msg = &mut *msg; let ctx = &*ffi_msg.context; - block_on({ - ffi_msg - .message - .get_summarytext(ctx, approx_characters.try_into().unwrap_or_default()) - }) - .strdup() + let summary = block_on(ffi_msg.message.get_summary(ctx, None)) + .log_err(ctx, "dc_msg_get_summarytext failed") + .unwrap_or_default(); + match usize::try_from(approx_characters) { + Ok(chars) => summary.truncated_text(chars).strdup(), + Err(_) => summary.text.strdup(), + } } #[no_mangle] diff --git a/src/lot.rs b/src/lot.rs index cac5cc6bf..db54fefa4 100644 --- a/src/lot.rs +++ b/src/lot.rs @@ -162,6 +162,7 @@ impl From for LotState { impl From for Lot { fn from(summary: Summary) -> Self { + let text2 = Some(summary.truncated_text(160).to_string()); let (text1, text1_meaning) = match summary.prefix { None => (None, Meaning::None), Some(SummaryPrefix::Draft(text)) => (Some(text), Meaning::Text1Draft), @@ -171,7 +172,7 @@ impl From for Lot { Self { text1_meaning, text1, - text2: Some(summary.text), + text2, timestamp: summary.timestamp, state: summary.state.into(), ..Default::default() diff --git a/src/message.rs b/src/message.rs index c7ebeca5e..46a9cbe9f 100644 --- a/src/message.rs +++ b/src/message.rs @@ -30,7 +30,7 @@ use crate::mimeparser::{parse_message_id, FailureReport, SystemMessage}; use crate::param::{Param, Params}; use crate::pgp::split_armored_data; use crate::stock_str; -use crate::summary::{get_summarytext_by_raw, Summary}; +use crate::summary::Summary; /// Message ID, including reserved IDs. /// @@ -592,7 +592,7 @@ impl Message { } /// Returns message summary for display in the search results. - pub async fn get_summary(&mut self, context: &Context, chat: Option<&Chat>) -> Result { + pub async fn get_summary(&self, context: &Context, chat: Option<&Chat>) -> Result { let chat_loaded: Chat; let chat = if let Some(chat) = chat { chat @@ -616,18 +616,6 @@ impl Message { Ok(Summary::new(context, self, chat, contact.as_ref()).await) } - pub async fn get_summarytext(&self, context: &Context, approx_characters: usize) -> String { - get_summarytext_by_raw( - self.viewtype, - self.text.as_ref(), - self.is_forwarded(), - &self.param, - approx_characters, - context, - ) - .await - } - // It's a little unfortunate that the UI has to first call dc_msg_get_override_sender_name() and then if it was NULL, call // dc_contact_get_display_name() but this was the best solution: // - We could load a Contact struct from the db here to call get_display_name() instead of returning None, but then we had a db @@ -871,7 +859,11 @@ impl Message { Param::Quote, if text.is_empty() { // Use summary, similar to "Image" to avoid sending empty quote. - quote.get_summarytext(context, 500).await + quote + .get_summary(context, None) + .await? + .truncated_text(500) + .to_string() } else { text }, diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 210de7eb0..bf9f9fe0d 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -1157,7 +1157,11 @@ impl<'a> MimeFactory<'a> { { stock_str::encrypted_msg(context).await } else { - self.msg.get_summarytext(context, 32).await + self.msg + .get_summary(context, None) + .await? + .truncated_text(32) + .to_string() }; let p2 = stock_str::read_rcpt_mail_body(context, p1).await; let message_text = format!("{}\r\n", format_flowed(&p2)); diff --git a/src/summary.rs b/src/summary.rs index 885ff2a85..fc6adc2d4 100644 --- a/src/summary.rs +++ b/src/summary.rs @@ -7,15 +7,12 @@ use crate::context::Context; use crate::dc_tools::dc_truncate; use crate::message::{Message, MessageState}; use crate::mimeparser::SystemMessage; -use crate::param::{Param, Params}; +use crate::param::Param; use crate::stock_str; use itertools::Itertools; +use std::borrow::Cow; use std::fmt; -// In practice, the user additionally cuts the string themselves -// pixel-accurate. -const SUMMARY_CHARACTERS: usize = 160; - /// Prefix displayed before message and separated by ":" in the chatlist. #[derive(Debug)] pub enum SummaryPrefix { @@ -85,15 +82,7 @@ impl Summary { } }; - let mut text = get_summarytext_by_raw( - msg.viewtype, - msg.text.as_ref(), - msg.is_forwarded(), - &msg.param, - SUMMARY_CHARACTERS, - context, - ) - .await; + let mut text = msg.get_summary_text(context).await; if text.is_empty() && msg.quoted_text().is_some() { text = stock_str::reply_noun(context).await @@ -106,88 +95,87 @@ impl Summary { state: msg.state, } } + + /// Returns the [`Summary::text`] attribute truncated to an approximate length. + pub fn truncated_text(&self, approx_chars: usize) -> Cow { + dc_truncate(&self.text, approx_chars) + } } -/// Returns a summary text. -pub async fn get_summarytext_by_raw( - viewtype: Viewtype, - text: Option>, - was_forwarded: bool, - param: &Params, - approx_characters: usize, - context: &Context, -) -> String { - let mut append_text = true; - let prefix = match viewtype { - Viewtype::Image => stock_str::image(context).await, - Viewtype::Gif => stock_str::gif(context).await, - Viewtype::Sticker => stock_str::sticker(context).await, - Viewtype::Video => stock_str::video(context).await, - Viewtype::Voice => stock_str::voice_message(context).await, - Viewtype::Audio | Viewtype::File => { - if param.get_cmd() == SystemMessage::AutocryptSetupMessage { - append_text = false; - stock_str::ac_setup_msg_subject(context).await - } else { - let file_name: String = param - .get_path(Param::File, context) - .unwrap_or(None) - .and_then(|path| { - path.file_name() - .map(|fname| fname.to_string_lossy().into_owned()) - }) - .unwrap_or_else(|| String::from("ErrFileName")); - let label = if viewtype == Viewtype::Audio { - stock_str::audio(context).await +impl Message { + /// Returns a summary text. + async fn get_summary_text(&self, context: &Context) -> String { + let mut append_text = true; + let prefix = match self.viewtype { + Viewtype::Image => stock_str::image(context).await, + Viewtype::Gif => stock_str::gif(context).await, + Viewtype::Sticker => stock_str::sticker(context).await, + Viewtype::Video => stock_str::video(context).await, + Viewtype::Voice => stock_str::voice_message(context).await, + Viewtype::Audio | Viewtype::File => { + if self.param.get_cmd() == SystemMessage::AutocryptSetupMessage { + append_text = false; + stock_str::ac_setup_msg_subject(context).await } else { - stock_str::file(context).await - }; - format!("{} – {}", label, file_name) + let file_name: String = self + .param + .get_path(Param::File, context) + .unwrap_or(None) + .and_then(|path| { + path.file_name() + .map(|fname| fname.to_string_lossy().into_owned()) + }) + .unwrap_or_else(|| String::from("ErrFileName")); + let label = if self.viewtype == Viewtype::Audio { + stock_str::audio(context).await + } else { + stock_str::file(context).await + }; + format!("{} – {}", label, file_name) + } } - } - Viewtype::VideochatInvitation => { - append_text = false; - stock_str::videochat_invitation(context).await - } - _ => { - if param.get_cmd() != SystemMessage::LocationOnly { - "".to_string() - } else { + Viewtype::VideochatInvitation => { append_text = false; - stock_str::location(context).await + stock_str::videochat_invitation(context).await } + _ => { + if self.param.get_cmd() != SystemMessage::LocationOnly { + "".to_string() + } else { + append_text = false; + stock_str::location(context).await + } + } + }; + + if !append_text { + return prefix; } - }; - if !append_text { - return prefix; - } - - let summary_content = if let Some(text) = text { - if text.as_ref().is_empty() { - prefix - } else if prefix.is_empty() { - dc_truncate(text.as_ref(), approx_characters).to_string() + let summary_content = if let Some(text) = &self.text { + if text.is_empty() { + prefix + } else if prefix.is_empty() { + text.to_string() + } else { + format!("{} – {}", prefix, text) + } } else { - let tmp = format!("{} – {}", prefix, text.as_ref()); - dc_truncate(&tmp, approx_characters).to_string() - } - } else { - prefix - }; + prefix + }; - let summary = if was_forwarded { - let tmp = format!( - "{}: {}", - stock_str::forwarded(context).await, + let summary = if self.is_forwarded() { + format!( + "{}: {}", + stock_str::forwarded(context).await, + summary_content + ) + } else { summary_content - ); - dc_truncate(&tmp, approx_characters).to_string() - } else { - summary_content - }; + }; - summary.split_whitespace().join(" ") + summary.split_whitespace().join(" ") + } } #[cfg(test)] @@ -196,7 +184,7 @@ mod tests { use crate::test_utils as test; #[async_std::test] - async fn test_get_summarytext_by_raw() { + async fn test_get_summary_text() { let d = test::TestContext::new().await; let ctx = &d.ctx; @@ -204,190 +192,125 @@ mod tests { let empty_text = Some("".to_string()); let no_text: Option = None; - let mut some_file = Params::new(); - some_file.set(Param::File, "foo.bar"); - + let mut msg = Message::new(Viewtype::Text); + msg.set_text(some_text.clone()); assert_eq!( - get_summarytext_by_raw( - Viewtype::Text, - some_text.as_ref(), - false, - &Params::new(), - 50, - ctx - ) - .await, + msg.get_summary_text(ctx).await, "bla bla" // for simple text, the type is not added to the summary ); + let mut msg = Message::new(Viewtype::Image); + msg.set_text(no_text.clone()); + msg.set_file("foo.bar", None); assert_eq!( - get_summarytext_by_raw( - Viewtype::Image, - no_text.as_ref(), - false, - &some_file, - 50, - ctx - ) - .await, + msg.get_summary_text(ctx).await, "Image" // file names are not added for images ); + let mut msg = Message::new(Viewtype::Video); + msg.set_text(no_text.clone()); + msg.set_file("foo.bar", None); assert_eq!( - get_summarytext_by_raw( - Viewtype::Video, - no_text.as_ref(), - false, - &some_file, - 50, - ctx - ) - .await, + msg.get_summary_text(ctx).await, "Video" // file names are not added for videos ); + let mut msg = Message::new(Viewtype::Gif); + msg.set_text(no_text.clone()); + msg.set_file("foo.bar", None); assert_eq!( - get_summarytext_by_raw(Viewtype::Gif, no_text.as_ref(), false, &some_file, 50, ctx,) - .await, + msg.get_summary_text(ctx).await, "GIF" // file names are not added for GIFs ); + let mut msg = Message::new(Viewtype::Sticker); + msg.set_text(no_text.clone()); + msg.set_file("foo.bar", None); assert_eq!( - get_summarytext_by_raw( - Viewtype::Sticker, - no_text.as_ref(), - false, - &some_file, - 50, - ctx, - ) - .await, + msg.get_summary_text(ctx).await, "Sticker" // file names are not added for stickers ); + let mut msg = Message::new(Viewtype::Voice); + msg.set_text(empty_text.clone()); + msg.set_file("foo.bar", None); assert_eq!( - get_summarytext_by_raw( - Viewtype::Voice, - empty_text.as_ref(), - false, - &some_file, - 50, - ctx, - ) - .await, + msg.get_summary_text(ctx).await, "Voice message" // file names are not added for voice messages, empty text is skipped ); + let mut msg = Message::new(Viewtype::Voice); + msg.set_text(no_text.clone()); + msg.set_file("foo.bar", None); assert_eq!( - get_summarytext_by_raw( - Viewtype::Voice, - no_text.as_ref(), - false, - &some_file, - 50, - ctx - ) - .await, + msg.get_summary_text(ctx).await, "Voice message" // file names are not added for voice messages ); + let mut msg = Message::new(Viewtype::Voice); + msg.set_text(some_text.clone()); + msg.set_file("foo.bar", None); assert_eq!( - get_summarytext_by_raw( - Viewtype::Voice, - some_text.as_ref(), - false, - &some_file, - 50, - ctx - ) - .await, + msg.get_summary_text(ctx).await, "Voice message \u{2013} bla bla" // `\u{2013}` explicitly checks for "EN DASH" ); + let mut msg = Message::new(Viewtype::Audio); + msg.set_text(no_text.clone()); + msg.set_file("foo.bar", None); assert_eq!( - get_summarytext_by_raw( - Viewtype::Audio, - no_text.as_ref(), - false, - &some_file, - 50, - ctx - ) - .await, + msg.get_summary_text(ctx).await, "Audio \u{2013} foo.bar" // file name is added for audio ); + let mut msg = Message::new(Viewtype::Audio); + msg.set_text(empty_text.clone()); + msg.set_file("foo.bar", None); assert_eq!( - get_summarytext_by_raw( - Viewtype::Audio, - empty_text.as_ref(), - false, - &some_file, - 50, - ctx, - ) - .await, + msg.get_summary_text(ctx).await, "Audio \u{2013} foo.bar" // file name is added for audio, empty text is not added ); + let mut msg = Message::new(Viewtype::Audio); + msg.set_text(some_text.clone()); + msg.set_file("foo.bar", None); assert_eq!( - get_summarytext_by_raw( - Viewtype::Audio, - some_text.as_ref(), - false, - &some_file, - 50, - ctx - ) - .await, + msg.get_summary_text(ctx).await, "Audio \u{2013} foo.bar \u{2013} bla bla" // file name and text added for audio ); + let mut msg = Message::new(Viewtype::File); + msg.set_text(some_text.clone()); + msg.set_file("foo.bar", None); assert_eq!( - get_summarytext_by_raw( - Viewtype::File, - some_text.as_ref(), - false, - &some_file, - 50, - ctx - ) - .await, + msg.get_summary_text(ctx).await, "File \u{2013} foo.bar \u{2013} bla bla" // file name is added for files ); // Forwarded + let mut msg = Message::new(Viewtype::Text); + msg.set_text(some_text.clone()); + msg.param.set_int(Param::Forwarded, 1); assert_eq!( - get_summarytext_by_raw( - Viewtype::Text, - some_text.as_ref(), - true, - &Params::new(), - 50, - ctx - ) - .await, + msg.get_summary_text(ctx).await, "Forwarded: bla bla" // for simple text, the type is not added to the summary ); + + let mut msg = Message::new(Viewtype::File); + msg.set_text(some_text.clone()); + msg.set_file("foo.bar", None); + msg.param.set_int(Param::Forwarded, 1); assert_eq!( - get_summarytext_by_raw( - Viewtype::File, - some_text.as_ref(), - true, - &some_file, - 50, - ctx - ) - .await, + msg.get_summary_text(ctx).await, "Forwarded: File \u{2013} foo.bar \u{2013} bla bla" ); - let mut asm_file = Params::new(); - asm_file.set(Param::File, "foo.bar"); - asm_file.set_cmd(SystemMessage::AutocryptSetupMessage); + let mut msg = Message::new(Viewtype::File); + msg.set_text(no_text.clone()); + msg.param.set(Param::File, "foo.bar"); + msg.param.set_cmd(SystemMessage::AutocryptSetupMessage); assert_eq!( - get_summarytext_by_raw(Viewtype::File, no_text.as_ref(), false, &asm_file, 50, ctx) - .await, + msg.get_summary_text(ctx).await, "Autocrypt Setup Message" // file name is not added for autocrypt setup messages ); }