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

Conductor integration, and Kotlin support #58

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

szantogab
Copy link

@szantogab szantogab commented May 2, 2017

This is a rather large pull request, and I make this pull request to discuss whether the direction we're heading is good or not.

I created 3 modules: core, conductor, and kotlin. Each module can be published to bintray as a separate module, so the goal was to be able to use them independently.

The core module consists of the basic stuff that is needed to implement MVVM in Java: DataBinding adapters, BindingConversions, ReadOnlyField. Some classes are added here: ReadOnlyList and so on. For now I removed MvvmActivity, but that can be included here as well, or in a separate module. The core module has been migrated to RxJava2, I think we should drop support for RxJava1, or just create a separate branch, so that if anyone wants to use the current library with RxJava1 can do so.

The Kotlin module only contains two Kotlin files, and its goal is to provide Kotlin support, so a set of cool extension functions like Observable.asField(), and ObservableField.asObservable().

And finally, the last module is the Conductor module. Its goal is to build upon the Conductor Android framework, so that we can eliminate Activities and Fragments, and all our UI components can be written as Controllers. Also, Conductor retains Controllers on orientation change, so our ViewModel lifecycle can be 1:1 tied to the Controller lifecycle. This way, we don't lose any data on orientation change. Also, the ViewModel now can be aware of the Controller's lifecycle.

Consider the following code:

class LoginViewModel @Inject constructor(userService: UserService,
                                         loginNavigator: LoginNavigator,
                                         messageHelper: MessageHelper,
                                         lp: ControllerLifecycleProvider) : ViewModel
    val email = ObservableField<String>("")
    val password = ObservableField<String>("")
    val loginTap = PublishSubject.create<Any>()

    private val loginResult = loginTap
            .flatMapFirst(userService.login(email.get(), password.get()))
            .share()
            .attachToLifecycle(lp, ControllerEvent.ATTACH, ControllerEvent.DETACH)

    init {
        loginResult.subscribeBy(onNext = {
            loginNavigator.navigateToMain()
        })
    }

So as you can see, the loginResult observable will be subscribed to when the Controller's attach event happens, and unsubscribed (completed) when the detach event happens.

What do you think? :)

Copy link
Owner

@manas-chaudhari manas-chaudhari left a comment

Choose a reason for hiding this comment

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

Thanks for creating this very interesting pull request.

I am curious about Conductor framework. It would be very insightful if you could implement/describe an example of conductor + MVVM.


override fun onCreateView(inflater: LayoutInflater, container: ViewGroup): View {
val vm = this::class.java.getDeclaredField("viewModel")
vm.isAccessible = true
Copy link
Owner

Choose a reason for hiding this comment

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

Why use reflection?

}

@NonNull
public static <T> Disposable bindTo(@NonNull final Observable<T> observable, @NonNull final ObservableField<T> field) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think bindTo is dangerous. If a field is bound to multiple Observables, the value can match to either of the Observables, which brings back mutation problems.

Lets look at usecases for bindTo and see if they can be tackled with ReadOnlyField?

import io.reactivex.disposables.Disposable;
import io.reactivex.functions.Consumer;

public class ReadOnlyList<T extends List<U>, U> extends ObservableArrayList<U> {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you elaborate on the usecase for ReadOnlyList?

*/
@CheckResult
@CheckReturnValue
fun startLoading(start1: Observable<*>, start2: Observable<Any> = PublishSubject.create()): Observable<Boolean> {
Copy link
Owner

Choose a reason for hiding this comment

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

Why two arguments?

* it should be invisible. Finish the call with [endLoading].
*
* Example:
* val loading = startLoading(button1Tap, button2Tap).endLoading(response1, response2)
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting approach. It is very likely that response1 is created by applying flatMap on button1Tap. Can we create a complete example? Perhaps we can come up with a custom operator OR transform, something like:

(result, loading) = onButtonTap.load {
  return api.loadSomething()
}

@@ -38,6 +39,6 @@ install {

}
}
}
}.writeTo("build/poms/pom-default.xml")
Copy link
Owner

Choose a reason for hiding this comment

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

What does this do?

@@ -25,7 +25,7 @@

import rx.functions.Action0;

public class ItemViewModel implements ViewModel {
public class ItemViewModel extends ViewModel {
Copy link
Owner

Choose a reason for hiding this comment

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

Why switch to inheritance? Interfaces allow for more flexibility in building ViewModels.

@manas-chaudhari
Copy link
Owner

Also, in reference to the LoginViewModel code:

As subscription completes after detach, that would dispose all subscriptions. So, how do they get reattached on attach?

@manas-chaudhari manas-chaudhari force-pushed the master branch 9 times, most recently from 635161c to 4314f01 Compare June 15, 2018 13:03
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.

2 participants