Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: add ChatListChanged and ChatListItemChanged events #4476

Merged
merged 51 commits into from
Apr 15, 2024

Conversation

Simon-Laux
Copy link
Member

@Simon-Laux Simon-Laux commented Jun 15, 2023

Todo:

  • Make sure all cases have events
  • Make sure all new cases will have events (this todo can be converted to an issue)
  • make a desktop pr using this (chat list update event deltachat-desktop#3268)
  • add debouncing of those events in core (draft in chatlistitem UI events debouncing #5171) let's do this in a dedicated pr, if we need it at all
  • Test if all cases work (manually with desktop once this pr is ready to be merged)
  • Make a list for testers with all test-cases (I already have a list, but it's not readable by testers) - I think it's not needed anymore - because of the automated test
  • Make tests for the majority of cases
  • rebase and remove openrpc.json from git history
  • rename DC_EVENT_UI_CHATLIST_CHANGED to something like DC_EVENT_UI_CHATLIST_ORDER_CHANGED
    • clarified docs instead

missing optional tests:

  • Chatlist correctly updated after AEAP (on what devices?) (maybe not needed because contact name change is already tested and AEAP is still experimental)
  • change protection (1:1 chat gets guranteed encryption) (maybe triggered by new message anyway?)
  • securejoin dm chat with contact (group join is already tested)

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

@r10s
Copy link
Member

r10s commented Jun 21, 2023

ftr, this pr picks up suggestions and discussions from previously closed #2850

@Simon-Laux Simon-Laux self-assigned this Sep 28, 2023
@link2xt link2xt deleted the branch main October 25, 2023 21:22
@link2xt link2xt closed this Oct 25, 2023
@link2xt link2xt reopened this Oct 25, 2023
@Simon-Laux Simon-Laux changed the base branch from master to main October 26, 2023 11:07
@Simon-Laux Simon-Laux force-pushed the simon/UIChatListItemChangedEvent2 branch from 1f5cdc4 to 7483aa5 Compare January 11, 2024 01:49
@Simon-Laux Simon-Laux force-pushed the simon/UIChatListItemChangedEvent2 branch from 2c37341 to bdfd970 Compare January 23, 2024 02:05
@link2xt link2xt force-pushed the main branch 2 times, most recently from 1abb12e to 2af9ff1 Compare March 4, 2024 21:10
@Simon-Laux Simon-Laux force-pushed the simon/UIChatListItemChangedEvent2 branch from bdfd970 to 54ab9c3 Compare March 17, 2024 20:11
@@ -301,8 +301,8 @@ pub enum EventType {

/// Inform UI that a single chat list item changed and needs to be rerendered
/// If `chat_id` is set to None, then all currently visible chats need to be rerendered, and all not-visible items need to be cleared from cache if the UI has a cache.
UIChatListItemChanged {
/// The id of the changed chat or None for all chats.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the documentation for None was removed?

src/contact.rs Outdated

if updated_name {
// update the chats the contact that changed their name is part of
// (treefit): could make sense to only update chats where the last message is from the contact, but the db query for that is more expensive
Copy link
Collaborator

@iequidoo iequidoo Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this loop can be expensive too, we don't know how many chats we have with the contact. Though even if we take only chats with the last message from this contact, we don't know. I'd avoid any loops and just emit an event with chat_id: None. Better to let the app decide which chats to update (those which are on the screen currently) and rate-limit these updates. Another solution is to add LIMIT to the SELECT statement and if we hit the limit, emit an event with chat_id: None, otherwise emit separate events for chats

src/events/ui_events.rs Outdated Show resolved Hide resolved
src/events/ui_events.rs Outdated Show resolved Hide resolved
@r10s
Copy link
Member

r10s commented Mar 18, 2024

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

@Simon-Laux
Copy link
Member Author

Simon-Laux commented Mar 18, 2024

  • 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

Copy link
Collaborator

@link2xt link2xt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what UIEventsState does and why is it needed.

If it is for debouncing, I suggest to split this PR into PR that simply adds events and another PR on top of this that adds debouncing for these two events.

Last time we discussed debouncing for UI events my proposal was to remember highest Event.id of each event we want to debounce inside the EventEmitter (e.g. remember highest id of UIChatListItemChanged { chat_id: ChatId(5) }, UIChatListItemChanged { chat_id: ChatId(10) }, UIChatListChanged separately. Then when we try to retrieve event from EventEmitter we check if the item we just retrieved from the queue is not the newest one pushed (i.e. there is already the same item inside the queue that will be retrieved later) and get the next item from the queue if this is the case.

deltachat-ffi/deltachat.h Outdated Show resolved Hide resolved
#define DC_EVENT_UI_CHATLIST_CHANGED 2300

/**
* Inform UI that all or a single chat list item changed and needs to be rerendered
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chat_id = 0 means that multiple chatlist item have changed, not "all"?

Copy link
Member Author

@Simon-Laux Simon-Laux Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should invalidate cache and reload all items that are visible or where in cache in the ui - not sure how to word that

deltachat-jsonrpc/src/api/types/events.rs Outdated Show resolved Hide resolved
deltachat-jsonrpc/src/api/types/events.rs Outdated Show resolved Hide resolved
src/events/payload.rs Outdated Show resolved Hide resolved
src/events/ui_events.rs Outdated Show resolved Hide resolved
src/events/ui_events.rs Outdated Show resolved Hide resolved
src/events/ui_events.rs Outdated Show resolved Hide resolved
src/events/ui_events.rs Outdated Show resolved Hide resolved
src/events/ui_events.rs Outdated Show resolved Hide resolved
src/contact.rs Outdated
}

/// Get all chats the contact is part of
pub async fn get_chats_with_contact(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to keep this method, even though we probably won't use it anymore in this pr:

  • it't to expensive to run in emitting events compared to telling the ui just to reload the whole list
  • on the other hand this method could be useful on it's own maybe?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But currently at least in DC Desktop you can click on the contact and see the "Shared Chats" list. How does it work w/o such a method?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not checked, but current UIs probably use dc_get_chatlist with query_id set to contact ID.

src/contact.rs Outdated
}

/// Get all chats the contact is part of
pub async fn get_chats_with_contact(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But currently at least in DC Desktop you can click on the contact and see the "Shared Chats" list. How does it work w/o such a method?

src/contact.rs Outdated
@@ -1603,7 +1603,7 @@ pub(crate) async fn set_profile_image(
if changed {
contact.update_param(context).await?;
context.emit_event(EventType::ContactsChanged(Some(contact_id)));
ui_events::emit_chatlist_item_changed_for_contacts_dm_chat(context, contact_id);
ui_events::emit_chatlist_item_changed_for_contact_chat(context, contact_id).await;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it not sufficient for the app to know the contact ID? We better add contact_id + chat_type (1:1, group, etc.) to struct Summary so that the app can iterate over summaries and quickly find the chatlist item changed w/o doing extra db queries.

Copy link
Member Author

@Simon-Laux Simon-Laux Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is about the dm/1:1 chat with that contact (profile image and recently online), the db query for that should be cheap and saves us a round trip to core. But sure we can also discuss creating new dedicated events for it, my idea here is to have some small amount of chatlist events that the ui listens for. (with the motto: if you listen for all events with chatlist in the name you are set)

The name update is the expensive query, I already changed that to reload all cached items.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this db query is cheap, but if we just add all necessary info to struct Summary, we don't need any additional events at all. So, having EventType::ContactsChanged is sufficient. Also we don't need an extra round trip to the core because, i guess, the app already has the list of all summaries (it should be built to display the chat list) and can iterate over them (actually only over currently visible ones) to find out which items has changed.

Copy link
Member Author

@Simon-Laux Simon-Laux Mar 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having all these different events to listen to is a mess, my goal here is to move the concern to core to give out events when the (granted virtual) resource of chatlist changes (be it order or item content).

I also plan to add tests for all the cases to core.

So in my mind we could also do this (I don't want to discuss exact naming at this step):

  • for avatar and recently online changes
    • add contact id to chat list items of DM chats
    • add ChatlistContactDirectMessagesChatChanged(contactId) event
    • -> currently in this case core gets the dm chat id and just updates that without the ui needing to do anything extra for this case
  • for contact name (and AEAP as it can change email and thus display name for contacts without a name)
    • add contact ids for all summaries
    • add ChatlistContactNameChanged(contactId) event
    • -> currently core tells ui to reload all cached/visible chatlistitems, using this event it could be slightly more efficient, but unclear if it's worth the extra ui effort as I'm sure someone will want to use the invalidate/reload all items escape hatch in the future for other things anyways.

I am against reusing EventType::ContactsChanged as those events are not precise enough and you have many of them to handle, so a new feature comes around and it needs a new event to be listened to in the UI, the point of this pr/effort is to make the ui part much easier that you exactly know which few events you need to handle to have handled all cases for when the chatlist updates, ideally as precise as (reasonable in db time constrains) possible, that the lazy virtual list in desktop can be partially updated as needed and not reload all chats when one of them changes.

deltachat-ffi/src/lib.rs Outdated Show resolved Hide resolved
@Simon-Laux Simon-Laux changed the title feat: add UIChatListChanged and UIChatListItemChanged events feat: add ChatListChanged and ChatListItemChanged events Mar 25, 2024
@Simon-Laux Simon-Laux force-pushed the simon/UIChatListItemChangedEvent2 branch from 688233d to 487db7d Compare April 2, 2024 19:40
@Simon-Laux Simon-Laux marked this pull request as ready for review April 2, 2024 20:14
@Simon-Laux Simon-Laux force-pushed the simon/UIChatListItemChangedEvent2 branch from 30ac692 to 061a63e Compare April 4, 2024 17:45
@link2xt
Copy link
Collaborator

link2xt commented Apr 5, 2024

Backup transfer in tests fail because of #5431

@link2xt
Copy link
Collaborator

link2xt commented Apr 7, 2024

@Simon-Laux Could you rebase this on top of main? Backup transfer should be fixed now that #5439 is merged.

@Simon-Laux Simon-Laux force-pushed the simon/UIChatListItemChangedEvent2 branch from 5e94feb to 851a622 Compare April 11, 2024 21:59
(te test was for groups, the chat is created later in that case)
src/mimeparser.rs Outdated Show resolved Hide resolved
Co-authored-by: link2xt <link2xt@testrun.org>
@link2xt link2xt enabled auto-merge (squash) April 15, 2024 22:20
@link2xt link2xt disabled auto-merge April 15, 2024 22:20
@link2xt link2xt enabled auto-merge (squash) April 15, 2024 22:21
@link2xt link2xt changed the title feat: add ChatListChanged and ChatListItemChanged events api: add ChatListChanged and ChatListItemChanged events Apr 15, 2024
@link2xt link2xt merged commit f9465f7 into main Apr 15, 2024
32 of 38 checks passed
@link2xt link2xt deleted the simon/UIChatListItemChangedEvent2 branch April 15, 2024 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants