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

Avoid deleting sent messages on non room events #4960

Merged
merged 5 commits into from
Jan 17, 2022

Conversation

ariskotsomitopoulos
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos commented Jan 17, 2022

Following fix #4928 this is an improvement that prevents SENT messages to be deleted when there are no room events received and prevents blinking

@@ -514,6 +514,9 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle
* we clear all SENT events, and we are sure that we will receive it from /sync or pagination
*/
private fun fixStuckLocalEcho(rooms: List<RoomEntity>) {
// when there are not room events, there is no need to delete SENT messages
// this might be useful for events like typing etc
if (rooms.isNullOrEmpty()) return
Copy link
Member

Choose a reason for hiding this comment

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

If there are only typing events, I am not sure that rooms will be empty.
Also we can have the case where we have new Events from other users, so we can still have a blink. @ganfra WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree for the first part, I will update it a bit to check timeline events and not ephemeral etc. Now regarding the other we can fix this by removing them when we found an event from the same user! Do you think that will work?

Copy link
Member

@bmarty bmarty Jan 17, 2022

Choose a reason for hiding this comment

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

removing them when we found an event from the same user

Yes, maybe. Would be nice to have @ganfra feedback as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also keep in mind, that on iOS they delete it much earlier (on SENT), so I think we improved the experience by delaying it until those constraints

Copy link
Member

Choose a reason for hiding this comment

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

I had flickering issues on friday sending reply and quote events (not sure the type is relevant).
Also, maybe just cleaning SENT events after a paused sync (look at SyncThread) would be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, the flickering will mainly stop with that fix, while now we only delete local echos on specific cases. Maybe, pausing on syncThread will work, but not sure what will happened with that approach when there is no internet connection and the paused sync is called.

Copy link
Contributor Author

@ariskotsomitopoulos ariskotsomitopoulos Jan 17, 2022

Choose a reason for hiding this comment

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

I would suggest to merge it while it will for sure eliminate a lot of blinking, and if we observe more, we follow a different approach like the one you suggest

Copy link
Member

Choose a reason for hiding this comment

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

OK

@github-actions
Copy link

github-actions bot commented Jan 17, 2022

Unit Test Results

  66 files  ±0    66 suites  ±0   53s ⏱️ -1s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 52348e3. ± Comparison against base commit f025554.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jan 17, 2022

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    passed="21" failures="0" errors="0" skipped="2"
  • [org.matrix.android.sdk.account]
    passed="5" failures="0" errors="0" skipped="2"
  • [org.matrix.android.sdk.internal]
    passed="158" failures="1" errors="0" skipped="38"
  • [org.matrix.android.sdk.ordering]
    passed="16" failures="0" errors="0" skipped="0"
  • [org.matrix.android.sdk.PermalinkParserTest]
    passed="1" failures="1" errors="0" skipped="0"

@bmarty bmarty merged commit eafb76b into develop Jan 17, 2022
@bmarty bmarty deleted the feature/aris/improve_local_echo_stuck_fix branch January 17, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants