Add Jetpack navigation module #351

marukami · 2020-02-15T09:10:04Z

Navigation 2.2.0 requires Java8. So, everything was updated to Java8
Add a navGraphViewModel for navigation graph view models
navigationLifecycleAwareLazy a slightly forked lifecycleAwareLazy that subscribes the MvRx view when possible and if not defers the lazy until the fragment access the view model
MvRxNavDirections a NavDirections that makes it a little easier to pass an argument Parcelable and set the navigation label if provided
Added very basic example navigation repo.
Added Test Fragment scenario for navigation module.

NOTES

The mvrx view needs to wait for until at least the onViewCreated callback. During lifecycle flows like re-configuration the NavHostFragment is not accessible until after the activity onCreate has finished.
Oops accidentally deleted branch for Add Jetpack navigation module #337 and it closed the PR 😅

jpventura · 2020-03-27T17:42:17Z

@elihart @gpeal both #364 and #367 refactors were made to reduce the unnecessary code of inserted by this pull request.

I would like to know if, once it is approved, you have any plans of merging it into MvRx project or if you will keep it as a suggestion. Maybe we could split @marukami code into two PRs (one fore mvrx-navigation and another for hello-navigation).

Please let me know if you have any further questions.

marukami · 2020-04-02T23:43:53Z

@elihart @gpeal both #364 and #367 refactors were made to reduce the unnecessary code of inserted by this pull request.

I would like to know if, once it is approved, you have any plans of merging it into MvRx project or if you will keep it as a suggestion. Maybe we could split @marukami code into two PRs (one fore mvrx-navigation and another for hello-navigation).

Please let me know if you have any further questions.

@jpventura #364 and #367 Look good to me thanks.

I would like to know if, once it is approved, you have any plans of merging it into MvRx project or if you will keep it as a suggestion.

Getting it merged would be good. I update the PR to remove the example as androidx.navigation@2.3.0 adds a test NavHost that makes creating some tests actually possible. Tho, the Navigation component does not seem to work in test src, so, I put the test code in debug… not the best fix, hopefully, that's serviceable as an OK-ish workaround.

Maybe we could split @marukami code into two PRs (one fore mvrx-navigation and another for hello-navigation).

Yep, an example module a separate PR works.

@elihart @gpeal let me know what you think about this revised PR.

jpventura · 2020-04-03T12:40:31Z

dependencies.gradle

