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 and MvRx #97

Closed
mradzinski opened this issue Sep 24, 2018 · 21 comments
Closed

Conductor and MvRx #97

mradzinski opened this issue Sep 24, 2018 · 21 comments

Comments

@mradzinski
Copy link

mradzinski commented Sep 24, 2018

Hey, Just wondering how crazy it is for me to attempt integratin MvRx with Conductor. I would be more than happy to provide a PR/separate module, but I first need to know how feasible this is.

Is MvRx based on delegation? Haven't check the internals yet to make sure of this nor I've fully tested MvRx myself, but if its delegation based I should be able to use MvRx on Controllers (or any sort of Views) instead of Fragments, right?

Any tips are very much appreciated :)

Thanks!

@gpeal
Copy link
Collaborator

gpeal commented Sep 24, 2018

@mradzinski I haven't used Conductor so it's hard to say. What do you me by delegation exactly? We use property delegates for some things.

@mradzinski
Copy link
Author

mradzinski commented Sep 24, 2018

Hey @gpeal, sorry, I was sleepy AF last night and didn't take enough time to give you some more context 😅 .

To simply put it, Conductor is a library to build View-based Android applications. You don't use Fragments but Controllers (which at the very end are backed up by a Fragment) which have their own specific lifecycle (very similar to the one of Fragment but a bit more predictable/simplified).

My question could be summarized into: If I have a Controller (or any View for what matters) implement MvRxView, should that suffice for that Controller to become the View of MVVM (or of MvRX in this context)? That's what's basically happening under the hood of a BaseMvRxFragment for example.

I know there're a bunch of extensions which I would've to change a bit here and there, but I guess the most important part for this to work is to have a Controller implelent MvRxView, right?

Thanks!

@gpeal
Copy link
Collaborator

gpeal commented Sep 25, 2018

@mradzinski A lot of MvRx just takes a LifecycleOwner as a parameter so if your View implements that, it may just work. Feel free to try it!

@mradzinski
Copy link
Author

mradzinski commented Sep 26, 2018

Hey @gpeal, Correct me if I'm wrong, but I'm not entirely sure implementing LifecycleOwner will be enough mostly because MvRx is prepared to be used with Fragments and Activities but not with an application made out of something entirely different such as Conductor Controllers. Although I can make a Conductor's Controller (or any View with a life cycle) implement LifecycleOwner and ViewModelStoreOwner pretty much easily, my biggest concern comes from the fact that there's a big dependency between MvRx and Fragments.

For example: as far as I know activityViewModel and fragmentViewModel will simply not work in other scenarios other than what their names suggest. Passing arguments to the initial state through KEY_ARG will also not work (check MvRxViewModelStore.restoreViewModels). MvRxViewModelProvider also assumes Activities or Fragments. And I guess there's a big etc.

I don't think any View implementing LifecycleOwner as you mentioned suffice for MvRx to work. I think it takes a bunch more than that to have something working. Again, I might be completely wrong here, so I'm just looking up for a bit of guidance on the internals of MvRx and what would need to be implemented if deviating from the usage of Fragments.

Cheers and thanks again!

@gpeal
Copy link
Collaborator

gpeal commented Sep 30, 2018

@mradzinski To do this correctly might require you to clone the project and start poking around. I'm happy to answer any questions you have though. Just @gpeal me on replies here so I get a notification for it.

@mradzinski
Copy link
Author

I'm already one step ahead and began poking around last Friday @gpeal. I'll be OOO next week, so probably I'll be able to invest some time into this.

I'll let you know in case I have any questions, but so far the advantages of a healthy, well structured codebase are paying off and the process has been very straightforward (thanks for that guys).

@gpeal
Copy link
Collaborator

gpeal commented Dec 12, 2018

@mradzinski Any update here?

@wugeorgeq
Copy link

