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

Add recipe for auto disposing ViewModel #254

Merged
merged 2 commits into from
Oct 8, 2018

Conversation

ShaishavGandhi
Copy link
Collaborator

Description: This is an initial pass at addressing #253 . The meat of the stuff is in AutoDisposeViewModel where we automatically dispose any streams in onCleared. The behavior is modeling AndroidLifecycleProvider and such. The only way to use it meaningfully is to extend this base class. Unfortunately, I don't see a way of using something like ViewModelLifecyleProvider.from(viewModel). Let me know if that implementation looks good.

The rest of the stuff is around constructing a sample that shows the use of the AutoDisposeViewModel. Again, I haven't invested that much time into it, just to see if this would be a good fit for the sample. Right now we just delay the emission of a String to simulate what a long network request would look like. The essence of the concept is that since you're subscribing in the ViewModel and then just passing on the results using a BehaviorRelay or `LiveData, even if config changes, your API call still continues (something that wouldn't if you had subscribed to the same observable in your Activity/Fragment since the onDestroy would cancel the call).

If this looks good to you, we can use mockwebserver from OkHttp to throttle and simulate a network request and make it clearer.

Related issue(s): #253

Signed-off-by: shaishavgandhi05 <shaishgandhi@gmail.com>
Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this, thanks!

@ZacSweers
Copy link
Collaborator

Let's do the throttle as well!

@ShaishavGandhi
Copy link
Collaborator Author

Sounds good! I will update the sample with the throttling.

How do you feel about adding this as a module that AutoDispose exposes (just like android-arch-component)? It's a small class but I've used it across several projects already and I have to copy paste it everywhere. Would be cool just to get it with an artifact!

@ShaishavGandhi
Copy link
Collaborator Author

ShaishavGandhi commented Oct 1, 2018

So I took an initial look at doing it via mockwebserver and it looks like I'll still have to setup a bunch of stuff with Retrofit etc. since mockwebserver will simulate a network response but it needs the network to be called. That seems like a lot of boilerplate for a working sample 😄

I'm alternatively thinking of adding a request progress bar which simulates the downloading of packets of data. We can easily do that by throttling each emission via RxJava and showing the progress in a ProgressBar on the Activity.

What do you think?

@ZacSweers
Copy link
Collaborator

That works too, but I don't mind having a nontrivial example as well

@ZacSweers
Copy link
Collaborator

Also re: artifact - let's hold off for now and revisit after 1.0. Having a recipe is a good start

@ZacSweers
Copy link
Collaborator

Out of curiosity, should we look at borrowing anything from here? https://gist.github.com/LouisCAD/58d3017eedb60ce00721cb32a461980f

@ShaishavGandhi
Copy link
Collaborator Author

Sorry for the delay here in updating the pull request. I've been trying to workaround some mockwebserver issues with running in a non-test environment and it's not playing well. I should have something soon, either with mockwebserver or the download progress thing.

Regarding the gist: I'm not terribly familiar with coroutines but as I understand in that gist, it lets you add cancellations to coroutines taking advantage of Lifecycle and adding the GenericLifecycleObserver to get callbacks on lifecycle changes. This seems useful when you're using coroutines in Fragments or Activities.

As far as ViewModel goes, the general advice from Yigit and co. has been not to reference any LifecycleOwner in the ViewModel. It's susceptible to leaks since the ViewModel will often outlive the LifecycleOwner (especially in orientation changes).

The other option is to potentially have a ViewModel implement a LifecycleOwner. It says in the docs that you can make your custom components implement LifecycleOwner to handle lifecycle changes. However, I don't think it'd be appropriate to do that considering that the ViewModel only really has two events (created and cleared/destroyed).

Signed-off-by: shaishavgandhi05 <shaishgandhi@gmail.com>
@ShaishavGandhi
Copy link
Collaborator Author

I've updated the sample with a download progress indicator. Somehow I couldn't get mockwebserver to play well in a non test environment. It kept complaining of ports and such. Finally gave up and came up with this instead. I've added a bunch of documentation for clarity. Here's a screenshot of the screen for reference:

device-2018-10-05-153435

Copy link
Collaborator

@ZacSweers ZacSweers 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

@ZacSweers ZacSweers merged commit 1ad5492 into uber:master Oct 8, 2018
@ShaishavGandhi ShaishavGandhi deleted the sg/autodisposing-viewmodel branch October 8, 2018 15:18
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