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

Feature/fga/mavericks 2 #4190

Merged
merged 28 commits into from
Oct 12, 2021
Merged

Feature/fga/mavericks 2 #4190

merged 28 commits into from
Oct 12, 2021

Conversation

ganfra
Copy link
Member

@ganfra ganfra commented Oct 8, 2021

This PR is introducing the migration to Mavericks 2.
Mavericks 2 is replacing usage of Rx by Flow internally and on the API too.
See here for more details: https://airbnb.io/mavericks/#/new-2x

So in this PR, you will find:

  • matrix-sdk-android-flow to make some binding LiveData <-> Flow (replacing matrix-sdk-android-rx)
  • replace everywhere MvRx to Mavericks
  • replacing most of the Rx usages to Flow

There is still some RxBinding and Behavior/SourceRelay to replace to finish the work.

abstract class VectorViewModel<S : MvRxState, VA : VectorViewModelAction, VE : VectorViewEvents>(initialState: S) :
BaseMvRxViewModel<S>(initialState, false) {
abstract class VectorViewModel<S : MavericksState, VA : VectorViewModelAction, VE : VectorViewEvents>(initialState: S) :
BaseMvRxViewModel<S>(initialState) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Still using BaseMvRxViewModel as we have some Rx. In the end it will be changed to MavericksViewModel.

Copy link
Member

Choose a reason for hiding this comment

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

OK, can you create an issue to track the future change please?

Copy link
Member

Choose a reason for hiding this comment

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

@github-actions
Copy link

github-actions bot commented Oct 8, 2021

Unit Test Results

  42 files  ±0    42 suites  ±0   42s ⏱️ -5s
  83 tests ±0    83 ✔️ ±0  0 💤 ±0  0 ±0 
220 runs  ±0  220 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit a24a9b4. ± Comparison against base commit 3a387c5.

♻️ This comment has been updated with latest results.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, will investigate the issue with compiler.
Can you write a quick migration guide in /docs, to help the fork to handle the required change please?

matrix-sdk-android-flow/build.gradle Outdated Show resolved Hide resolved
matrix-sdk-android-flow/build.gradle Outdated Show resolved Hide resolved
matrix-sdk-android-flow/build.gradle Outdated Show resolved Hide resolved
@@ -137,7 +138,7 @@ class VectorApplication :
}
logInfo()
LazyThreeTen.init(this)

Mavericks.initialize(debugMode = false)
Copy link
Member

Choose a reason for hiding this comment

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

use BuildConfig.DEBUG ? (not checked the effect)

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't as it will double check setState to be idempotent, but we have some "tricks" which breaks this

@@ -88,7 +89,7 @@ import timber.log.Timber
import java.util.concurrent.TimeUnit
import kotlin.system.measureTimeMillis

abstract class VectorBaseActivity<VB : ViewBinding> : AppCompatActivity(), HasScreenInjector {
abstract class VectorBaseActivity<VB : ViewBinding> : AppCompatActivity(), HasScreenInjector, MvRxView {
Copy link
Member

Choose a reason for hiding this comment

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

We still use MvRxView here, and not MavericksView?

@@ -49,7 +50,7 @@ import io.reactivex.disposables.Disposable
import timber.log.Timber
import java.util.concurrent.TimeUnit

abstract class VectorBaseFragment<VB : ViewBinding> : BaseMvRxFragment(), HasScreenInjector {
abstract class VectorBaseFragment<VB : ViewBinding> : Fragment(), MvRxView, HasScreenInjector {
Copy link
Member

Choose a reason for hiding this comment

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

same remark

@bmarty
Copy link
Member

bmarty commented Oct 11, 2021

Also can you add a file for the changelog please?

@bmarty
Copy link
Member

bmarty commented Oct 11, 2021

FTR the allScreen sanity test is passing, but with settings test disabled due to an issue which was on develop

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

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

awesome stuff! 💯

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the update!

@bmarty bmarty enabled auto-merge October 12, 2021 12:00
@bmarty bmarty merged commit e3034e5 into develop Oct 12, 2021
@bmarty bmarty deleted the feature/fga/mavericks_2 branch October 12, 2021 12:11
@bmarty bmarty mentioned this pull request Oct 12, 2021
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