We've successfully integrated conductor with viewmodel, primarily utilizing this https://github.com/miquelbeltran/conductor-viewmodel/blob/master/conductor-viewmodel/src/main/java/work/beltran/conductorviewmodel/ControllerLifecycleOwner.java

I'm toying with the idea of forking MvRx and implementing a base Controller class in similar fashion as MvRx but realized that you guys @gpeal are manually saving the viewmodels. Afaik, using the factory with the ViewModelProvider would mean this is done automatically, so I'm wondering what the motivation was behind manually persisting the viewmodels

@gpeal
Copy link
Collaborator

gpeal commented Dec 19, 2018

@wugeorgeq We do use the normal ViewModelProvider but we extend/intercept it so we can properly handle things like process restoration with @PersistState

https://github.com/airbnb/MvRx/wiki#restoration-in-a-new-process

@mradzinski
Copy link
Author

@gpeal Sorry, got quite busy with some projects. Haven't lost interest, though I need to find some spare time to toy with this. I'll keep you guys posted.

@wugeorgeq
Copy link

Ah my bad! I wasn't aware of difference between config change and process death in ViewModels and incorrectly assumed that the library was doing more than it needed to. Embarrassing D:

@gpeal
Copy link
Collaborator

gpeal commented Jan 2, 2019

@mradzinski I'm going to close this to keep the backlog clean but if you want to write a wiki page on how to use MvRx with conductor or need something else specific, feel free to open a new issue!

@gpeal gpeal closed this as completed Jan 2, 2019
@mradzinski
Copy link
Author

@gpeal I've resumed this attempt and found out its not going to be possible given the current project state. Having ViewModelContext() sealed (link) prevents creating a new ViewModelContext which is needed as part of MvRxViewModelProvider.get().

Is there a reason why ViewModelContext needs to be sealed? Just wondering how possible it is to make it open and hence extensible.

@gpeal
Copy link
Collaborator

gpeal commented Feb 4, 2019

@mradzinski Can you use ActivityViewModelContext for conductor?

@mradzinski
Copy link
Author

@gpeal yes, but that wouldn't mean the VM would be scoped to the Activity instead of the Controller? (which acts as a Fragment)

@mradzinski
Copy link
Author

I forked MvRx and I'm pretty close to achieve the integration @gpeal, the only problem I have so far is that VM's are not surviving config changes, hence a new instance is being created every time with an initial state. Any idea why that might be happening? I'm happy to push my changes to the forked version (bare in mind it's a proof of concept and not even close to a final implementation).

@gpeal
Copy link
Collaborator

gpeal commented Feb 5, 2019

@mradzinski ViewModelProviders from Jetpack actually does the retaining so you might want to look into that.
The ViewModelContext is just created to give you context to use for DI/VM creation. The actual scope of your ViewModel depends on the ViewModelProvider you use.

@mradzinski
Copy link
Author

mradzinski commented Feb 5, 2019

@gpeal Isn't MvRx using it's own implementation of a ViewModelProvider, ViewModelStore which takes care of restoring the VM state throughout args and its own VM factory?

@mradzinski
Copy link
Author

So, I managed to make everything work, there was some lifecycle issues on my end. Much to my regret there's no actual way to make this an extension to MvRx. MvRx is currently tied to Activities and Fragments, and I had to modify plenty of internal methods and classes to make this to work.

@gpeal If at any point you guys at Airbnb become interested in Conductor, or simply MvRx evolves allowing some sort of extensibility then ping me here and I'll PR all this.

@manueldidonna
Copy link

@mradzinski it would be great if you shared your hard work as a library

@mradzinski
Copy link
Author

mradzinski commented Feb 5, 2019

@manueldidonna Like I said, I'll not be sharing it as a library due to how hard it'll be to keep it up to date with MvRx (it would need to be an exact clone of this repo with a few methods here and there).

You can check my implementation here though: https://github.com/mradzinski/MvRx/tree/matias/conductor-integration

You can check the latest commit on that branch to understand better why this can't be made a module itself.

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

No branches or pull requests

4 participants