Skip to content

Avoid deleting sent messages on non room events #4960

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.

Sign up for GitHub

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

Merged
bmarty merged 5 commits into from
Jan 17, 2022
+15 −10
Merged

Avoid deleting sent messages on non room events #4960

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3656896
Avoid deleting sent messages on non room events
ariskotsomitopoulos Jan 17, 2022
493374c
Add changelog
ariskotsomitopoulos Jan 17, 2022
01ade64
fix ktlint format
ariskotsomitopoulos Jan 17, 2022
879c4ff
Improve logic
ariskotsomitopoulos Jan 17, 2022
52348e3
ktlint fixes
ariskotsomitopoulos Jan 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/4960.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improves local echo blinking when non room events received
3 changes: 3 additions & 0 deletions ...rc/main/java/org/matrix/android/sdk/internal/session/sync/handler/room/RoomSyncHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@bmarty bmarty Jan 17, 2022

Copy link
Copy Markdown
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?

@ariskotsomitopoulos ariskotsomitopoulos Jan 17, 2022

Copy link
Copy Markdown
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?

@bmarty bmarty Jan 17, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

@ariskotsomitopoulos ariskotsomitopoulos Jan 17, 2022

Copy link
Copy Markdown
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

@ganfra ganfra Jan 17, 2022

Copy link
Copy Markdown
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?

@ariskotsomitopoulos ariskotsomitopoulos Jan 17, 2022

Copy link
Copy Markdown
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.

@ariskotsomitopoulos ariskotsomitopoulos Jan 17, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@ganfra ganfra Jan 17, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK

rooms.forEach { roomEntity ->
roomEntity.sendingTimelineEvents.filter { timelineEvent ->
timelineEvent.root?.sendState == SendState.SENT
Expand Down