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 bindView implementation and tests #12

Merged
merged 7 commits into from
Aug 29, 2015

Conversation

ZacSweers
Copy link
Contributor

Also updated a couple dependencies along the way

@ZacSweers
Copy link
Contributor Author

@dlew let me know if you want me to add tests for OnSubscribeViewDetachedFromWindowFirst. I wasn't sure how necessary it is since it's a fairly simple class.

@dlew
Copy link
Contributor

dlew commented Aug 8, 2015

Was this mostly taken from the old RxAndroid repo?

I think it looks good, but I'd like to take it a slightly different direction (implementation-wise). Right now it's lacking a bit in two ways:

  1. We have ViewEvent.ATTACH but never actually emit it.
  2. Our OnSubscribe has a bunch of logic in it that can be replicated from other operators.

I'd like it if we had a solution that's composed of two steps:

  1. An RxBinding-esque way to get Observable<ViewEvent> from a View, completely separate from anything else. (Perhaps maybe even just in RxBinding itself, then we can make this depend on that library).
  2. Combine that with takeUntil to automatically unsubscribe after detachment.

@ZacSweers
Copy link
Contributor Author

Yup, mostly taken from there. If you're open to depending on RxBinding though, I
can definitely do the latter part. RxBinding doesn't have attach/detach
events yet, but Jake says he wants it so I could pull request that over
there first and then update this.

That's also be nice because then there wouldn't be any overlap between view
event implementations.
On Sat, Aug 8, 2015 at 10:23 AM Daniel Lew notifications@github.com wrote:

Was this mostly taken from the old RxAndroid repo?

I think it looks good, but I'd like to take it a slightly different
direction (implementation-wise). Right now it's lacking a bit in two ways:

  1. We have ViewEvent.ATTACH but never actually emit it.
  2. Our OnSubscribe has a bunch of logic in it that can be replicated
    from other operators.

I'd like it if we had a solution that's composed of two steps:

An RxBinding-esque way to get Observable from a View,
completely separate from anything else. (Perhaps maybe even just in
RxBinding itself, then we can make this depend on that library).
2.

Combine that with takeUntil to automatically unsubscribe after
detachment.


Reply to this email directly or view it on GitHub
#12 (comment).

@dlew
Copy link
Contributor

dlew commented Aug 9, 2015

I don't mind depending on RxBinding for now. Worst-case scenario we could always spit it out as another module later. But honestly I think most people using RxAndroid will want RxBinding anyways.

@ZacSweers
Copy link
Contributor Author

Agreed. Opened a PR there for attach events: JakeWharton/RxBinding#76

@ZacSweers
Copy link
Contributor Author

That PR is in RxBinding now, so I'll update this after its next release! Should I close it for now or just leave it open until then?

@dlew
Copy link
Contributor

dlew commented Aug 10, 2015

Whatever you want to do. It doesn't much matter to me.

@ZacSweers
Copy link
Contributor Author

Alright, I'll leave it open for now
On Mon, Aug 10, 2015 at 6:17 AM Daniel Lew notifications@github.com wrote:

Whatever you want to do. It doesn't much matter to me.


Reply to this email directly or view it on GitHub
#12 (comment).

@dlew
Copy link
Contributor

dlew commented Aug 23, 2015

Now that there's a new version of RxBinding out, would this be possible to do?

@ZacSweers
Copy link
Contributor Author

Yup, I'll update it when I get the chance today or tomorrow.

On Sun, Aug 23, 2015, 7:52 AM Daniel Lew notifications@github.com wrote:

Now that there's a new version of RxBinding out, would this be possible to
do?


Reply to this email directly or view it on GitHub
#12 (comment).

@ZacSweers
Copy link
Contributor Author

@dlew rebased and updated with use of RxBinding.detaches()

* @return a reusable {@link Observable.Transformer} that unsubscribes the source during the View lifecycle
*/
public static <T> Observable.Transformer<T, T> bindView(final View view) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Extra newlines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You want more newlines or to remove this? I was just matching the pattern I thought other functions were using here

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove - the others don't have an extra newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gah, fixed in b4c72bb

I misread code above it and thought there was a space:

screenshot 2015-08-25 14 28 31

@dlew
Copy link
Contributor

dlew commented Aug 24, 2015

We should add a sample at some point, doesn't have to be here, but if not here then we'll create an issue to track.

Before, this would only work for "Object"s and not subclasses, such as Strings. This fixes that with some proper generics and adds a test for it
* @param lifecycle the lifecycle sequence of a View
* @return a reusable {@link Observable.Transformer} that unsubscribes the source during the View lifecycle
*/
public static <T, E> Observable.Transformer<T, T> bindView(final Observable<E> lifecycle) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be <E> or <? extends E>? I only ask because we're sort of copying takeUntil and that's what they do (again, not exactly sure why since I suck at generics).

Choose a reason for hiding this comment

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

Mimic Observable#takeUntil and use <? extends E>.

Btw, this is the difference between the three:

  • Observable<E> an Observable that only outputs E and exactly that.
  • Observable<? extends E> an Observable that outputs anything with an upperbound of E (so any descendant of E or E itself).
  • Observable<?> an Observable that outputs anything, can't use that here because the Operator (Transformer) expects a type!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in e8fdbdb

dlew added a commit that referenced this pull request Aug 29, 2015
Add bindView implementation and tests
@dlew dlew merged commit 308093e into trello-archive:master Aug 29, 2015
@ZacSweers ZacSweers deleted the z/bindview branch August 30, 2015 00:15
@SleeplessByte
Copy link

👍

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.

3 participants