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

Implement view attach events + tests #76

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,41 @@ package com.jakewharton.rxbinding.view
import android.view.DragEvent
import android.view.MotionEvent
import android.view.View
import rx.Observable
import com.jakewharton.rxbinding.internal.Functions
import rx.Observable
import rx.functions.Action1
import rx.functions.Func0
import rx.functions.Func1

/**
* Create an observable of timestamps for clicks on `view`.
* Create an observable which emits on `view` attach events. The emitted value is
* unspecified and should only be used as notification.
*
* *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)
Copy link
Owner

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hindsight 🤔


/**
* Create an observable of attach and detach events on `view`.
*
* *Warning:* The created observable keeps a strong reference to `view`. Unsubscribe
* to free this reference.
*/
public inline fun View.attachEvents(): Observable<ViewAttachEvent> = RxView.attachEvents(this)

/**
* Create an observable which emits on `view` detach events. The emitted value is
* unspecified and should only be used as notification.
*
* *Warning:* The created observable keeps a strong reference to `view`. Unsubscribe
* to free this reference.
*/
public inline fun View.detaches(): Observable<Any> = RxView.detaches(this)

/**
* Create an observable which emits on `view` click events. The emitted value is
* unspecified and should only be used as notification.
*
* *Warning:* The created observable keeps a strong reference to `view`. Unsubscribe
* to free this reference.
Expand Down Expand Up @@ -108,7 +135,8 @@ public inline fun View.focusChanges(): Observable<Boolean> = RxView.focusChanges
public inline fun View.focusChangeEvents(): Observable<ViewFocusChangeEvent> = RxView.focusChangeEvents(this)

/**
* Create an observable of timestamps for long-clicks on `view`.
* Create an observable which emits on `view` long-click events. The emitted value is
* unspecified and should only be used as notification.
*
* *Warning:* The created observable keeps a strong reference to `view`. Unsubscribe
* to free this reference.
Expand All @@ -119,7 +147,8 @@ public inline fun View.focusChangeEvents(): Observable<ViewFocusChangeEvent> = R
public inline fun View.longClicks(): Observable<Any> = RxView.longClicks(this)

/**
* Create an observable of timestamps for clicks on `view`.
* Create an observable which emits on `view` long-click events. The emitted value is
* unspecified and should only be used as notification.
*
* *Warning:* The created observable keeps a strong reference to `view`. Unsubscribe
* to free this reference.
Expand Down
1 change: 1 addition & 0 deletions rxbinding/src/androidTest/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package="com.jakewharton.rxbinding">

<application>
<activity android:name=".view.RxViewAttachTestActivity"/>
<activity android:name=".widget.RxAdapterViewTestActivity"/>
<activity android:name=".widget.RxRatingBarTestActivity"/>
<activity android:name=".widget.RxSeekBarTestActivity"/>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
package com.jakewharton.rxbinding.view;

import android.app.Instrumentation;
import android.support.test.InstrumentationRegistry;
import android.support.test.rule.ActivityTestRule;
import android.support.test.runner.AndroidJUnit4;
import android.view.View;
import android.widget.FrameLayout;

import com.jakewharton.rxbinding.RecordingObserver;

import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;

import rx.Subscription;
import rx.android.schedulers.AndroidSchedulers;

import static com.google.common.truth.Truth.assertThat;
import static com.jakewharton.rxbinding.view.ViewAttachEvent.Kind.ATTACH;
import static com.jakewharton.rxbinding.view.ViewAttachEvent.Kind.DETACH;

@RunWith(AndroidJUnit4.class)
public final class RxViewAttachTest {
@Rule public final ActivityTestRule<RxViewAttachTestActivity> activityRule =
new ActivityTestRule<>(RxViewAttachTestActivity.class);

private final Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation();
private FrameLayout parent;
private View child;

@Before public void setUp() {
RxViewAttachTestActivity activity = activityRule.getActivity();
parent = activity.parent;
child = activity.child;
}

@Test public void attaches() {
RecordingObserver<Object> o = new RecordingObserver<>();
Subscription subscription = RxView.attaches(child)
.subscribeOn(AndroidSchedulers.mainThread())
.subscribe(o);
o.assertNoMoreEvents(); // No initial value.
Copy link
Owner

Choose a reason for hiding this comment

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

We are able to query an initial value from the view. The question is should we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the test or the library itself? I thought about checking against it in the initial implementation of the Rxlifecycle PR, but isAttachedToWindow() is API 19+...

Copy link
Owner

Choose a reason for hiding this comment

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

Oh sweet. That rules it out then!


instrumentation.runOnMainSync(new Runnable() {
@Override public void run() {
parent.addView(child);
}
});
assertThat(o.takeNext()).isNotNull();
instrumentation.runOnMainSync(new Runnable() {
@Override public void run() {
parent.removeView(child);
}
});
o.assertNoMoreEvents();

subscription.unsubscribe();

instrumentation.runOnMainSync(new Runnable() {
@Override public void run() {
parent.addView(child);
parent.removeView(child);
}
});
o.assertNoMoreEvents();
}

@Test public void attachEvents() {
RecordingObserver<ViewAttachEvent> o = new RecordingObserver<>();
Subscription subscription = RxView.attachEvents(child)
.subscribeOn(AndroidSchedulers.mainThread())
.subscribe(o);
o.assertNoMoreEvents(); // No initial value.

instrumentation.runOnMainSync(new Runnable() {
@Override public void run() {
parent.addView(child);
}
});
assertThat(o.takeNext().kind()).isEqualTo(ATTACH);
instrumentation.runOnMainSync(new Runnable() {
@Override public void run() {
parent.removeView(child);
}
});
assertThat(o.takeNext().kind()).isEqualTo(DETACH);

subscription.unsubscribe();

instrumentation.runOnMainSync(new Runnable() {
@Override public void run() {
parent.addView(child);
parent.removeView(child);
}
});
o.assertNoMoreEvents();
}

@Test public void detaches() {
RecordingObserver<Object> o = new RecordingObserver<>();
Subscription subscription = RxView.detaches(child)
.subscribeOn(AndroidSchedulers.mainThread())
.subscribe(o);
o.assertNoMoreEvents(); // No initial value.

instrumentation.runOnMainSync(new Runnable() {
@Override public void run() {
parent.addView(child);
}
});
o.assertNoMoreEvents();
instrumentation.runOnMainSync(new Runnable() {
@Override public void run() {
parent.removeView(child);
}
});
assertThat(o.takeNext()).isNotNull();

subscription.unsubscribe();

instrumentation.runOnMainSync(new Runnable() {
@Override public void run() {
parent.addView(child);
parent.removeView(child);
}
});
o.assertNoMoreEvents();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.jakewharton.rxbinding.view;

import android.app.Activity;
import android.os.Bundle;
import android.view.View;
import android.widget.FrameLayout;

public final class RxViewAttachTestActivity extends Activity {
FrameLayout parent;
View child;

@Override protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
parent = new FrameLayout(this);
child = new View(this);
setContentView(parent);
}
}
49 changes: 45 additions & 4 deletions rxbinding/src/main/java/com/jakewharton/rxbinding/view/RxView.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
import android.view.DragEvent;
import android.view.MotionEvent;
import android.view.View;
import rx.Observable;

import com.jakewharton.rxbinding.internal.Functions;

import rx.Observable;
import rx.functions.Action1;
import rx.functions.Func0;
import rx.functions.Func1;
Expand All @@ -19,7 +21,44 @@
*/
public final class RxView {
/**
* Create an observable of timestamps for clicks on {@code view}.
* Create an observable which emits on {@code view} attach events. The emitted value is
* unspecified and should only be used as notification.
* <p>
* <em>Warning:</em> The created observable keeps a strong reference to {@code view}. Unsubscribe
* to free this reference.
*/
public static Observable<Object> attaches(View view) {
checkNotNull(view, "view == null");
return Observable.create(new ViewAttachesOnSubscribe(view, true));
}

/**
* Create an observable of attach and detach events on {@code view}.
* <p>
* <em>Warning:</em> The created observable keeps a strong reference to {@code view}. Unsubscribe
* to free this reference.
*/
@CheckResult
public static Observable<ViewAttachEvent> attachEvents(View view) {
Copy link
Owner

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

But I'm fine leaving it out here for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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 bb30a23

checkNotNull(view, "view == null");
return Observable.create(new ViewAttachEventOnSubscribe(view));
}

/**
* Create an observable which emits on {@code view} detach events. The emitted value is
* unspecified and should only be used as notification.
* <p>
* <em>Warning:</em> The created observable keeps a strong reference to {@code view}. Unsubscribe
* to free this reference.
*/
public static Observable<Object> detaches(View view) {
checkNotNull(view, "view == null");
return Observable.create(new ViewAttachesOnSubscribe(view, false));
}

/**
* Create an observable which emits on {@code view} click events. The emitted value is
* unspecified and should only be used as notification.
* <p>
* <em>Warning:</em> The created observable keeps a strong reference to {@code view}. Unsubscribe
* to free this reference.
Expand Down Expand Up @@ -153,7 +192,8 @@ public static Observable<ViewFocusChangeEvent> focusChangeEvents(View view) {
}

/**
* Create an observable of timestamps for long-clicks on {@code view}.
* Create an observable which emits on {@code view} long-click events. The emitted value is
* unspecified and should only be used as notification.
* <p>
* <em>Warning:</em> The created observable keeps a strong reference to {@code view}. Unsubscribe
* to free this reference.
Expand All @@ -168,7 +208,8 @@ public static Observable<Object> longClicks(View view) {
}

/**
* Create an observable of timestamps for clicks on {@code view}.
* Create an observable which emits on {@code view} long-click events. The emitted value is
* unspecified and should only be used as notification.
* <p>
* <em>Warning:</em> The created observable keeps a strong reference to {@code view}. Unsubscribe
* to free this reference.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package com.jakewharton.rxbinding.view;

import android.content.Context;
import android.support.annotation.NonNull;
import android.view.View;

/**
* A view attach event on a view.
* <p>
* <strong>Warning:</strong> Instances keep a strong reference to the view. Operators that
* cache instances have the potential to leak the associated {@link Context}.
*/
public final class ViewAttachEvent extends ViewEvent<View> {

public enum Kind {
ATTACH, DETACH
}

public static ViewAttachEvent create(@NonNull View view, Kind kind) {
return new ViewAttachEvent(view, kind);
}

private final Kind kind;

private ViewAttachEvent(View view, Kind kind) {
super(view);
this.kind = kind;
}

public Kind kind() {
return kind;
}

@Override public boolean equals(Object o) {
if (o == this) return true;
if (!(o instanceof ViewAttachEvent)) return false;
ViewAttachEvent other = (ViewAttachEvent) o;
return other.view() == view()
&& other.kind() == kind();
}

@Override public int hashCode() {
int result = 17;
result = result * 37 + view().hashCode();
result = result * 37 + kind().hashCode();
return result;
}

@Override public String toString() {
return "ViewAttachEvent{view="
+ view()
+ ", kind="
+ kind()
+ '}';
}
}
Loading