Skip to content

Commit

Permalink
fix: replace connectivity state "Connected" with "Preparing"
Browse files Browse the repository at this point in the history
This better reflects that this state means
we just connected and there may me work to do.
This state is converted to DC_CONNECTIVITY_WORKING
instead of DC_CONNECTIVITY_CONNECTED state now.

Before this change when IMAP connected
to the server, it switched
from DC_CONNECTIVITY_NOT_CONNECTED
to DC_CONNECTIVITY_CONNECTING,
then to DC_CONNECTIVITY_CONNECTED (actually preparing)
then to DC_CONNECTIVITY_WORKING
and then to DC_CONNECTIVITY_CONNECTED again (actually idle).

On fast connections this resulted in flickering "Connected"
string in the status bar right before "Updating..."
and on slow connections this "Connected" state
before "Updating..." lasted for a while
leaving the user to wonder if there are no new messages
or if Delta Chat will still switch to "Updating..."
before going into "Connected" state again.
  • Loading branch information
link2xt committed Dec 2, 2024
1 parent 191eb7e commit fc136ef
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 23 deletions.
2 changes: 1 addition & 1 deletion python/tests/test_1_online.py
Original file line number Diff line number Diff line change
Expand Up @@ -1899,7 +1899,7 @@ def test_connectivity(acfactory, lp):

ac1.start_io()
ac1._evtracker.wait_for_connectivity(dc.const.DC_CONNECTIVITY_CONNECTING)
ac1._evtracker.wait_for_connectivity_change(dc.const.DC_CONNECTIVITY_CONNECTING, dc.const.DC_CONNECTIVITY_CONNECTED)
ac1._evtracker.wait_for_connectivity_change(dc.const.DC_CONNECTIVITY_CONNECTING, dc.const.DC_CONNECTIVITY_WORKING)

lp.sec(
"Test that after calling start_io(), maybe_network() and waiting for `all_work_done()`, "
Expand Down
2 changes: 1 addition & 1 deletion src/imap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ impl Imap {
"IMAP-LOGIN as {}",
lp.user
)));
self.connectivity.set_connected(context).await;
self.connectivity.set_preparing(context).await;
info!(context, "Successfully logged into IMAP server");
return Ok(session);
}
Expand Down
66 changes: 45 additions & 21 deletions src/scheduler/connectivity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,25 @@ use super::InnerSchedulerState;

#[derive(Debug, Clone, Copy, PartialEq, Eq, EnumProperty, PartialOrd, Ord)]
pub enum Connectivity {
/// Not connected.
///
/// This may be because we just started,
/// because we lost connection and
/// were not able to connect and login yet
/// or because I/O is not started.
NotConnected = 1000,

/// Attempting to connect and log in.
Connecting = 2000,
/// Fetching or sending messages

/// Fetching or sending messages.
Working = 3000,

/// We are connected but not doing anything.
///
/// UIs display this as Connected,
/// which signals that no more messages
/// are coming in.
Connected = 4000,
}

