Make sure set_last_error is called for .log_err()

[flub]

We have a common .log_err() trait method which is very prevalent and
useful in the FFI bindings. However this was not hooked up with the
set_last_error/get_last_error mechanism, which the error! macro was
hooked up with.

This hooks correctly hooks up the two and makes sure to log all errors
around backup provider/retrieval.

Closes #4186, #4178

[r10s]

nice, lgtm

[Hocuri]

log_err() sends a Warning, like warn!(), so either warn!() should also call set_last_error() or log_err() should not call it.

Maybe we need to rename log_err(), it's confusing that it generates a Warning instead of an Error.

[flub]

log_err() sends a Warning, like warn!(), so either warn!() should also call set_last_error() or log_err() should not call it.

Maybe we need to rename log_err(), it's confusing that it generates a Warning instead of an Error.

LogExt::log_err() logs the Result::Err variant and that's why it is named this way and it's named correctly. It's name does not refer to the logging level used.

I think you could argue that LogExt::log_err() should use the error level. I can't find anything in the commit messages justifying the warn level. However I'd like to treat that as orthogonal to this fix since this really fixes an issue.

[Hocuri]

I agree, let's not talk about renaming log_err() here. So:

log_err() sends a Warning, like warn!(), so either warn!() should also call set_last_error() or log_err() should not call it.

I'd like to add: We spread log_err() over the codebase without thinking a lot about it, so if we let it change the 'last error', then this might accidentally overwrite the actual error.

this really fixes an issue.

So, do you propose to accept the inconsistency in order to fix #4186 and #4178? We can do this if you say it's necessary. My gut feeling would say "just use error!() then", but then again, I haven't looked into these issues a lot.

[flub]

I agree, let's not talk about renaming log_err() here. So:

log_err() sends a Warning, like warn!(), so either warn!() should also call set_last_error() or log_err() should not call it.

I'd like to add: We spread log_err() over the codebase without thinking a lot about it, so if we let it change the 'last error', then this might accidentally overwrite the actual error.

I don't think this is really a worry: .log_err()'s most important use is in the FFI and similary .get_last_error() is an FFI API (I might even argue it should have been implemented entirely there, but maybe that was tricky for implementation reasons). So our rust API's return good Result::Err variants, the ffi crate uses those errors and calls .log_err() on them and since they're the last to call set_last_error() it also is the last error as it should be reported.

this really fixes an issue.

So, do you propose to accept the inconsistency in order to fix #4186 and #4178? We can do this if you say it's necessary. My gut feeling would say "just use error!() then", but then again, I haven't looked into these issues a lot.

I don't think this is inconsistent, I think this is working as intended. The Error event level is for showing toasts to the user (thanks for reminding me of this on IRC - I couldn't quite remember this), which anyway we rather avoid. get_last_error is for the FFI and consistently using .log_err() in the FFI is the right way to achieve this.

[Hocuri]

get_last_error is for the FFI and consistently using .log_err() in the FFI is the right way to achieve this.

.log_err() is not only used in the FFI though. And this PR also changes the behavior of ok_or_log() and ok_or_log_msg() - I counted 25 usages outside of the FFI.

These functions are used as a replacement for warn!() - up to now, rewriting this:
https://github.com/deltachat/deltachat-core-rust/blob/57445eedb1dd724d1462c41a911dbb407ec8aae6/src/imex.rs#L769-L770
as:

.ok_or_log_msg(context, "Vacuum failed, exporting anyway");

would have been a pure refactoring. With this PR, it will be a semantical change.

Maybe we should also make warn!() call set_last_error()?

[iequidoo]

Maybe we should also make warn!() call set_last_error()?

ok_or_log() looks as a tool for logging non-critical errors (i.e. warnings), so i think better to pass from it to log_err_inner() an additional parameter telling that no set_last_err() call is needed

[r10s]

i think, this PR is good enough - it adds a single line and seems to solve all error reporting issues we have with backup sending.

sure, there may be more cases where last error should be set, however, most cases that cause actual, real problems seem to be covered.

and i agree, we should do refactor the error and warning handling at some point, but, as this easily comes with new bugs or annoyances, we should not do that just now, shortly before a release.

offtopic: historical background

one cause of the issues is that core and UIs had/have different views what an error is:

• UI treated/treats all error reported by DC_EVENT_ERROR as sth. that "need to be shown to the user" that is shown as a "toast"
• core, as usual for libraries, sees an errors often just as as a "line in the log", an intermediate, not necessarily final state

to not show tons of toasts to the user, core often uses warnings instead of errors.

the, maybe wrong, decision to just show all reported errors to the user, was made at the very beginning to push the first UI forward fast. meanwhile (since 2 years or so), android and ios UI do no longer report DC_EVENT_ERROR to the user - instead, in case a concrete function "fails" and failure "is expected", a "real dialog" is shown, which is better UX (but also more work).

but we never changed that "officially" and also desktop was not adapted as there was no real need as both approaches kind of work.

sending or connection failures, the most frequent errors, however, are reported differently and are not part of this issue at all.

[Hocuri]

as this easily comes with new bugs or annoyances, we should not do that just now, shortly before a release.

This PR here is quite a big semantical change, too: It not only changes the behavior of backup sending, but of everything that uses dc_get_last_error(): normal import/export, when send_msg() returns 0, creating a QR code, ... (grep getLastError in the Android code for more examples). If we merge it, we should test that all of those still work, because now a mere warning that is reported using ok_or_log() can overwrite the error that was actually meant to be shown to the user.

Probably it's fine, but still, I would prefer if we e.g. introduced a new method for this, called log_error or so, that's only used in the FFI. Then this new method could set the last error, and other code would stay unaffected. This way, we would change the semantics only in the places where we know it brings us an advantage.

That being said: I'm OK with my opinion being ignored here if we need to merge this PR because it's urgent (and error-reporting is on a best-effort basis, anyway, so while I'm afraid that this PR might introduce bugs now or make it easier to accidentally introduce bugs in the future while refactoring, they wouldn't be very severe bugs).

Whoever merges this will need to use "Merge without waiting for requirements to be met (bypass branch protections)", anyway, because CI fails.

[flub]

I made an alternative: #4195