Feature/fga/timeline chunks rework #4405

ganfra · 2021-11-03T18:09:45Z

This PR is a first step in the refactoring of the timeline.
It should also fix #4280 (not reproduced)

The main changes are:

Previous live chunk are now deleted when we have a gap.
We create small chunks and link them with previous/next instead of merging
Each chunk listen only for his events
Creation of smaller classes.

This is still not ideal, but it will be easier to evolve from there after hopefully.

…rlapping events

…meline event

github-actions · 2021-11-03T18:49:08Z

Unit Test Results

  66 files ±0   66 suites ±0 59s ⏱️ +7s
135 tests ±0 135 ✔️ ±0 0 💤 ±0 0 ❌ ±0
418 runs ±0 418 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 9121585. ± Comparison against base commit ff2e7d8.

♻️ This comment has been updated with latest results.

…hod on the timeline)

…akes some time uselessly)

ouchadam · 2021-12-14T11:49:42Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/DatabaseCleaner.kt

            }
        }
    }

+    /*
    private fun cleanUp(realm: Realm, threshold: Long) {


is this cleanup not needed anymore? if so we could probably delete the class as it's only logging the number of rooms (or make it debug~ only)

ouchadam · 2021-12-14T12:51:56Z

.../src/main/java/org/matrix/android/sdk/internal/session/room/timeline/LoadTimelineStrategy.kt

+        private val mode: Mode,
+        private val dependencies: Dependencies) {
+
+    sealed class Mode {


nit: this could be a sealed interface

ouchadam · 2021-12-14T12:55:41Z

...c/main/java/org/matrix/android/sdk/internal/session/room/timeline/SendingEventsDataSource.kt

+    fun buildSendingEvents(): List<TimelineEvent>
+}
+
+internal class RealmSendingEventsDataSource(


💯 for extracting this to its own smaller class

ouchadam · 2021-12-14T12:58:23Z

...android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/TimelineChunk.kt

+            LoadMoreResult.FAILURE
+        }
+        return if (loadMoreResult == LoadMoreResult.SUCCESS) {
+            latch?.await()


might be safer to use await with a timeout or does this logic cancel when exiting the timeline?

The scope is closed so this should cancel when exiting

ouchadam

👍 no blockers from my side although I'm very new to this area and don't know a lot of the history/context

didn't notice any issues with a local smoke test

bmarty

Amazing work!
Thanks @ouchadam for the smoke test, happy to merge this PR fast to develop once the changelog files are added.

bmarty · 2021-12-15T20:34:22Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/timeline/Timeline.kt

+        /**
+         * Called when the pagination state has changed in one direction
+         */
+        fun onStateUpdated(direction: Direction, state: PaginationState) = Unit


Can you advertise the API break using a changelog file 4405.removal please? Another 4405.feature can simply tell that the timeline management has been reworked.

Why removal?

Because according to https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog:

.removal: Signifying a deprecation or removal of public API. Can be used to notifying about API change in the Matrix SDK

and https://github.com/vector-im/element-android/blob/develop/towncrier.toml#L25

bmarty · 2021-12-15T20:36:40Z

...d/src/main/java/org/matrix/android/sdk/internal/database/helper/TimelineEventEntityHelper.kt

@@ -28,3 +28,13 @@ internal fun TimelineEventEntity.Companion.nextId(realm: Realm): Long {
        currentIdNum.toLong() + 1
    }
 }