Expand All @@ -32,13 +47,17 @@ enum DetailedConnectivity {
Error(String),
#[default]
Uninitialized,

/// Attempting to connect,
/// until we successully log in.
Connecting,

/// Connection is just established, but there may be work to do.
Connected,
/// Connection is just established,
/// there may be work to do.
Preparing,

/// There is actual work to do, e.g. there are messages in SMTP queue
/// or we detected a message that should be downloaded.
/// or we detected a message on IMAP server that should be downloaded.
Working,

InterruptingIdle,
Expand All @@ -58,7 +77,13 @@ impl DetailedConnectivity {
DetailedConnectivity::Connecting => Some(Connectivity::Connecting),
DetailedConnectivity::Working => Some(Connectivity::Working),
DetailedConnectivity::InterruptingIdle => Some(Connectivity::Connected),
DetailedConnectivity::Connected => Some(Connectivity::Connected),

// At this point IMAP has just connected,
// but does not know yet if there are messages to download.
// We still convert this to Working state
// so user can see "Updating..." and not "Connected"
// which is reserved for idle state.
DetailedConnectivity::Preparing => Some(Connectivity::Working),

// Just don't return a connectivity, probably the folder is configured not to be
// watched or there is e.g. no "Sent" folder, so we are not interested in it
Expand All @@ -74,9 +99,9 @@ impl DetailedConnectivity {
| DetailedConnectivity::Uninitialized
| DetailedConnectivity::NotConfigured => "<span class=\"red dot\"></span>".to_string(),
DetailedConnectivity::Connecting => "<span class=\"yellow dot\"></span>".to_string(),
DetailedConnectivity::Working
DetailedConnectivity::Preparing
| DetailedConnectivity::Working
| DetailedConnectivity::InterruptingIdle
| DetailedConnectivity::Connected
| DetailedConnectivity::Idle => "<span class=\"green dot\"></span>".to_string(),
}
}
Expand All @@ -86,10 +111,12 @@ impl DetailedConnectivity {
DetailedConnectivity::Error(e) => stock_str::error(context, e).await,
DetailedConnectivity::Uninitialized => "Not started".to_string(),
DetailedConnectivity::Connecting => stock_str::connecting(context).await,
DetailedConnectivity::Working => stock_str::updating(context).await,
DetailedConnectivity::InterruptingIdle
| DetailedConnectivity::Connected
| DetailedConnectivity::Idle => stock_str::connected(context).await,
DetailedConnectivity::Preparing | DetailedConnectivity::Working => {
stock_str::updating(context).await
}
DetailedConnectivity::InterruptingIdle | DetailedConnectivity::Idle => {
stock_str::connected(context).await
}
DetailedConnectivity::NotConfigured => "Not configured".to_string(),
}
}
Expand All @@ -107,7 +134,7 @@ impl DetailedConnectivity {
// since sending the last message, connectivity could have changed, which we don't notice
// until another message is sent
DetailedConnectivity::InterruptingIdle
| DetailedConnectivity::Connected
| DetailedConnectivity::Preparing
| DetailedConnectivity::Idle => stock_str::last_msg_sent_successfully(context).await,
DetailedConnectivity::NotConfigured => "Not configured".to_string(),
}
Expand All @@ -120,7 +147,7 @@ impl DetailedConnectivity {
DetailedConnectivity::Connecting => false,
DetailedConnectivity::Working => false,
DetailedConnectivity::InterruptingIdle => false,
DetailedConnectivity::Connected => false, // Just connected, there may still be work to do.
DetailedConnectivity::Preparing => false, // Just connected, there may still be work to do.
DetailedConnectivity::NotConfigured => true,
DetailedConnectivity::Idle => true,
}
Expand Down Expand Up @@ -148,8 +175,8 @@ impl ConnectivityStore {
pub(crate) async fn set_working(&self, context: &Context) {
self.set(context, DetailedConnectivity::Working).await;
}
pub(crate) async fn set_connected(&self, context: &Context) {
self.set(context, DetailedConnectivity::Connected).await;
pub(crate) async fn set_preparing(&self, context: &Context) {
self.set(context, DetailedConnectivity::Preparing).await;
}
pub(crate) async fn set_not_configured(&self, context: &Context) {
self.set(context, DetailedConnectivity::NotConfigured).await;
Expand All @@ -169,7 +196,7 @@ impl ConnectivityStore {
}
}

/// Set all folder states to InterruptingIdle in case they were `Connected` before.
/// Set all folder states to InterruptingIdle in case they were `Idle` before.
/// Called during `dc_maybe_network()` to make sure that `dc_all_work_done()`
/// returns false immediately after `dc_maybe_network()`.
pub(crate) async fn idle_interrupted(inbox: ConnectivityStore, oboxes: Vec<ConnectivityStore>) {
Expand All @@ -179,8 +206,7 @@ pub(crate) async fn idle_interrupted(inbox: ConnectivityStore, oboxes: Vec<Conne
// returns Connected. But after dc_maybe_network(), dc_get_connectivity() must not
// return Connected until DC is completely done with fetching folders; this also
// includes scan_folders() which happens on the inbox thread.
if *connectivity_lock == DetailedConnectivity::Connected
|| *connectivity_lock == DetailedConnectivity::Idle
if *connectivity_lock == DetailedConnectivity::Idle
|| *connectivity_lock == DetailedConnectivity::NotConfigured
{
*connectivity_lock = DetailedConnectivity::InterruptingIdle;
Expand All @@ -189,9 +215,7 @@ pub(crate) async fn idle_interrupted(inbox: ConnectivityStore, oboxes: Vec<Conne

for state in oboxes {
let mut connectivity_lock = state.0.lock().await;
if *connectivity_lock == DetailedConnectivity::Connected
|| *connectivity_lock == DetailedConnectivity::Idle
{
if *connectivity_lock == DetailedConnectivity::Idle {
*connectivity_lock = DetailedConnectivity::InterruptingIdle;
}
}
Expand Down

0 comments on commit fc136ef

Please sign in to comment.