feat: Don't ignore receive_imf_inner() errors, try adding partially downloaded message if so (#7196) #7222
Conversation
src/download.rs
Outdated
| }; | ||
|
|
||
| if !comment.is_empty() { | ||
| text = format!("[{}] {}", comment, text); |
There was a problem hiding this comment.
Can this be set as Part.error instead of modifying the text? We are trying to get rid of the square bracket errors, the last one removed in #7116 I think.
There was a problem hiding this comment.
It makes sense, did so, but in this case a message looks like a normal partially downloaded message, so i also added a red exclamation mark emoji in square brackets. At least it looks similar in my Desktop and Android, the emoji is the same. But not sure we should get rid of square bracket errors, users often report bugs in screenshots, so square bracket errors are good for bug reporting
iequidoo
force-pushed
the
iequidoo/dont-ignore-receive_imf_inner-err
branch
from
f03f0eb to
f15fb01
Compare
This comment was marked as resolved.
This comment was marked as resolved.
iequidoo
force-pushed
the
iequidoo/dont-ignore-receive_imf_inner-err
branch
9 times, most recently
from
0319363 to
d708477
Compare
src/config.rs
Outdated
| EncryptedDeviceToken, | ||
|
|
||
| /// Return an error from the code and upon the condition specified. For tests. | ||
| InjectFaultCfg, |
There was a problem hiding this comment.
As this is only used for making receive_imf fail, I think it is better to just have a boolean config that is documented to fail message reception instead of generalized future-proof config with undocumented values that is unlikely to be used for anything else soon.
There was a problem hiding this comment.
Replaced with FailOnReceivingFullMsg, but if we need other fault injections in the future, it should be replaced with smth like that InjectFaultCfg
| ) -> Result<()> { | ||
| let prefix = match error { | ||
| None => "", | ||
| Some(_) => "[❗] ", |
There was a problem hiding this comment.
UIs already display a red exclamation mark on the message bubble, no need to add it inside the message.
There was a problem hiding this comment.
They display it for OutFailed messages. If just error is set, they don't display anything. At least Desktop doesn't
| String::from_utf8_lossy(imf_raw), | ||
| ); | ||
| } | ||
| if partial.is_none() { |
There was a problem hiding this comment.
Why restrict this to non-partial downloads?
There was a problem hiding this comment.
This is necessary to test the new logic in imap: if a message fails to be added as fully downloaded, it should be added as partially downloaded. For partially downloaded messages the whole receive_imf logic is simpler, this way we w/a logic errors, but still fail on temporary I/O errors
…ownloaded message instead (#7196) Ignoring `receive_imf_inner()` errors, i.e. silently skipping messages on failures, leads to bugs never fixed. As for temporary I/O errors, ignoring them leads to lost messages, in this case it's better to bubble up the error and get the IMAP loop stuck. However if there's some logic error, it's better to show it to the user so that it's more likely reported, and continue receiving messages. To distinguish these cases, on error, try adding the message as partially downloaded with the error set to `msgs.error`, this way the user also can retry downloading the message to finally see it if the problem is fixed.
iequidoo
force-pushed
the
iequidoo/dont-ignore-receive_imf_inner-err
branch
from
d708477 to
44261c3
Compare
Close #7196
Tested manually in Desktop as well