Implement view attach events + tests #76
Conversation
As brought up in JakeWharton#70
| public static final int DETACH = 1; | ||
|
|
||
| @IntDef({ATTACH, DETACH}) | ||
| public @interface Kind {} |
There was a problem hiding this comment.
All other events use an enum. If people want it changed that's a topic for a separate issue and should be done all at once. Switch this guy over.
There was a problem hiding this comment.
Oops, yeah I should've read the README. 0114616
| * to free this reference. | ||
| */ | ||
| @CheckResult | ||
| public static Observable<ViewAttachEvent> attachEvents(View view) { |
There was a problem hiding this comment.
Does this deserve a pair of siblings?
public static Observable<Object> attaches(View view) { .. }
public static Observable<Object> detaches(View view) { .. }These are zero-allocation operators which appeals to me from an efficiency perspective, but I definitely struggle with the duplication they require on a code-size front (for those not using ProGuard, who I probably shouldn't care about and actually includes myself).
There was a problem hiding this comment.
I wondered about that, but wasn't sure if it was worth the trouble when filter exists. Maybe hold off for now keep the door open if anyone requests it?
There was a problem hiding this comment.
I mean, you can claim filter for every binding we have but then any action you take has to allocate a wrapper object and for particularly chatty events that cost will start to add up as your compose more and more bindings together. My struggle between these here is real.
There was a problem hiding this comment.
But I'm fine leaving it out here for now.
There was a problem hiding this comment.
I have an idea of how it could be done in a simple way, let me tinker for a min and get back to you
There was a problem hiding this comment.
Ehh idea didn't pan out (was thinking about generifying ViewAttachEventOnSubscribe).
Fair point on the wrapper allocations, though. Thinking of the RxLifecycle use case, it would actually be perfect for that, so I'll add those siblings. Should only be one extra *OnSubscribe class, so nothing too crazy with code duplication.
| * *Warning:* The created observable keeps a strong reference to `view`. Unsubscribe | ||
| * to free this reference. | ||
| */ | ||
| public inline fun View.attaches(): Observable<Any> = RxView.attaches(this) |
There was a problem hiding this comment.
I'm sad we can't make this Unit in Kotlin. Maybe we should write all the code in Kotlin and generate the Java bindings instead...
|
Squashed into two commits and merged. Thanks! |
|
Always happy to help! Looking forward to using this in the other PR and get back our view lifecycle binding 😀 |
|
Just noticed I forgot to add CheckResult annotations to the sibling methods... #82 |
As brought up in #70
@dlew and I talked in trello-archive/RxLifecycle#12 about it being nice if we could back its
bindViewimplementation under the hood with the one you suggested in #70. This would also avoid the event class clashing if one uses both libraries.