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

Contact recently seen changes have missing events #5377

Closed
Simon-Laux opened this issue Mar 25, 2024 · 7 comments · Fixed by #5436
Closed

Contact recently seen changes have missing events #5377

Simon-Laux opened this issue Mar 25, 2024 · 7 comments · Fixed by #5436
Assignees
Labels
bug Something is not working

Comments

@Simon-Laux
Copy link
Member

Simon-Laux commented Mar 25, 2024

When a contact turns from "not recently seen" to "recently seen" there is always no ContactsChanged event.

When a contact turns from "recently seen" to "not recently seen" there is sometimes no ContactsChanged event.

Expected: Both cases should emit the ContactsChanged event

This cause a bug in the UI: deltachat/deltachat-desktop#3735 (on iOS the bug of missing events also exists, but is not as visible as the whole as the screen is refreshed each time the user switches screens, the same is likely true also for android, but I did not test there yet.)

@Simon-Laux Simon-Laux added the bug Something is not working label Mar 25, 2024
Simon-Laux added a commit that referenced this issue Mar 25, 2024
@Simon-Laux
Copy link
Member Author

@iequidoo iequidoo self-assigned this Apr 4, 2024
@iequidoo
Copy link
Collaborator

iequidoo commented Apr 5, 2024

It's not even documented that ContactsChanged must be emitted if so:

/// Contact(s) created, renamed, blocked or deleted.
///
/// @param data1 (int) If set, this is the contact_id of an added contact that should be selected.
ContactsChanged(Option<ContactId>),
So i think this is on purpose. But i agree that this should be fixed.

EDIT: I'm wrong, RecentlySeenLoop::run() actually tries to emit ContactsChanged, so the observed behaviour isn't on purpose.

@r10s
Copy link
Member

r10s commented Apr 5, 2024

[on iOS] the whole the screen is refreshed each time the user switches screens, the same is likely true also for android, but I did not test there yet.

this is not true.

android and ios also refresh the chatlist event based, not on switching screens.

you see this as there is zero delay when going back - i fact, the chatlist was there all the time, just fully covered

the change was made in 2021, see eg. deltachat/deltachat-ios#1374

however, whole chatlist is reloaded, not single chatlist items. that said, the redraw bug may still be in core. but the hint ios/android refresh behavior gives seems less clear :)

@Simon-Laux
Copy link
Member Author

Simon-Laux commented Apr 5, 2024

android and ios also refresh the chatlist event based, not on switching screens.

This is not primarily about the chatlist, this is about the indicator that is shown on the chat view/messagelist and the indicator on the profile view. To some extend also the chatlist, but the point is not that there are missing event listeners, the point is that there are cases where the event is missing, as far as I know there are listeners for contact changed event in all of the places I mentioned.

I was likely giving not enough information here leading to this misunderstanding.
Sorry for that, I'll try to be more verbose/clear in the future.

You can reproduce the missing events on iOS by:

  • set screen lock time to "never"
  • go to a chat
  • send a message to your other account (account B) (that is on a different device)
  • use account B to send a read receipt for that message, in my test the green dot only appeared when I left the chat and went back (to chatlist or profile view) - in my second test showing it worked, but maybe it's updating using a different event because I couldn't find a listener for it in the code, maybe that extra listener is missing in desktop, or my first chat was different, like receiving a read receipt in a different chat, I'll check later
  • after the dot is shown
    • (either it appeared for you if you didn't have the bug or sent a message instead of a read receipt, or you went to another screen and back) wait 11min (10min is the time it should be hidden after, you can recompile core to shorten the time, see my branch for the test)
  • See that the dot is still there and not hidden, though if you go into another screen and back it is updated.
  • don't forget to revert screen lock time if this isn't only a test device

anyways Ignoring my iOS testing which only the second half is still valid (dot does not vanish), as long as the test I wrote in https://github.com/deltachat/deltachat-core-rust/tree/simon/i5377-test-for-recently-seen is valid and fails we have the problem of missing events.

@r10s
Copy link
Member

r10s commented Apr 5, 2024

all fine, i did not doubt there is a bug :)
i just wanted to clarify the (repeatedly) wrong understanding how chatlist works on android/ios :)

sorry if i gave a wrong impression here. thanks a lot for taking care!

@adbenitez
Copy link
Member

about the change from "no green dot" to "green dot", there is no contact changed event because it is not strictly needed, android (and I guess iOS) use incoming message, msg read, etc events to update the state of the green dot as a contact can only change to online if you receive a message or read receipt from them,

about not receiving events when the user change from online to offline, that would be then a bug but it used to trigger the even just fine on Android last time I check

@Simon-Laux
Copy link
Member Author

Simon-Laux commented Apr 5, 2024

only change to online if you receive a message or read receipt from them

you don't get that info if the message/read receipt was received in a group chat if it is as you described. (because the events you mentioned have chat and message id, no contact id)
I would prefer dedicated events to such hacks that rely on other sometimes unrelated events.

iequidoo added a commit that referenced this issue Apr 6, 2024
…ange (#5377)

- Always emit `ContactsChanged` from `contact::update_last_seen()` if a contact was seen recently
  just for simplicity and symmetry with `RecentlySeenLoop::run()` which also may emit several events
  for single contact.
- Fix sleep time calculation in `RecentlySeenLoop::run()` -- `now` must be updated on every
  iteration, before the initial value was used every time which led to progressively long sleeps.
iequidoo added a commit that referenced this issue Apr 6, 2024
…ange (#5377)

- Always emit `ContactsChanged` from `contact::update_last_seen()` if a contact was seen recently
  just for simplicity and symmetry with `RecentlySeenLoop::run()` which also may emit several events
  for single contact.
- Fix sleep time calculation in `RecentlySeenLoop::run()` -- `now` must be updated on every
  iteration, before the initial value was used every time which led to progressively long sleeps.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working
Projects
4 participants