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

add chatlist changed event #2850

Closed
wants to merge 1 commit into from
Closed

Conversation

Simon-Laux
Copy link
Member

adds an event that signals the ui to reload the chatlist. (when order changes or items are added/removed)
currently DC_EVENT_MSGS_CHANGED with chatid 0 is sometimes used for that, but it's missing at some places.
so in the long run I think it would make sense to deprecate DC_EVENT_MSGS_CHANGED and replace it with more distict/specific smaller events.

also desktop already emulates this event, so merging this can remove that logic from the desktop code.

@link2xt
Copy link
Collaborator

link2xt commented Nov 29, 2021

I'd rather have it named ChatlistChanged and use CHATLIST for consistency with already existing Chatlist type.

@link2xt
Copy link
Collaborator

link2xt commented Nov 29, 2021

Removing ChatId::new(0) everywhere would be very nice indeed.

Copy link
Collaborator

@Hocuri Hocuri left a comment

Choose a reason for hiding this comment

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

Android just registers for all events that could change the chatlist:

    eventCenter.addObserver(DcContext.DC_EVENT_CHAT_MODIFIED, this);
    eventCenter.addObserver(DcContext.DC_EVENT_CONTACTS_CHANGED, this);
    eventCenter.addObserver(DcContext.DC_EVENT_INCOMING_MSG, this);
    eventCenter.addObserver(DcContext.DC_EVENT_MSGS_CHANGED, this);
[Note: the next four events can only do changes within existing chats]
    eventCenter.addObserver(DcContext.DC_EVENT_MSGS_NOTICED, this);
    eventCenter.addObserver(DcContext.DC_EVENT_MSG_DELIVERED, this);
    eventCenter.addObserver(DcContext.DC_EVENT_MSG_FAILED, this);
    eventCenter.addObserver(DcContext.DC_EVENT_MSG_READ, this);

and then debounces chatlist reloading to 100ms (loadChatlistAsync()) (it also registers for CONNECTIVITY_CHANGED and SELFAVATAR_CHANGED but doesn't reload the chatlist for these events). And in the most cases, these events do mean that the chatlist has to be reloaded, so we don't do many unnecessary reloads.

I'm not totally sure about this PR, as it introduces a place where things can go wrong. Like, we could forget a place where we would have to emit ChatListChanged.

It looks like the first four events above can add/remove/reorder chats and require a full chatlist reload, while the last four events only do changes within existing chats.

Maybe you can use these events in Desktop, too?

If this works for you, too, we should probably point out in the documentation that these are the events that can be used for chatlist reloading.

@@ -301,10 +302,12 @@ impl ChatId {
Chattype::Group => {
info!(context, "Can't block groups yet, deleting the chat");
self.delete(context).await?;
context.emit_event(EventType::ChatListChanged);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Emitting the event is already done in delete()

Comment on lines +253 to +254
// This event is fired when the ordering or contents of the chatlist change,
// for changes in individual chat list items listen to `IncomingMsg`, `MsgsNoticed`, `MsgDelivered`, `MsgFailed`, `MsgRead` and `ChatModified`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does "or contents" mean that the event is also fired if something within a chatlist item changed (like, a read receipt was received for the last message you sent and now two green checkmarks are shown)

Copy link
Member Author

Choose a reason for hiding this comment

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

no, I'm open for suggestion on how to make this more clear

@r10s
Copy link
Member

r10s commented Nov 29, 2021

currently DC_EVENT_MSGS_CHANGED with chatid 0 is sometimes used for that

well, "sometimes" seems to be a bit understated here - in fact, DC_EVENT_MSGS_CHANGED is sent in all cases where the new event is sent - apart from contact blocking, and i would consider this as a bug.

so, in practice, DC_EVENT_MSGS_CHANGED is always followed by a DC_EVENT_CHATLIST_CHANGED. i can imagine that such an event could be useful for batch updates or so, but this is not part of this pr, and i also think, debouncing would be needed anyway.

so, not sure if the added complexity and the doubled events help much here.

and i agree, we should not merge that now, we are in release phase and there are already lots of moving targets and bug came in by revamped things, and this again has the potential to break things badly, esp. when we start to modify existing events.

also, as desktop handles the events as they are already ("also desktop already emulates this event"), it seems not be helpful for current issues or needed urgently.

@Simon-Laux
Copy link
Member Author

Android and iOS don't always display the chatlist.
Also I don't like the DC_EVENT_MSGS_CHANGED event as it can mean multiple things, so I'd suggest breaking it up into smaller events in the long run.

But sure maybe it's better for now to just put a DC_EVENT_MSGS_CHANGED(chatid=0) event where ever I placed a chatlist update and there currently isn't a DC_EVENT_MSGS_CHANGED yet.
and change its documentation that it's clearer that it should be used for that.

@r10s
Copy link
Member

r10s commented Nov 30, 2021

Android and iOS don't always display the chatlist.

but they could, this is not an issue with the current events when listening to the interesting ones as @Hocuri pointed out above.
in fact, the activity with the chatlist is never destroyed or so but is updated by events even if overlaid completely - that way, you can go back amazingly fast. for ios, we just changed to the same behavior, making things much more responsive.

But sure maybe it's better for now to just put a DC_EVENT_MSGS_CHANGED(chatid=0) event where ever I placed a chatlist update and there currently isn't a DC_EVENT_MSGS_CHANGED yet.
and change its documentation that it's clearer that it should be used for that.

+1 - these affects blocking contacts only.

i suggest to close this pr, do that in another pr and go for w30 or so then :) there are bigger fishes to get :)

i would not be up for revamping the event system currently.

@Simon-Laux Simon-Laux closed this Dec 1, 2021
@link2xt link2xt added this to Closed PRs and Issues in Project Resurrection via automation Dec 1, 2021
@r10s
Copy link
Member

r10s commented Mar 8, 2022

we thought that over recently, as already pointed out above, it makes sense to introduce a DC_EVENT_CHATLIST_CHANGED in addition to the existing events. however, that would make sense only of that new DC_EVENT_CHATLIST_CHANGED is debounced and only be emitted when all messages and updates arrived, avoiding UIs being blocked by lots of updates.

we could introduce that new event easily, and the chatlists would just listen to that event instead of DC_EVENT_MSGS_CHANGED/DC_EVENT_INCOMING_MSG (that part is also the gist of this pr).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Project Resurrection
Closed PRs and Issues
Development

Successfully merging this pull request may close these issues.

None yet

4 participants