"ex" means "used to be, but no longer is".
"ext" means "extended", which is the intended meaning.
"Ext" is more common not only in the Rust ecosystem but in general.
We already had several people (myself included) asking
what "ex" is supposed to mean.
To reproduce this commit, search and replace `_ex(?!\w)` -> `_ext`.
But don't change `list_transports_ex` because it's public API.
This value is not used for any logic
except the disappearing messages timer rollback prevention.
It is unlikely that non-chat clients
participate in chats with disappearing messages,
and the case was not tested anyway.
Adds `assert_warn`, `assert_error` and `assert_warns_or_errors`
methods to `TestContext`, that let us assert that
a certain warning or error is logged during the test.
Also asserts test logs should not contain any other
errors or warnings.
Adjusts tests accordingly.
Signed-off-by: Jagoda Ślązak <jslazak@jslazak.com>
Devices no longer implicitely use the From address of sync messages
to determine their primary transport. Receivers have no concept of it and own devices
may disagree on which relay is reachable because of VPN or different networks:
1. Make setting a primary transport (`configured_addr`) a per-device non-synced operation.
2. Transport rows (add/remove/unpublish) keep syncing like before.
3. A device reelects a primary if a sync message unpublished/removed the current primary
if there is a better candidate.
4. `TransportsModified` event is emitted at most once on an incoming transport sync message.
Users will notice the change in that changing primary transport in settings/advanced/relays
will not synchronize to other devices anymore.
this PR adds support for reactions in broadcast channels.
> the idea of broadcast reactions is that they are sent as usual from
subscribers to owner. after some time, the owner broadcasts them to all
subscribers, who only get to see reaction+count, not who-reacted-what
the PR is quite large, but a good share are tests and otherwise many
things are straight forward.
review should be done by-file, not by-commit. to make review easier,
here is a high-level overview:
first a change in the existing internal `Reactions` object was required.
before this PR, `Reactions` had a "contact to reaction map" only, and
the "frequencies map", that are actually mainly needed for UI, were
calculated as needed. with this PR, the "frequencies map" is the field
that always exist, the "contact map" is only available on top of that.
moreover, this PR shifts that part to the core, it was unfortunately in
the bindings before.
with that preparation things done as follows:
1. reactions from broadcast channel subscriber (`Chattype::InBroadcast`)
to broadcast channel owner (`Chattype::OutBroadcast`) are sent as usual,
only change for that step was to allow sending them at all
2. the owner receives reactions and saves them to the existing
`reactions` table as usual. additionally, the changed message is
remembered in `reactions_need_broadcast` table
3. in the IMAP loop, when ~10 minutes have passed, and
`reactions_need_broadcast` contains entries, a single, hidden message
with accumulated reactions is sent. this message may contain reactions
to different messages. for each message, all known reactions are sent as
reaction+count.
4. subscriber receive that message and save the accumulated reactions in
`reactions_broadcasted`
6. `get_message_reactions` is adapted so that `frequencies` are set
independently of who-reacted-what (the old and only field).
who-reacted-what is called `by_contact` now. it is always set for
compatibility reasons, however, it is not exhaustive for subscribers.
in general, UI should work with frequencies, the API itself, however,
has not changed.
other tweaks:
- outgoing channels are muted on creation, and UI shall allow to
unmute/mute them as all other chats. reason is that reactions are
notified, but in many cases not of large interest. this is also what
telegram is doing
- to have an intermediate feedback when reacting, the local state should
include ones own reaction, even if it is not yet broadcasted. for that,
we modify `reactions_broadcasted` using `modify_frequencies()` as needed
when sending an reaction. there are still some situations where the
update may not include ones own reaction, in this case it is added
lately by `refine_frequencies()`, so that `get_message_reactions()`
always contain SELF.
(in a first implementation, we always increased SELF reaction in
refine_frequencies(), however, that was worse and led to SELF counted
twice once the owner sent broadcast)
<details>
<summary>wire format</summary>
wire format is a JSON in the `Broadcast-Reactions:` header.
additionally, `Content-Disposition: reaction` is set to not show the
hidden message on existing devices.
using a header also allows us to broadcast reactions with resent channel
messages (on joining) later.
```
{
"messages": [
{
"id": "12345678",
"reactions": [
{ "emoji": "👍", "count": 4 },
{ "emoji": "🎉", "count": 2 }
]
},
{
"id": "23456789",
"reactions": []
}
]
}
```
for `id`, the wire format needs to use `rfc724_mid` as `msg_id` are
local only.
</details>
### known issues
- if the channel owner uses multiple devices, broadcasted reaction
updates are sent from each device. the updates are not that big, so that
is probably not a big deal. if it turns out that this is an issue, we
can think about fixes in another PR. might be done by restarting our
10-minute-wait once we see an update from another device
- we cannot set contact_id for DC_EVENT_REACTIONS_CHANGED - but i doubt
it was ever used
### for another pr
- ~~add `Broadcast-Reactions:` header also for resent channel messages,
so that new subscriber do not only get the latest messages, but also
their reactions. for that, the `Broadcast-Reactions:` header can go to
the corresponding message, no need to send extra messages. we would need
to change the sending part to send all reactions for a given message. on
receiving part, we need to make sure, `receive_broadcast_reactions()` is
called when the message actually exist.~~
EDIT: subsequent PR for resending broadcast reactions at
https://github.com/chatmail/core/pull/8496
- add api to allow only a subset of reactions, fiter incoming reactions
before broadcasting
### misc.
ui pr: https://github.com/deltachat/deltachat-ios/pull/3225 and
https://github.com/deltachat/deltachat-android/pull/4560 , which both
were tested successfully with this core PR already. desktop is meant to
be done once this is merged
---------
Co-authored-by: l <link2xt@testrun.org>
- treat an Autocrypt header as a kind of Autocrypt-Gossip: addr=<self>
letting it participate in the existing cooperative key gossipping mechanics.
- speed up processing of incoming autocrypt-gossip headers by using
1 sql commit for N gossip headers instead of the current N commits.
Unifies internal naming of single/direct/1:1/normal chats
to match the naming used in the API.
Closes: #8368
Signed-off-by: Jagoda Ślązak <jslazak@jslazak.com>
---------
Signed-off-by: Jagoda Ślązak <jslazak@jslazak.com>
The `FolderMeaning` and the `target_folder()` function and related code
were needed for the message moving logic. The message moving logic isn't
needed anymore, so, this PR is a first step to remove it.
We only ever look at one folder by now (usually the Inbox folder), so,
any message moving logic isn't needed anymore because we will never
notice a message that is supposed to be moved.
The `target` column of the `imap` table is mostly the same as the
`folder` column now. Except when the message is supposed to be deleted,
then it's an empty string.
Remove is_chatmail flag, and rely on IMAP metadata
advertising the recipients limit (usually 1000),
falling back to 50 (or fewer in some exceptional cases)
just as before when is_chatmail was false.
Also, server metadata is now keyed per transport,
which in the future eases collecting ICE servers
from all relays (instead of just any first relay).
A new table `pending_reactions` is now used,
to store reactions that arrived before the referenced message.
When receiving a new message, pending_reactions is checked and any
reactions that arrived earlier are applied.
Fixes: #8367
Signed-off-by: Jagoda Ślązak <jslazak@jslazak.com>
An Iroh-Node-Addr message (realtime advertisement) may arrive earlier than the referenced webxdc in
case of multi-transport. By not creating a tombstone we still can receive it later on another
transport.
It's unclear whether this logic works, but I think we should at least
properly document why it exists (I already sent this text while figuring
out what the code does, so it was easy making this PR now)
Could add a XXX note that it's unsure whether this works
This way they also can be processed by `markseen_msgs()` resulting in self-MDNs which improves
multi-device synchronization. MDNs to contacts won't really be sent because silent group changes
messages are self-removal messages which don't request MDNs.
Otherwise it's not possible to see the full message text if it's too long in a received edit. On the
sender side let it still be text so that the message can be edited again.
Webxdc updates go to pre-message and when the message has no text,
the whole pre-message is trashed if it has a webxdc update.
The solution is not to trash pre-message in this case so preview is displayed.
I.e. don't fail `remove_contact_from_chat()` for such a contact because there may be a race
condition with a remote removal of the contact done without trace, but don't send a removal message
and sync message in this case and don't emit a `ChatModified` event.
If a contact is already a past member, we still send a removal message to the chat, this is safe and
protects from lost removal messages, so there's no need to complicate the code in this case.
receive_imf should not fail in this case,
but trash the post-message and updates.
The problem with previously used message::rfc724_mid_exists check
was that rfc724_mid_exists may return a trashed message.
Fix https://github.com/chatmail/core/issues/8195
Supersedes https://github.com/chatmail/core/pull/8217
The most interesting change is in `delete_expired_imap_messages()`
because there, messages are marked for deletion on single-device
chatmail profiles.
The logic in `delete_expired_imap_messages()` was wrong before, and
pre-messages were not deleted at all. This is fixed by also querying
`pre_rfc724_mid` in addition to `rfc724_mid` from the `msgs` table.
In order not to make the SQL statement too complex, I split it into two.
WRT tests:
- `test_immediate_autodelete()` tests the auto-deletion;
`test_imap_autodelete_fully_downloaded_msg()` tests that even for a
message that is split into pre- and post-message, both messages are
deleted.
- A lot of tests test that if bcc_self is on, messages are not
auto-deleted. E.g. `test_one_account_send_bcc_setting()` and
`test_markseen_message_and_mdn()` relies on the fact that messages stay
on the server when bcc_self is on.
- Unfortunately, it is not possible to test that messages are only
deleted for chatmail servers, because we don't have any tests that use a
non-chatmail server.
If we missed a message changing the primary transport, we shouldn't remove it when applying
a SyncTransports message, such a state doesn't look correct even if it's temporary.
Implementing `FromStr` and then calling `parse()` creates an
indirection, which is hard to follow for people who are not familiar
with Rust. @r10s recently voiced this problem when we were
pair-programming, and I agree.
We can decide what exactly to call the new function.
I didn't remove all `FromStr` implementations yet; `FromStr for
Fingerprint`, `FromStr for Params`, `FromStr for MozConfigTag` and
`FromStr for EphemeralTimer` are still left.
There's no check for `from_id` so another group member can delete the
message, but this can only happen in case of message reordering and the
problem already exists for usual messages, so we may ignore it and
overall a group represents a scope of trust.
Co-authored-by: Hocuri <hocuri@gmx.de>
rfc724_mid_exists() returns the message id not only when the message is fully downloaded, so we need
to check this additionally. Also improve test_receive_pre_message to check that receiving the
pre-message again doesn't affect the existing message.
I used some AI to draft a first version of this, and then reworked it.
This is one of multiple possibilities to fix
https://github.com/chatmail/core/issues/8041: For bots, the IncomingMsg
event is not emitted when the pre-message arrives, only when the
post-message arrives. Also, post-messages are downloaded immediately,
not after all the small messages are downloaded.
The `get_next_msgs()` API is deprecated. Instead, bots need to listen to
the IncomingMsg event in order to be notified about new events. Is this
acceptable for bots?
THE PROBLEM THAT WAS SOLVED BY THIS:
With pre-messages, it's hard for bots to wait for the message to be fully downloaded and then process it.
Up until now, bots used get_next_msgs() to query the unprocessed messages, then set last_msg_id after processing a message to that they won't process it again.
But this will now also return messages that were not fully downloaded.
ALTERNATIVES:
In the following, I will explain
the alternatives, and for why it's not so easy to just make the
`get_next_msgs()` API work. If it's not understandable, I'm happy to
elaborate more.
Core can't just completely ignore the pre-message for two reasons:
- If a post-message containing a Webxdc arrives later, and some webxdc updates arrive in the meantime, then these updates will be lost.
- The post-message doesn't contain the text (reasoning was to avoid duplicate text for people who didn't upgrade yet during the 2.43.0 rollout)
There are multiple solutions:
- Add the message as hidden in the database when the pre-message arrives.
- When the post-message arrives and we want to make it available for bots, we need to update the msg_id because of how the `get_next_msgs()` API works. This means that we need to update all webxdc updates that reference this msg_id.
- Alternatively, we could make webxdc's reference the rfc724_mid instead of the msg_id, so that we don't need stable msg_ids anymore
- Alternatively, we could deprecate `get_next_msgs()`, and ask bots to use plain events for message processing again. It's not that bad; worst case, the bot crashes and then forgets to react to some messages, but the user will just try again. And if some message makes the bot crash, then it might actually be good not to try and process it again.
- Store the pre-message text and `PostMsgMetadata` (or alternatively, the whole mime) in a new database table. Wait with processing it until the post-message arrives.
Additionally, the logic that small messages are downloaded before post-messages should be disabled for bots, in order to prevent reordering.
This avoids creating 1:1 chat on a second device when joining a channel.
Now when joining a channel there may be no 1:1 chat with the inviter
when the channel is created. In this case we still create the channel
as unblocked even if 1:1 chat would be a contact request
because joining the channel is an explicit action
and it is not possible to add someone who did not scan a QR
to the channel manually.
Fix https://github.com/chatmail/core/issues/8042
The problem was that after receiving the bcc_self'ed pre-message in
`receive_imf`, the logic there only looked for a pending
`smtp`-table-entry that matches the rfc724_mid, and if there was none
then it thought "Great, apparently the message is fully sent out, we can
mark it as delivered!".
But with pre-messages, the same message can have two `smtp` entries (one
for the pre-message and one for the post-message), and the message
should only be marked as delivered once both of them are sent out.
Now, I changed the logic to look for all entries with the same msg_id.
This is actually the same SQL query used in smtp.rs, so, I extracted it
into a new function; feel free to suggest a better name for it.
I tested on Android that it now works fine.
I'll add a test in a follow-up PR.
There are a lot of other problems with sending large files, though:
- The pre-message is sent before the post-message, so that for the
receiver it looks as if the message arrived, but stays in
"downloading..." forever
- There is quite a time delay between clicking on "Send" and the
outgoing message appearing in the chat
- The message shortly gets a letter icon right after it is sent
- I'm wondering if there is a way to give feedback to the user
immediately if the message is too big
- It's unclear when exactly we want to send read receipts
I'll open a follow-up issue for these.
WofWca
Jagoda Estera Ślązak
link2xt
holger krekel
B. Petersen
Hocuri
iequidoo
iequidooandHocuri