@@ -74,6 +73,7 @@ ext {
            kotlin                : "org.jetbrains.kotlin:kotlin-stdlib-jdk8:${versions.kotlin}",
            kotlinReflect         : "org.jetbrains.kotlin:kotlin-reflect:${versions.kotlin}",
            lifecycleExtensions   : "androidx.lifecycle:lifecycle-extensions:${versions.lifecycle}",
+            lifecycleCommonJava8  : "androidx.lifecycle:lifecycle-common-java8:${versions.lifecycle}",


Nice catch!

All versions, libraries, testLibraries, and androidTestLibraries were lexicographically sorted. Also Java 8 should be the default compiler across all modules.

Maybe lifecycleCommon would be more semantic/consistent name. Does it make sense @marukami?

were lexicographically sorted

Ah, I see. lifecycleCommon seems fine

jpventura · 2020-04-03T12:41:18Z

dogs/build.gradle

@@ -20,8 +20,8 @@ android {
    }

    compileOptions {
-        sourceCompatibility = 1.8
-        targetCompatibility = 1.8
+        sourceCompatibility JavaVersion.VERSION_1_8


Nice catch!

jpventura · 2020-04-03T12:44:05Z

mvrx-navigation/build.gradle

+    toolVersion = "0.8.2"
+}
+
+dependencies {


Keep dependencies lexicographycally sorted 😉

dependencies { implementation libraries.kotlin implementation libraries.lifecycleCommon implementation project(":mvrx") implementation project(":testing") testImplementation libraries.rxAndroid testImplementation testLibraries.junit testImplementation testLibraries.mockito testImplementation testLibraries.roboeletric }

jpventura · 2020-04-03T12:46:23Z

mvrx-navigation/build.gradle

+
+// module specific dependencies
+dependencies {
+    def nav_version = "2.3.0-alpha04"


@marukami I also had hard times with alpha libraries from Google.

Since this would be part of an official Airbnb library, maybe it would be better using the latest stable navigation-fragment-ktx (in dependencies.gradle file it is currently set to 2.2.1).

@elihart @gpeal any words on this?

@marukami I also had hard times with alpha libraries from Google.

I've found the Androidx stuff to be fine and most issues you can workaround. Far as I can tell Stable and Alpha are mostly the same except the newer APIs, tho this is Andoidx and Google so who knows how they are composing the stable and alpha channel code.

For the Alpha here I was planning to use the TestNavHostController that comes in the 2.3.0-alpha; I've found it a lot better than mocking in a few projects. Tho, I did not end up using it, so, yer I think 2.2.1 should work.

2.2.1 it is then.

jpventura · 2020-04-03T12:50:01Z

mvrx-navigation/build.gradle

+}
+
+// module specific dependencies
+dependencies {


I am using your branch in a personal project of mine (that is the whole reason why I was pushing the refactors 😉).

@marukami I was trying to figure out why you made two separated segments of dependencies into this build.gradle.

Does not api already suggests that a package is restricted to the module?

Some libraries already are saved at dependencies.gradle:

dependencies { api libraries.navigationFragmentKtx api libraries.navigationUiKtx api libraries.rxAndroid debugApi libraries.fragment debugApi libraries.fragmentKtx implementation libraries.kotlin implementation libraries.lifecycleCommon implementation project(":mvrx") implementation project(":testing") debugImplementation libraries.fragmentTesting testImplementation testLibraries.junit testImplementation testLibraries.mockito testImplementation testLibraries.navigationTesting testImplementation testLibraries.roboeletric }

I was trying to figure out why you made two separate segments of dependencies into this build.gradle.

We do not need dependencies.gradle here as these libraries are not shared with any other module and having a separator like a 2nd dependencies seemed like a nice way to segment the core project dependencies from the module-specific dependencies.

Does not api already suggests that a package is restricted to the module?

That rule seems a bit oversimplified. It's an OK rule of thumb inside a project module as for a Library I'm not sure. Tho in a library I don't think it's a restricted module otherwise then we would API include most things.

The assumption is here is the same as with implementation for Koltin STD and AppCompat. In that the consumer is assumed to already be including the navigation-component modules otherwise they can't have a navigation graph to attach the ViewModel.

Tho, I've not managed many libraries and I migth be missing something here.

jpventura · 2020-04-03T12:52:40Z

mvrx/build.gradle


    api libraries.rxAndroid
    api libraries.rxJava
+    api libraries.lifecycleCommonJava8


api libraries.lifecycleCommon api libraries.rxAndroid api libraries.rxJava

jpventura · 2020-04-03T12:54:30Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxViewModelFactory.kt

@@ -101,11 +101,13 @@ data class FragmentViewModelContext(
    /**
     * The fragment owner of the ViewModel.
     */
-    val fragment: Fragment
+    val fragment: Fragment,
+    private val ownerProvider: ViewModelStoreOwner = fragment,


@marukami name suggestion: storeOwner

jpventura · 2020-04-03T12:58:28Z

mvrx/src/main/kotlin/com/airbnb/mvrx/lifecycleAwareLazy.kt

@@ -16,7 +14,7 @@ private object UninitializedValue
 */
 @RestrictTo(RestrictTo.Scope.LIBRARY)
 @SuppressWarnings("Detekt.ClassNaming")
-class lifecycleAwareLazy<out T>(private val owner: LifecycleOwner, initializer: () -> T) : Lazy<T>, Serializable {
+class lifecycleAwareLazy<out T>(owner: LifecycleOwner, initializer: () -> T) : Lazy<T>, Serializable, DefaultLifecycleObserver {


@marukami suggestion

class lifecycleAwareLazy<out F>( owner: LifecycleOwner, initializer: () -> F ) : Lazy<F>, Serializable, DefaultLifecycleObserver {

I don't know what is the correct Kotlin style according Google/JetBrains when there are tons of parameters and interfaces, but this is the way I cheat the linter and keep things readable (well, at least it does not complain 😉)

I don't know what is the correct Kotlin style according Google/JetBrains

There isn't one 😀 I find it a bit hard as the project style XML seems to be at odds with the detetkt rules and AS keeps differing from the projects actual rules.

I only removed the private scope access. I guess we can update the style to match things while we are here.

I don't know what is the correct Kotlin style according Google/JetBrains when there are tons of parameters and interfaces, but this is the way I cheat the linter and keep things readable (well, at least it does not complain 😉)

This project has code style checked into git, so you can set this up to be done automatically via code reformatting (ctrl+alt+L or command+option+L by default in Android Studio). The code style for this is kotlin -> wrapping and braces -> function declaration parameters -> chop down if long.

When I made this PR this was the project style. After #387 is merged I'm going to rebase and format to match the updated styles.

jpventura · 2020-04-03T15:08:45Z

mvrx-navigation/build.gradle

+    enabled = false
+}
+
+task coverage(type: JacocoReport, dependsOn: "testDebugUnitTest") {


@marukami absolute n00b Gradle question:

Since this task is also present at mvrx, could it be moved to the root build.gradle file?

Yep, the best way is to create a named task in buildSrc and then loop over subprojects and register the task to every module. The tricky thing here is that we have example modules mixed in. Given we only have a few modules a simple custom plugin would reduce the most code per module.

jpventura · 2020-04-04T11:38:55Z

@marukami I don't know what are @elihart and @gpeal roadmap about replacing Rx by Kotlin coroutines).

If your features will be merged after #347, build navigation module over coroutines branch could speed up integrating your features in the next release.

jpventura · 2020-04-04T12:47:56Z

@marukami my last pull request broke Travis CI environment just because my code was not following the correct style, just like yours 😄

Adding this to .git/hooks/pre-push protected my from my future me:

#!/usr/bin/env bash

./gradlew lintFix && ./gradlew test

marukami · 2020-04-05T22:36:41Z

@marukami my last pull request broke Travis CI environment just because my code was not following the correct style, just like yours 😄

Adding this to .git/hooks/pre-push protected my from my future me:
#!/usr/bin/env bash

./gradlew lintFix && ./gradlew test

Righto. Are CI jobs only master, not PRs?

OK then that's easy to fix

elihart

@marukami Thanks for all your work on this. Sorry it's taken us a while to get to, I'll try to spend some time on this soon

gpeal · 2020-08-14T16:29:29Z

@marukami I'd like to merge this before your other PR so you can include a sample. Without it, it won't be easy for people to figure out how to use it.

However, I think you may have mis-targeted this PR. It is targeting master. Did you intend to target release/2.0.0?

marukami · 2020-08-17T02:26:06Z

However, I think you may have mis-targeted this PR. It is targeting master. Did you intend to target release/2.0.0?

Ah, yes, I was working on a release/2.0.0 variant. Will fix up today.

# Conflicts: # CHANGELOG.md # mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxViewModelFactory.kt

* added navigationLifecycleAwareLazy as Navigation ViewModel need to wait for onStart to safely access the navController

marukami · 2020-09-06T12:45:38Z

@marukami I'd like to merge this before your other PR so you can include a sample. Without it, it won't be easy for people to figure out how to use it.

However, I think you may have mis-targeted this PR. It is targeting master. Did you intend to target release/2.0.0?

@gpeal I fix up my git for this 1.0 PR and push up #443 for the 2.0 release.

gpeal · 2021-01-02T23:27:26Z

Closed by #443

marukami mentioned this pull request Feb 15, 2020

Add Jetpack navigation module #337

Closed

marukami requested a review from gpeal February 16, 2020 05:09

jpventura mentioned this pull request Mar 26, 2020

MvRx at Airbnb utilities #112

Open

marukami force-pushed the navigation-viewmodel-store branch 4 times, most recently from d6b3c80 to 02c586f Compare April 2, 2020 23:27

jpventura reviewed Apr 3, 2020

View reviewed changes

jpventura mentioned this pull request Apr 4, 2020

build: lifecycle-common instead of lifecycle-extensions #372

Merged

marukami force-pushed the navigation-viewmodel-store branch from 02c586f to 50ded18 Compare April 6, 2020 23:48

marukami mentioned this pull request Apr 7, 2020

use plugin to remove build.gradle duplication #373

Merged

elihart reviewed Apr 13, 2020

View reviewed changes

marukami force-pushed the navigation-viewmodel-store branch from 50ded18 to 96f783e Compare April 14, 2020 15:41

marukami force-pushed the navigation-viewmodel-store branch from 96f783e to d8141ff Compare May 12, 2020 00:51

marukami mentioned this pull request May 12, 2020

Allow for custom factories for VM Store #387

Closed

marukami force-pushed the navigation-viewmodel-store branch from d8141ff to 4f36703 Compare May 13, 2020 23:00

marukami force-pushed the navigation-viewmodel-store branch from 4f36703 to aee0783 Compare August 13, 2020 23:26

marukami force-pushed the navigation-viewmodel-store branch from aee0783 to 08b7a44 Compare August 13, 2020 23:41

marukami force-pushed the navigation-viewmodel-store branch 3 times, most recently from 1ea1191 to eb3d684 Compare August 17, 2020 03:03

marukami added 2 commits September 5, 2020 22:27

Allow for custom factories for VM Store

c6ab621

# Conflicts: # CHANGELOG.md # mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxViewModelFactory.kt

Add Jetpack Android Navigation Module

42598c2

* added navigationLifecycleAwareLazy as Navigation ViewModel need to wait for onStart to safely access the navController

marukami force-pushed the navigation-viewmodel-store branch from eb3d684 to 42598c2 Compare September 6, 2020 07:57

marukami mentioned this pull request Sep 6, 2020

Add Jetpack navigation module v2 #443

Merged

Update build file to match other modules

7edf5f0

marukami requested a review from elihart September 28, 2020 23:02

gpeal closed this Jan 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Jetpack navigation module #351

Add Jetpack navigation module #351

marukami commented Feb 15, 2020 •

edited

Loading

jpventura commented Mar 27, 2020

marukami commented Apr 2, 2020

jpventura Apr 3, 2020

mt-mitchell Apr 5, 2020

jpventura Apr 3, 2020

jpventura Apr 3, 2020 •

edited

Loading

jpventura Apr 3, 2020 •

edited

Loading

marukami Apr 5, 2020

jpventura Apr 3, 2020 •

edited

Loading

jpventura Apr 3, 2020 •

edited

Loading

marukami Apr 5, 2020

jpventura Apr 3, 2020

jpventura Apr 3, 2020

jpventura Apr 3, 2020

marukami Apr 5, 2020

Minirogue Jun 10, 2020

marukami Jun 11, 2020

jpventura Apr 3, 2020

marukami Apr 5, 2020 •

edited

Loading

jpventura commented Apr 4, 2020

jpventura commented Apr 4, 2020

marukami commented Apr 5, 2020

elihart left a comment

gpeal commented Aug 14, 2020 •

edited

Loading

marukami commented Aug 17, 2020

marukami commented Sep 6, 2020

gpeal commented Jan 2, 2021

Add Jetpack navigation module #351

Add Jetpack navigation module #351

Conversation

marukami commented Feb 15, 2020 • edited Loading

jpventura commented Mar 27, 2020

marukami commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpventura Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

jpventura Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpventura Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

jpventura Apr 3, 2020 • 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

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

marukami Apr 5, 2020 • edited Loading

Choose a reason for hiding this comment

jpventura commented Apr 4, 2020

jpventura commented Apr 4, 2020

marukami commented Apr 5, 2020

elihart left a comment

Choose a reason for hiding this comment

gpeal commented Aug 14, 2020 • edited Loading

marukami commented Aug 17, 2020

marukami commented Sep 6, 2020

gpeal commented Jan 2, 2021

marukami commented Feb 15, 2020 •

edited

Loading

jpventura Apr 3, 2020 •

edited

Loading

jpventura Apr 3, 2020 •

edited

Loading

jpventura Apr 3, 2020 •

edited

Loading

jpventura Apr 3, 2020 •

edited

Loading

marukami Apr 5, 2020 •

edited

Loading

gpeal commented Aug 14, 2020 •

edited

Loading