-
Notifications
You must be signed in to change notification settings - Fork 3k
Add actions for setting TextView text. #85
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
| package rx.android.view; | ||
|
|
||
| import android.view.View; | ||
|
|
||
| import android.widget.TextView; | ||
| import rx.functions.Action1; | ||
|
|
||
| /** | ||
|
|
@@ -112,4 +112,28 @@ public static Action1<? super Boolean> setVisibility(View view) { | |
| public static Action1<? super Boolean> setVisibility(View view, int visibilityOnFalse) { | ||
| return new ViewActionSetVisibility(view, visibilityOnFalse); | ||
| } | ||
|
|
||
| /** | ||
| * Set the text of a {@link TextView} based on values emitted by an Observable. | ||
| */ | ||
| public static Action1<? super CharSequence> setText(final TextView textView) { | ||
| return new Action1<CharSequence>() { | ||
| @Override | ||
| public void call(CharSequence charSequence) { | ||
| textView.setText(charSequence); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just realized that your implementations are not as robust as the other ViewActions. They all return an instance of ViewAction1 which keeps a weak reference to the view and handles when it is null. The implementation should be like this: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh. Yeah we should match though I'm not a fan of the whole weak-reference-all-the-things movement. Also, all these others are not only using explicit top-level types for their actions but they're public... where's the facepalm emoji? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bah I'll fix it to be consistent. Nice catch. On Tue, Nov 25, 2014, 17:29 Jake Wharton notifications@github.com wrote:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #86 opened |
||
| } | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Set the text of a {@link TextView} based on values emitted by an Observable. | ||
| */ | ||
| public static Action1<? super Integer> setTextResource(final TextView textView) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, this is a return type not a parameter type. Explains all the comments on the issue lol. |
||
| return new Action1<Integer>() { | ||
| @Override | ||
| public void call(Integer integer) { | ||
| textView.setText(integer); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That does not exist in TextView There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, so it doesn't. Was thinking of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, that makes me question the utility of this helper. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, sweet. I knew I knew what I was talking about, just didn't realize they overloaded in this case. 👍 Android sucks at naming. |
||
| } | ||
| }; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| package rx.android.view; | ||
|
|
||
| import android.widget.TextView; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.mockito.Mockito; | ||
| import org.robolectric.RobolectricTestRunner; | ||
| import rx.subjects.PublishSubject; | ||
|
|
||
| import static org.mockito.Mockito.verify; | ||
|
|
||
| @RunWith(RobolectricTestRunner.class) | ||
| public class ViewActionSetTextTest { | ||
|
|
||
| private TextView textView; | ||
|
|
||
| @Before | ||
| public void setUp() { | ||
| textView = Mockito.mock(TextView.class); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSetsTextViewCharSequence() { | ||
| final PublishSubject<String> subject = PublishSubject.create(); | ||
| subject.subscribe(ViewActions.setText(textView)); | ||
|
|
||
| subject.onNext("Hello"); | ||
| verify(textView).setText("Hello"); | ||
|
|
||
| subject.onNext("World"); | ||
| verify(textView).setText("World"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSetsTextViewTextResource() { | ||
| final PublishSubject<Integer> subject = PublishSubject.create(); | ||
| subject.subscribe(ViewActions.setTextResource(textView)); | ||
|
|
||
| subject.onNext(1); | ||
| verify(textView).setText(1); | ||
|
|
||
| subject.onNext(3); | ||
| verify(textView).setText(3); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that you didn't add @param and @returns which are on every other method in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dalewking I tend to avoid them when they add nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine with me, just noticed the inconsistency with the rest of the file.