deltachat-core-rust
deltachat-core-rust copied to clipboard
feat: add `UIChatListChanged` and `UIChatListItemChanged` events
Todo:
- [ ] Make sure all cases have events
- [X] make a desktop pr using this (https://github.com/deltachat/deltachat-desktop/pull/3268)
- [ ] add debouncing of those events in core (draft in https://github.com/deltachat/deltachat-core-rust/pull/5171)
- [ ] Test if all cases work
- [ ] Make a list for testers with all test-cases (I already have a list, but it's not readable by testers)
Why now:
- This makes desktop code simpler (only 3 cases and 2 events to listen to)
- This should fix issues that currently are there or are sometimes hidden by a strange workaround in desktop
- going actively over all cases will help us to fix chatlist bugs where it is not updated when it should be
ftr, this pr picks up suggestions and discussions from previously closed https://github.com/deltachat/deltachat-core-rust/pull/2850
i know, this is still a draft, just some thought:
up to now, events emitted by our event system are "cheap", meaning, no extra database access or calculation is needed for emitting events. the events are just emitting what is known anyways.
this PR partly introduces "expensive" events that need additional database calls, currently, only when a contact name is changed, however DC_EVENT_UI_CHATLIST_ITEM_CHANGED will probably be needed in more cases where eg. contacts-table or other related tables are changed. this is comparable expensive to calculate, leading to potentially many events. not sure if that is helping if on the other hand debouncing in core is needed for desktop.
as there is already a DC_EVENT_UI_CHATLIST_CHANGED maybe it is better to just fire that on contact-db changes, imu UI is responsible for reloading the list then and needs to deal with that anyways, that should be just fine in case joined data are updated (DC_EVENT_UI_CHATLIST_ITEM_CHANGED can still be fired in cases where the information is already there and "cheap")
if we stay with "expensive" events, we should make the code optional by a compiler switch or so; there is no need to slow down iOS, Android (and probably UbuntuTouch) that do not need these events
- true
emit_chatlist_items_changed_for_contact
(update name or aeap) is quite expensive and could just be refresh the whole list (not order, just items) -
emit_chatlist_item_changed_for_contacts_dm_chat
(profile image changed, recently seen changed) on the other hand should stay IMO, as it is less expensive, If we need it we could also cache dm chat if it exists in the contacts table to make accessing it even faster.
also there is currently the question of how tho integrate it:
A. make a separate loop for ui events that deduplicates until the next event is requested from it
B. A
but integrated in existing event loop (I would need link2xt's help for that, as I'm not deep enough into the future/async stuff)
C. do not dedup the events as it is fast enough, maybe add a dedicated event for 1:1/dm chat update to also save the db query and move the burden of that to the frontend
D. some combination/mix of other options or your idea?
Currently this pr has the following events/cases:
- Order changed or what chatlist items are displayed (list of chat ids, not chat list item content):
DC_EVENT_UI_CHATLIST_ITEM_CHANGED
- content of one chatlistitem should be reloaded:
DC_EVENT_UI_CHATLIST_ITEM_CHANGED (chat_id: Some(id))
- content of all (cached) chatlistitems should be reloaded:
DC_EVENT_UI_CHATLIST_ITEM_CHANGED (chat_id: None)
maybe we should more events:
- contact changed so update the dm chat for the following contact id, but the ui would just also call core to convert contact id to dm chat id
- reload all chats where contact name is in last message (ui could look in cache for this, when we add the contact id of the last message to the jsonrpc chatlistitem), but we can also just invalidate the cache and say all items should be reloaded
Backup transfer in tests fail because of https://github.com/deltachat/deltachat-core-rust/issues/5431
@Simon-Laux Could you rebase this on top of main? Backup transfer should be fixed now that #5439 is merged.