disposing render subscription after onDestroyView

[kioba]

Issue:

this might be an issue if an async event comes back and triggers a render between the onDestroyView and onDestroybecause the fragment view is already destroyed with the onDestroyView but we did not unsubscribe the render subscription.

Clearing the disposables in the fragments happening with the onDestroy event:

override fun onDestroy() { super.onDestroy() disposables.dispose() }

fragment lifecycle:
... onStop onDestroyView onDestroy

Reproducing the issue:
this could be reproduced if you call detach after binding the ViewModel and delaying the state observable.
detach: detach triggers the destruction of the view without calling onDestroy
delay: simulates the late render event

Solution suggestion:
Moving up the bind event to the onStart() and the unsubscription to the onStop() ?

[oldergod]

How about just moving disposables.dispose() before the super.onDestroy() ?

[kioba]

onDestroyView is a different lifecycle event for fragments, Moving the disposables.dispose() before the super.onDestroy() does not solve it.

calling detach on a fragment inside a fragmentTransaction actually triggers only onDestroyView without triggering onDestroy

Do you think the start() and stop() could be an issue?

[oldergod]

The only thing I can think of is that if we move those to start() and stop(), we probably would have to change how we trigger the initial intent.
Basically, the app is open, you switch to another app and come back: on start(), I don't want the view to reload stuff it should already have, I'd like it to reuse the latest view model available.

[kioba]

If the initial intent filter is used we take only the first initial intent and the rest won't be triggered as long as the ViewModel is not destroyed.

[oldergod]

Oh right, I forgot about that one. Would you like to submit a PR?

[kioba]

yes, happy to do it 👍

[nateridderman]

I came here wondering about the same issue. I think the proposed solution makes sense.

[nateridderman]

I was having trouble with binding in onStart and unbinding in onStop. When I leave the app and come back, it's not subscribed to the render calls. I'm not sure why, but switching to onActivityCreated/ onDestroyView worked for me, which is the scope of my ViewModel. The only reason I used onActivityCreated instead of onCreateView was because I needed the activity available for unrelated reasons.

[nateridderman]

Figured out my issue with onStart/onStop. I was calling disposables.dispose() instead of disposables.clear() in onStop, preventing it from accepting new disposables. @kioba lmk if you get stuck on a PR

[kioba]

@nateridderman the disposables.clear() solved an issue where returning to the list of TODOs the last added one was never shown. Thanks for the suggestions! 👍

[oldergod]

Thank you all.

[oldergod]

I had an after thought and I see the fix wasn't the right one, it also created #49 . Not sure how to deal with fragment lifecycle still; I'd rather move away from it maybe.

[kioba]

I will go and figure out why the refresh event is triggered.
What I can see is if we change the initial intent solution it won't trigger multiple times on the onStart:

  private fun initialIntent(): Observable<TasksIntent> {
    return Observable.fromCallable { TasksIntent.RefreshIntent(false, "INITIAL") }
        .cast(TasksIntent::class.java)
        .startWith(TasksIntent.InitialIntent)
  }

OnStart should not be called more than once before onStop and could not reproduce it. I am not

Not sure how to deal with fragment lifecycle still; I'd rather move away from it maybe.

Could you please give an example of what else could be improved? I am also interested in what is your idea behind the move away from fragments.

Happy to help solve these issues

EDIT: I could not reproduce the same issue with pullToRefresh.

[oldergod]

This very issue because caused by the weirdness of fragments' lifecycle is a good enough reason for me to not use them.
Since there's a 1:1 relationship to activities and fragments in this project, I think deleting fragments and only use activities is a good alternative.
Or maybe just let the activity, and not the fragment, manage the stream lifecycle.

[7-cat]

override fun render(state: xxxViewState) {
if (view == null) {
return
}
...
}
I'm not sure this help?