+
+internal fun TimelineEventEntity.isMoreRecentThan(eventToCheck: TimelineEventEntity): Boolean {


Maybe rename eventToCheck to other

bmarty · 2021-12-15T20:37:40Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/model/ChunkEntity.kt

                                var stateEvents: RealmList<EventEntity> = RealmList(),
                                var timelineEvents: RealmList<TimelineEventEntity> = RealmList(),
-                                var numberOfTimelineEvents: Long = 0,
+                                // var numberOfTimelineEvents: Long = 0,


Just delete the commented out code? Or will it be uncommented in the future?

Will delete, thanks

bmarty · 2021-12-15T20:38:17Z

...-android/src/main/java/org/matrix/android/sdk/internal/database/model/TimelineEventEntity.kt

@@ -46,7 +46,5 @@ internal fun TimelineEventEntity.deleteOnCascade(canDeleteRoot: Boolean) {
    if (canDeleteRoot) {
        root?.deleteFromRealm()
    }
-    annotations?.deleteOnCascade()
-    readReceipts?.deleteOnCascade()


Not needed anymore? Can you explain?

Annotations and read receipts persistence is supposed to be independent
from timeline. This is what caused reactions to be lost during gap

So the table for annotations will grow up indefinitely, right?

Yes until cache is cleared

bmarty · 2021-12-15T20:39:06Z

...droid/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/DefaultTimeline.kt

-        TimelineInput.Listener,
-        UIEchoManager.Listener {
+
+internal class DefaultTimeline internal constructor(private val roomId: String,


Maybe remove internal constructor

bmarty · 2021-12-15T20:43:11Z

.../main/java/org/matrix/android/sdk/internal/session/room/timeline/TokenChunkEventPersistor.kt

-     * ========================================================================================================
-     * </pre>
-     */
-


bmarty

Thanks for the update!

ganfra added 15 commits September 7, 2021 19:28

Timeline: try new strategy for handling chunks (no merging)

9238037

Timeline: small on fixes on new implementation

4f145e3

Merge branch 'develop' into feature/fga/timeline_chunks_rework

57e5eca

Timeline rework: continue branching things.

94a6950

Timeline: add some logs and fix epoxy cache

da75642

Timeline rework: handle lastForwardChunk

2283030

Timeline rework: add db migration

7f9c191

Merge branch 'develop' into feature/fga/timeline_chunks_rework

63aa5b4

Timeline rework: add some comments and fix pagination when having ove…

b370f84

…rlapping events

Timeline rework: make sure migration doesn't crash

cd1da73

Merge branch 'develop' into feature/fga/timeline_chunks_rework

e4896ca

Timeline: don't remove annotations and read receipts when deleting ti…

d42a2e6

…meline event

Timeline: remove useless methods

ce5ccd4

Timeline: remove previous lastForward chunk

52d0da7

Timeline: just some renaming + constant

a1fdd31

ganfra marked this pull request as draft November 3, 2021 18:16

ganfra added 13 commits November 4, 2021 13:11

Timeline: fix hasReachedEnd

92a37f1

Timeline: continue trying to make Read marker/receipts working

52df50a

Timeline: fix double link issue when server is messed up...

8c0b2a6

Merge branch 'develop' into feature/fga/timeline_chunks_rework

3d27e21

Timeline: re-add usage of TimelineSettings

e562d76

Merge branch 'develop' into feature/fga/timeline_chunks_rework

3d31ba9

Timeline: update when loading states changed

03961fe

Timeline: avoid notifying when decryption error is same as previous

76eddef

Timeline: try to optimise a bit the loading

014da84

Timeline: get off main thread when waiting for positionOrReadMarker

7fa4bf1

Merge branch 'develop' into feature/fga/timeline_chunks_rework

8ca60ea

Timeline: check for create event to hide loader

3217277

Merge branch 'develop' into feature/fga/timeline_chunks_rework

49eee0d

ganfra added 9 commits December 7, 2021 21:13

Timeline: make 3 integration tests passing (also add some suspend met…

29a4fd1

…hod on the timeline)

Merge branch 'develop' into feature/fga/timeline_chunks_rework

bf1be4f

Timeline: some clean up

b53433e

Timeline: change a bit when postPagination is triggered

1e2e9e1

Timeline: remove onStateUpdated in controller

c830d49

Timeline: url preview listen to Flow in ViewModel

31ba912

Timeline: remove LifecycleOwner on EpoxyModel as it's not used (and t…

faebf95

…akes some time uselessly)

Timeline: clean up

bf287d1

Merge branch 'develop' into feature/fga/timeline_chunks_rework

1280687

ganfra marked this pull request as ready for review December 9, 2021 13:21

ouchadam self-requested a review December 14, 2021 09:08

ouchadam reviewed Dec 14, 2021

View reviewed changes

ouchadam approved these changes Dec 14, 2021

View reviewed changes

bmarty reviewed Dec 15, 2021

View reviewed changes

ganfra added 2 commits January 3, 2022 16:06

Timeline : clean after PR reviews

12b775c

Merge branch 'develop' into feature/fga/timeline_chunks_rework

9121585

bmarty approved these changes Jan 3, 2022

View reviewed changes

bmarty merged commit d670d3e into develop Jan 3, 2022

bmarty deleted the feature/fga/timeline_chunks_rework branch January 3, 2022 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/fga/timeline chunks rework #4405

Feature/fga/timeline chunks rework #4405

ganfra commented Nov 3, 2021

github-actions bot commented Nov 3, 2021 •

edited

Loading

ouchadam Dec 14, 2021

ganfra Jan 3, 2022

ouchadam Dec 14, 2021

ganfra Jan 3, 2022

ouchadam Dec 14, 2021

ouchadam Dec 14, 2021

ganfra Dec 16, 2021

ouchadam left a comment

bmarty left a comment

bmarty Dec 15, 2021

ganfra Dec 16, 2021

bmarty Dec 16, 2021

ganfra Jan 3, 2022

bmarty Dec 15, 2021

ganfra Jan 3, 2022

bmarty Dec 15, 2021

ganfra Dec 16, 2021

bmarty Dec 15, 2021

ganfra Dec 16, 2021

bmarty Dec 31, 2021

ganfra Jan 3, 2022

bmarty Dec 15, 2021

ganfra Jan 3, 2022

bmarty Dec 15, 2021

bmarty left a comment

Feature/fga/timeline chunks rework #4405

Feature/fga/timeline chunks rework #4405

Conversation

ganfra commented Nov 3, 2021

github-actions bot commented Nov 3, 2021 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2021 •

edited

Loading