From 45b0a4ec2721b73005995003acf9d4d305ed9a4c Mon Sep 17 00:00:00 2001 From: Dmitry Bogatov Date: Sat, 21 Sep 2019 05:24:03 +0000 Subject: [PATCH 1/7] Factor part of `set_draft_raw` into new function `maybe_delete_draft` --- src/chat.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 19e78478d..d9d922a25 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -849,18 +849,24 @@ pub unsafe fn set_draft(context: &Context, chat_id: u32, msg: Option<&mut Messag }; } +/// Delete draft message in specified chat, if there is one. +/// +/// Return {true}, if message was deleted, {false} otherwise. +fn maybe_delete_draft(context: &Context, chat_id: u32) -> bool { + let draft = get_draft_msg_id(context, chat_id); + if draft != 0 { + dc_delete_msg_from_db(context, draft); + return true; + } + false +} + // similar to as dc_set_draft() but does not emit an event #[allow(non_snake_case)] unsafe fn set_draft_raw(context: &Context, chat_id: u32, mut msg: Option<&mut Message>) -> bool { let mut OK_TO_CONTINUE = true; - let mut sth_changed = false; - - let prev_draft_msg_id = get_draft_msg_id(context, chat_id); - if 0 != prev_draft_msg_id { - dc_delete_msg_from_db(context, prev_draft_msg_id); - sth_changed = true; - } + let mut sth_changed = maybe_delete_draft(context, chat_id); if let Some(ref mut msg) = msg { // save new draft From dbf14179dcbecafe07aa6566b5976f8224246cc3 Mon Sep 17 00:00:00 2001 From: Dmitry Bogatov Date: Sat, 21 Sep 2019 05:34:48 +0000 Subject: [PATCH 2/7] Make message argument to set_draft_raw() no longer optional Previously, "set_draft_raw" function was used with "msg = None" only for side-effect of removing current draft message in chat. After draft removal functionality was factored into separate "maybe_delete_draft" function, it is used directly in only call site, which used to invoke "set_draft_raw" with optional message. This function is private, and this refactoring does not change FFI interface. Note: This commit fails CI due incorrect formatting. It is done deliberately to simplify review process. --- src/chat.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index d9d922a25..158b608b5 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -844,9 +844,15 @@ pub unsafe fn set_draft(context: &Context, chat_id: u32, msg: Option<&mut Messag if chat_id <= DC_CHAT_ID_LAST_SPECIAL { return; } - if set_draft_raw(context, chat_id, msg) { - context.call_cb(Event::MsgsChanged { chat_id, msg_id: 0 }); + + let changed = match msg { + None => maybe_delete_draft(context, chat_id), + Some(msg) => set_draft_raw(context, chat_id, msg), }; + + if changed { + context.call_cb(Event::MsgsChanged { chat_id, msg_id: 0 }); + } } /// Delete draft message in specified chat, if there is one. @@ -863,12 +869,12 @@ fn maybe_delete_draft(context: &Context, chat_id: u32) -> bool { // similar to as dc_set_draft() but does not emit an event #[allow(non_snake_case)] -unsafe fn set_draft_raw(context: &Context, chat_id: u32, mut msg: Option<&mut Message>) -> bool { +unsafe fn set_draft_raw(context: &Context, chat_id: u32, msg: &mut Message) -> bool { let mut OK_TO_CONTINUE = true; let mut sth_changed = maybe_delete_draft(context, chat_id); - if let Some(ref mut msg) = msg { + { // save new draft if msg.type_0 == Viewtype::Text { OK_TO_CONTINUE = msg.text.as_ref().map_or(false, |s| !s.is_empty()); @@ -1294,7 +1300,7 @@ pub unsafe fn create_group_chat( if add_to_chat_contacts_table(context, chat_id, 1) { let mut draft_msg = dc_msg_new(Viewtype::Text); dc_msg_set_text(&mut draft_msg, draft_txt.as_ptr()); - set_draft_raw(context, chat_id, Some(&mut draft_msg)); + set_draft_raw(context, chat_id, &mut draft_msg); } context.call_cb(Event::MsgsChanged { From ad32b5ca8ff3073b4ab939d93b077c380fb006ce Mon Sep 17 00:00:00 2001 From: Dmitry Bogatov Date: Sat, 21 Sep 2019 05:44:31 +0000 Subject: [PATCH 3/7] cargo-fmt --- src/chat.rs | 72 ++++++++++++++++++++++++++--------------------------- 1 file changed, 35 insertions(+), 37 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 158b608b5..b810a801d 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -874,45 +874,43 @@ unsafe fn set_draft_raw(context: &Context, chat_id: u32, msg: &mut Message) -> b let mut sth_changed = maybe_delete_draft(context, chat_id); - { - // save new draft - if msg.type_0 == Viewtype::Text { - OK_TO_CONTINUE = msg.text.as_ref().map_or(false, |s| !s.is_empty()); - } else if msgtype_has_file(msg.type_0) { - if let Some(path_filename) = msg.param.get(Param::File) { - let mut path_filename = path_filename.to_string(); - if dc_msg_is_increation(msg) && !dc_is_blobdir_path(context, &path_filename) { - OK_TO_CONTINUE = false; - } else if !dc_make_rel_and_copy(context, &mut path_filename) { - OK_TO_CONTINUE = false; - } else { - msg.param.set(Param::File, path_filename); - } + // save new draft + if msg.type_0 == Viewtype::Text { + OK_TO_CONTINUE = msg.text.as_ref().map_or(false, |s| !s.is_empty()); + } else if msgtype_has_file(msg.type_0) { + if let Some(path_filename) = msg.param.get(Param::File) { + let mut path_filename = path_filename.to_string(); + if dc_msg_is_increation(msg) && !dc_is_blobdir_path(context, &path_filename) { + OK_TO_CONTINUE = false; + } else if !dc_make_rel_and_copy(context, &mut path_filename) { + OK_TO_CONTINUE = false; + } else { + msg.param.set(Param::File, path_filename); } - } else { - OK_TO_CONTINUE = false; } - if OK_TO_CONTINUE { - if sql::execute( - context, - &context.sql, - "INSERT INTO msgs (chat_id, from_id, timestamp, type, state, txt, param, hidden) \ - VALUES (?,?,?, ?,?,?,?,?);", - params![ - chat_id as i32, - 1, - time(), - msg.type_0, - MessageState::OutDraft, - msg.text.as_ref().map(String::as_str).unwrap_or(""), - msg.param.to_string(), - 1, - ], - ) - .is_ok() - { - sth_changed = true; - } + } else { + OK_TO_CONTINUE = false; + } + if OK_TO_CONTINUE { + if sql::execute( + context, + &context.sql, + "INSERT INTO msgs (chat_id, from_id, timestamp, type, state, txt, param, hidden) \ + VALUES (?,?,?, ?,?,?,?,?);", + params![ + chat_id as i32, + 1, + time(), + msg.type_0, + MessageState::OutDraft, + msg.text.as_ref().map(String::as_str).unwrap_or(""), + msg.param.to_string(), + 1, + ], + ) + .is_ok() + { + sth_changed = true; } } sth_changed From 26f176eb7eefe80766990de3351f40a40407c45b Mon Sep 17 00:00:00 2001 From: Dmitry Bogatov Date: Sat, 21 Sep 2019 06:04:55 +0000 Subject: [PATCH 4/7] Factor another part of `set_draft_raw` into separate function --- src/chat.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index b810a801d..8c4ba3912 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -867,12 +867,12 @@ fn maybe_delete_draft(context: &Context, chat_id: u32) -> bool { false } -// similar to as dc_set_draft() but does not emit an event -#[allow(non_snake_case)] -unsafe fn set_draft_raw(context: &Context, chat_id: u32, msg: &mut Message) -> bool { +/// Set provided message as draft message for specified chat. +/// +/// Return true on success, false on database error. +fn do_set_draft(context: &Context, chat_id: u32, msg: &mut Message) -> bool { let mut OK_TO_CONTINUE = true; - - let mut sth_changed = maybe_delete_draft(context, chat_id); + let mut sth_changed = false; // save new draft if msg.type_0 == Viewtype::Text { @@ -916,6 +916,16 @@ unsafe fn set_draft_raw(context: &Context, chat_id: u32, msg: &mut Message) -> b sth_changed } +// similar to as dc_set_draft() but does not emit an event +#[allow(non_snake_case)] +unsafe fn set_draft_raw(context: &Context, chat_id: u32, msg: &mut Message) -> bool { + let deleted = maybe_delete_draft(context, chat_id); + let set = do_set_draft(context, chat_id, msg); + + // Can't inline. Both functions above must be called, no shortcut! + deleted || set +} + fn get_draft_msg_id(context: &Context, chat_id: u32) -> u32 { context .sql From 0523868a88e0fd98c4d2e22e351d40969f51da6e Mon Sep 17 00:00:00 2001 From: Dmitry Bogatov Date: Sat, 21 Sep 2019 06:16:15 +0000 Subject: [PATCH 5/7] Avoid ok-to-continue pattern in "do_set_draft" function Note: I strongly suggest reviewing this commit in side-by-side mode. Note: This commit fails CI due incorrect formatting. It is done deliberately to simplify review process. --- src/chat.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 8c4ba3912..2098106f8 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -871,27 +871,27 @@ fn maybe_delete_draft(context: &Context, chat_id: u32) -> bool { /// /// Return true on success, false on database error. fn do_set_draft(context: &Context, chat_id: u32, msg: &mut Message) -> bool { - let mut OK_TO_CONTINUE = true; let mut sth_changed = false; - // save new draft - if msg.type_0 == Viewtype::Text { - OK_TO_CONTINUE = msg.text.as_ref().map_or(false, |s| !s.is_empty()); - } else if msgtype_has_file(msg.type_0) { + match msg.type_0 { + Viewtype::Unknown => return false, + Viewtype::Text => if msg.text.as_ref().map_or(false, |s| s.is_empty()) { + return false; + }, + _ => if let Some(path_filename) = msg.param.get(Param::File) { let mut path_filename = path_filename.to_string(); if dc_msg_is_increation(msg) && !dc_is_blobdir_path(context, &path_filename) { - OK_TO_CONTINUE = false; - } else if !dc_make_rel_and_copy(context, &mut path_filename) { - OK_TO_CONTINUE = false; - } else { - msg.param.set(Param::File, path_filename); + return false; } + if !dc_make_rel_and_copy(context, &mut path_filename) { + return false; + } + msg.param.set(Param::File, path_filename); } - } else { - OK_TO_CONTINUE = false; } - if OK_TO_CONTINUE { + + { if sql::execute( context, &context.sql, From 5b917e7d10144ba902e7285d10e10dc9dda7e2df Mon Sep 17 00:00:00 2001 From: Dmitry Bogatov Date: Sat, 21 Sep 2019 06:19:21 +0000 Subject: [PATCH 6/7] Remove last mutable variable from do_set_draft --- src/chat.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 2098106f8..2ed97b557 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -871,7 +871,6 @@ fn maybe_delete_draft(context: &Context, chat_id: u32) -> bool { /// /// Return true on success, false on database error. fn do_set_draft(context: &Context, chat_id: u32, msg: &mut Message) -> bool { - let mut sth_changed = false; match msg.type_0 { Viewtype::Unknown => return false, @@ -891,8 +890,7 @@ fn do_set_draft(context: &Context, chat_id: u32, msg: &mut Message) -> bool { } } - { - if sql::execute( + sql::execute( context, &context.sql, "INSERT INTO msgs (chat_id, from_id, timestamp, type, state, txt, param, hidden) \ @@ -909,11 +907,6 @@ fn do_set_draft(context: &Context, chat_id: u32, msg: &mut Message) -> bool { ], ) .is_ok() - { - sth_changed = true; - } - } - sth_changed } // similar to as dc_set_draft() but does not emit an event From 43f6db3252107e8166ce931eb68f265821d910c9 Mon Sep 17 00:00:00 2001 From: Dmitry Bogatov Date: Sat, 21 Sep 2019 06:19:55 +0000 Subject: [PATCH 7/7] cargo-fmt --- src/chat.rs | 58 +++++++++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 2ed97b557..546afc81f 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -871,42 +871,44 @@ fn maybe_delete_draft(context: &Context, chat_id: u32) -> bool { /// /// Return true on success, false on database error. fn do_set_draft(context: &Context, chat_id: u32, msg: &mut Message) -> bool { - match msg.type_0 { Viewtype::Unknown => return false, - Viewtype::Text => if msg.text.as_ref().map_or(false, |s| s.is_empty()) { - return false; - }, - _ => - if let Some(path_filename) = msg.param.get(Param::File) { - let mut path_filename = path_filename.to_string(); - if dc_msg_is_increation(msg) && !dc_is_blobdir_path(context, &path_filename) { + Viewtype::Text => { + if msg.text.as_ref().map_or(false, |s| s.is_empty()) { return false; } - if !dc_make_rel_and_copy(context, &mut path_filename) { - return false; + } + _ => { + if let Some(path_filename) = msg.param.get(Param::File) { + let mut path_filename = path_filename.to_string(); + if dc_msg_is_increation(msg) && !dc_is_blobdir_path(context, &path_filename) { + return false; + } + if !dc_make_rel_and_copy(context, &mut path_filename) { + return false; + } + msg.param.set(Param::File, path_filename); } - msg.param.set(Param::File, path_filename); } } - sql::execute( - context, - &context.sql, - "INSERT INTO msgs (chat_id, from_id, timestamp, type, state, txt, param, hidden) \ - VALUES (?,?,?, ?,?,?,?,?);", - params![ - chat_id as i32, - 1, - time(), - msg.type_0, - MessageState::OutDraft, - msg.text.as_ref().map(String::as_str).unwrap_or(""), - msg.param.to_string(), - 1, - ], - ) - .is_ok() + sql::execute( + context, + &context.sql, + "INSERT INTO msgs (chat_id, from_id, timestamp, type, state, txt, param, hidden) \ + VALUES (?,?,?, ?,?,?,?,?);", + params![ + chat_id as i32, + 1, + time(), + msg.type_0, + MessageState::OutDraft, + msg.text.as_ref().map(String::as_str).unwrap_or(""), + msg.param.to_string(), + 1, + ], + ) + .is_ok() } // similar to as dc_set_draft() but does not emit an event