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

OperatorWeakBinding to not use WeakReferences anymore #1021

Merged
merged 3 commits into from
Apr 15, 2014

Conversation

mttkay
Copy link
Contributor

@mttkay mttkay commented Apr 6, 2014

related issues:
#1008
#1006
#979

I changed the samples to actually use bindActivity and bindFragment

@cloudbees-pull-request-builder

RxJava-pull-requests #944 FAILURE
Looks like there's a problem with this pull request

@mttkay
Copy link
Contributor Author

mttkay commented Apr 6, 2014

@samueltardieu @tehmou @zsxwing @mironov-nsk

Could you guys check this out? The discussion is in #1008

@samueltardieu
Copy link
Contributor

I think this is a good compromise:

  • the subscribers won't be garbage collected anymore before they are kept as a reference;
  • subscriptions will not be delivered if the component has been properly unsubscribed as documented;
  • subscriptions will not be delivered if the component has been freed or if the validator fails, so they won't arrive if the component has been replaced by another.

I am in favor of applying it.

@DylanSale
Copy link

I like this. I was already handling subscriptions in a superclass so the weak reference thing seemed unnecessary. I'd prefer it this way.

@niqo01
Copy link

niqo01 commented Apr 7, 2014

I followed both discussion #1008 and this one and for me this is still unclear. @mttkay I was wondering if you don't mine telling if

  • Implementation like this u2020 sample could leak?
  • If not, the purpose of bindActivity() and bindFragment()

@mttkay
Copy link
Contributor Author

mttkay commented Apr 8, 2014

Hi Nicolas,

ad u2020:
I'd have to look into the details, but it looks as if he unsubscribes in
onDetachedFromWindow. If that means releasing the context, then it won't
leak.

ad bindActivity/bindFragment.
There's two things here: these two helper methods, and the underlying
operator. The underlying operator binds an observable to a target object
using a predicate, releasing the reference to the target as soon as the
predicate starts to fail. The two helpers use this to bind observables to
Activities and Fragments respectively. More precisely:

bindActivity: schedules the observable's notifications on the main UI
thread, and adds a check that unsubscribes listeners and prevents
notifications from being forwarded as soon as the activity is about to get
finished (either by pressing back, or by calling finish()). It's just a
convenience method for calling observeOn(AndroidObservable.mainThread())
and checking manually in your observers whether the activity isFinishing())

bindFragment: same, just that the predicate here not only tests for the
host activity not getting finished, but also that the fragment is still
attached. It's just a safe guard in cases a notification arrives when the
fragment has already been detached from the activity. Since observers
usually access views and other context related fields, this would make them
crash.

Does that make sense?

@niqo01
Copy link

niqo01 commented Apr 8, 2014

@mttkay thanks a lot for this detailed answer.
u2020: Just to be sure, so unsubscribing releases any reference to the observer, right? In this sample the inner class Observer has a reference to the context via an adapter.

bindActivity bindFragment make sense.

@vinc3m1
Copy link

vinc3m1 commented Apr 8, 2014

So does this mean bindActivity and bindFragment effectively have the same behavior as the old fromFragment and fromActivity since we're no longer using WeakReferences? Other than naming and using lift, are there any advantages to bind* over from*?

@@ -24,88 +21,71 @@

private static final String LOG_TAG = "WeakBinding";

final WeakReference<R> boundRef;
private R boundRef;
private final Func1<? super R, Boolean> predicate;

public OperatorWeakBinding(R bound, Func1<? super R, Boolean> predicate) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a small suggestion. Since WeakReference is removed, I feel we need a new name rather than OperatorWeakBinding.

@zsxwing
Copy link
Member

zsxwing commented Apr 9, 2014

So does this mean bindActivity and bindFragment effectively have the same behavior as the old fromFragment and fromActivity since we're no longer using WeakReferences?

No. Actually the Observable returned by the old fromFragment and fromActivity can only be subscribed once.

@mttkay
Copy link
Contributor Author

mttkay commented Apr 9, 2014

@vinc3m1 on top of what @zsxwing said, they also tighten the validation on fragments a bit, since notifications generated while the activity is in the process of getting finished will not be forwarded anymore (just an extra safety net I guess)

@mttkay
Copy link
Contributor Author

mttkay commented Apr 9, 2014

Agree @zsxwing on the naming aspect. How about OperatorConditionalBinding? Does that make sense? other suggestions?

@vinc3m1
Copy link

vinc3m1 commented Apr 9, 2014

Ah, thanks for the clarification @mttkay @zsxwing! That single subscription thing should probably be documented right?

also OperatorConditionalBinding sounds good to me 👍

@zsxwing
Copy link
Member

zsxwing commented Apr 10, 2014

+1, OperatorConditionalBinding is good to me.

@benjchristensen
Copy link
Member

Looks like this is still in progress ... @mttkay please let me know when this is ready for merging.

@cloudbees-pull-request-builder

RxJava-pull-requests #951 SUCCESS
This pull request looks good

Having it in onViewCreated is fine, but misleading, since we don't need to reattach retained fragments. (the referenced object remains the same.)
@mttkay
Copy link
Contributor Author

mttkay commented Apr 12, 2014

@benjchristensen this is good to go.

@cloudbees-pull-request-builder

RxJava-pull-requests #952 SUCCESS
This pull request looks good

@benjchristensen
Copy link
Member

Thanks @mttkay

benjchristensen added a commit that referenced this pull request Apr 15, 2014
OperatorWeakBinding to not use WeakReferences anymore
@benjchristensen benjchristensen merged commit 463d913 into ReactiveX:master Apr 15, 2014
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.

None yet

8 participants