Feature/fga/rx flow migration #4363

ganfra · 2021-10-28T15:37:55Z

Closes #4219
Please test it carefully.
I made some profiling to track stuff being done on main thread, looking good.

ouchadam · 2021-10-28T15:54:07Z

vector/src/main/java/im/vector/app/features/home/ShortcutsHandler.kt

-
-        val session = activeSessionHolder.getSafeActiveSession() ?: return Disposables.empty()
+        hasPinCode.set(pinCodeStore.getEncodedPin() != null)
+        val session = activeSessionHolder.getSafeActiveSession() ?: return Job()
        return session.getRoomSummariesLive(


could/should this use SessionFlow instead?

Yup I guess

github-actions · 2021-10-28T15:56:46Z

Unit Test Results

  62 files ±0   62 suites ±0 49s ⏱️ -15s
118 tests ±0 118 ✔️ ±0 0 💤 ±0 0 ❌ ±0
350 runs ±0 350 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit e43bfae. ± Comparison against base commit c22d3fb.

♻️ This comment has been updated with latest results.

ouchadam · 2021-10-28T15:58:28Z

vector/src/main/java/im/vector/app/features/roomprofile/members/RoomMemberListViewModel.kt

@@ -95,7 +93,6 @@ class RoomMemberListViewModel @AssistedInject constructor(@Assisted initialState

        if (room.isEncrypted()) {
            room.flow().liveRoomMembers(roomMemberQueryParams)
-                    .flowOn(Dispatchers.Main)


oooh nice catch!

It's not really a catch, it's just that it's done by default as we launch in viewModelScope (which is Dipatchers.Main)

ouchadam · 2021-10-28T16:07:22Z

there's a lot of changes here to review 😅 Most of the changes look like 1:1 replacements~

for the failing test compilation, we can now remove InstantRxRule

ouchadam

small remark around if we should be using SessionFlow
otherwise LGTM, really awesome stuff 💯

no issues with a quick smoke test

joining rooms
sending/receiving messages
sign in

bmarty

A few remarks after a quick review.
Can you add a small migration file please?

bmarty · 2021-10-28T16:01:45Z

dependencies.gradle

@@ -41,7 +41,8 @@ ext.libs = [
        jetbrains   : [
                'coroutinesCore'          : "org.jetbrains.kotlinx:kotlinx-coroutines-core:$kotlinCoroutines",
                'coroutinesAndroid'       : "org.jetbrains.kotlinx:kotlinx-coroutines-android:$kotlinCoroutines",
-                'coroutinesRx2'           : "org.jetbrains.kotlinx:kotlinx-coroutines-rx2:$kotlinCoroutines"
+                'coroutinesRx2'           : "org.jetbrains.kotlinx:kotlinx-coroutines-rx2:$kotlinCoroutines",


Do we still need this?

It depends if we still want to provide the matrix-android-sdk-rx wrapper?

If Element Android does not use matrix-android-sdk-rx anymore, I think we can remove it. This module is not exported along with the SDK

bmarty · 2021-10-28T16:05:07Z

vector/src/main/java/im/vector/app/core/rx/RxConfig.kt

-class RxConfig @Inject constructor(
-        private val vectorPreferences: VectorPreferences
-) {
+class RxConfig @Inject constructor() {


Shall we just remove this class?

bmarty · 2021-10-28T16:07:35Z

vector/src/main/java/im/vector/app/features/home/UnreadMessagesSharedViewModel.kt

-        ).asObservable()
-                .throttleFirst(300, TimeUnit.MILLISECONDS)
+        ).asFlow()
+                .sample(300)


this is not replaced by throttleFirst here?

bmarty · 2021-10-28T16:08:10Z

vector/src/main/java/im/vector/app/features/home/HomeDetailViewModel.kt

-                .observeOn(Schedulers.computation())
-                .throttleFirst(300, TimeUnit.MILLISECONDS)
-                .subscribe {
+                .sample(300)


This is not replaced by throttleFirst here?

bmarty · 2021-10-28T16:08:32Z

vector/src/main/java/im/vector/app/features/home/UnreadMessagesSharedViewModel.kt

-                                ).size
-                            }
+                    ).asFlow()
+                            .sample(300)


throttleFirst?

bmarty · 2021-10-28T16:10:01Z

...r/src/main/java/im/vector/app/features/roomprofile/settings/joinrule/RoomJoinRuleActivity.kt

@@ -40,6 +40,7 @@ import im.vector.app.features.home.room.detail.upgrade.MigrateRoomBottomSheet
 import im.vector.app.features.roomprofile.RoomProfileArgs
 import im.vector.app.features.roomprofile.settings.joinrule.advanced.RoomJoinRuleChooseRestrictedActions
 import im.vector.app.features.roomprofile.settings.joinrule.advanced.RoomJoinRuleChooseRestrictedEvents
+import im.vector.app.features.roomprofile.settings.joinrule.advanced.RoomJoinRuleChooseRestrictedFragment


Strange change (just one new import in this file?)

Weird, maybe from a merge?

bmarty

Thanks for the update.
I will do another code review today to ensure everything is fine.

bmarty · 2021-10-29T07:02:26Z

docs/rx_flow_migration.md

+launchIn(viewModelScope)
+```
+
+Also be aware that when using these scopes the coroutine is launched on Dispatchers.Main by default.


Thanks for the doc! Really helpful

bmarty · 2021-10-29T07:03:44Z

vector/src/main/java/im/vector/app/features/home/UnreadMessagesSharedViewModel.kt

@@ -114,7 +114,7 @@ class UnreadMessagesSharedViewModel @AssistedInject constructor(@Assisted initia
                                this.memberships = Membership.activeMemberships()
                            }, sortOrder = RoomSortOrder.NONE
                    ).asFlow()
-                            .sample(300)
+                            .throttleFirst(300)


Since they were some small mistakes like that, I will review the PR again more carefully.

bmarty

Made a second deeper review. I'm not sure to get the diff betwwen sample and throttleFirst.

bmarty · 2021-10-29T07:52:21Z

dependencies.gradle

+                'glideImageViewFactory'  : "com.github.piasy:GlideImageViewFactory:$bigImageViewer",
+                'flowBinding'            : "io.github.reactivecircus.flowbinding:flowbinding-android:$flowBinding",
+                'flowBindingAppcompat'   : "io.github.reactivecircus.flowbinding:flowbinding-appcompat:$flowBinding",
+                'flowBindingMaterial'    : "io.github.reactivecircus.flowbinding:flowbinding-material:$flowBinding"


update the file open_source_license.html?

bmarty · 2021-10-29T09:38:04Z

vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailViewModel.kt

+
+        visibleEventsSource
+                .stream()
+                .chunk(1000)


chunk is replacing buffer?

Yes, it's the kotlin keyword for rx "buffer"

bmarty · 2021-10-29T09:41:16Z

vector/src/main/java/im/vector/app/features/reactions/EmojiReactionPickerActivity.kt

-                    .doOnError { err -> Timber.e(err) }
-                    .observeOn(AndroidSchedulers.mainThread())
-                    .subscribe { query ->
+                    .sample(600)


Another mistake here I think.

bmarty · 2021-10-29T09:43:06Z

vector/src/main/java/im/vector/app/features/settings/devices/DevicesViewModel.kt

@@ -166,12 +165,12 @@ class DevicesViewModel @AssistedInject constructor(
 //                    )
 //                }

-        refreshPublisher.throttleFirst(4_000, TimeUnit.MILLISECONDS)
-                .subscribe {
+        refreshSource.stream().sample(4_000)


Not sure this is really important but changed

bmarty · 2021-11-02T12:27:47Z

vector/src/main/assets/open_source_licenses.html

-        <b>rxbinding</b>
-        <br/>
-        Copyright (C) 2015 Jake Wharton
+        Copyright 2019 Yang Chen


bmarty

Thanks for the update
For Element 1.3.8

ganfra added 11 commits October 26, 2021 14:24

Flow migration: start replacing Rx by Flow

c936954

Flow: remove more rx

9479342

Flow: restore read receipts

8cf5b72

Flow migration: add back some test

a9d192f

Flow migration: remove Rx completely (rxbinding)

34cb99e

Merge branch 'develop' into feature/fga/rx_flow_migration

635ca8e

Flow migration: clean up

b93e67e

Flow migration: remove Shortcuts process from main thread

da47bfd

Merge branch 'develop' into feature/fga/rx_flow_migration

8364484

Flow migration: add Changelog

79c5af2

Flow migration: use throttleFirst instead of sample on UI

edf068e

ouchadam reviewed Oct 28, 2021

View reviewed changes

ouchadam approved these changes Oct 28, 2021

View reviewed changes

bmarty reviewed Oct 28, 2021

View reviewed changes

ganfra added 2 commits October 28, 2021 18:17

Flow migration: fix kotlinx-coroutines-debug dependencie

16e4a7f

Flow migration: update after PR reviews

bb4a820

bmarty reviewed Oct 29, 2021

View reviewed changes

Flow migration: more update after PR reviews

e43bfae

bmarty reviewed Nov 2, 2021

View reviewed changes

vector/src/main/assets/open_source_licenses.html

<b>rxbinding</b>

<br/>

Copyright (C) 2015 Jake Wharton

Copyright 2019 Yang Chen

Copy link

Member

bmarty Nov 2, 2021

Choose a reason for hiding this comment

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

👍

bmarty approved these changes Nov 4, 2021

View reviewed changes

bmarty merged commit f3655d4 into develop Nov 4, 2021

bmarty deleted the feature/fga/rx_flow_migration branch November 4, 2021 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/fga/rx flow migration #4363

Feature/fga/rx flow migration #4363

ganfra commented Oct 28, 2021

ouchadam Oct 28, 2021 •

edited

Loading

ganfra Oct 28, 2021

github-actions bot commented Oct 28, 2021 •

edited

Loading

ouchadam Oct 28, 2021

ganfra Oct 28, 2021

ouchadam commented Oct 28, 2021

ouchadam left a comment

bmarty left a comment

bmarty Oct 28, 2021

ganfra Oct 28, 2021

bmarty Oct 28, 2021

bmarty Oct 28, 2021

ganfra Oct 28, 2021

bmarty Oct 28, 2021

ganfra Oct 28, 2021

bmarty Oct 28, 2021

ganfra Oct 28, 2021

bmarty Oct 28, 2021

bmarty Oct 28, 2021

ganfra Oct 28, 2021

bmarty left a comment

bmarty Oct 29, 2021

bmarty Oct 29, 2021 •

edited

Loading

bmarty left a comment

bmarty Oct 29, 2021

ganfra Nov 2, 2021

bmarty Oct 29, 2021

ganfra Nov 2, 2021 •

edited

Loading

bmarty Oct 29, 2021

bmarty Oct 29, 2021

ganfra Nov 2, 2021

bmarty Nov 2, 2021

bmarty left a comment

Feature/fga/rx flow migration #4363

Feature/fga/rx flow migration #4363

Conversation

ganfra commented Oct 28, 2021

ouchadam Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 28, 2021 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam commented Oct 28, 2021

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

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty Oct 29, 2021 • edited Loading

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

ganfra Nov 2, 2021 • edited Loading

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

ouchadam Oct 28, 2021 •

edited

Loading

github-actions bot commented Oct 28, 2021 •

edited

Loading

bmarty Oct 29, 2021 •

edited

Loading

ganfra Nov 2, 2021 •

edited

Loading