Skip to content
This repository has been archived by the owner on Mar 16, 2021. It is now read-only.

Add deliverLatestToView to rx2/RxTiPresenterUtils #137

Merged
merged 3 commits into from
Dec 4, 2017

Conversation

GrahamBorland
Copy link
Contributor

This adds an implementation of deliverLatestToView to rx2/RxTiPresenterUtils.

It uses only existing RxJava2 operators, rather than the OperatorSemaphore from the Rx1 utils. I did initially try to port OperatorSemaphore but it quickly became terrifying.

I have not implemented the other two (deliverLatestCacheToView and deliverToView) since I have no use for them yet, and I'm not really sure what they are meant to do anyway. (Their documentation is confusing - see #34).

Partial fix for #9.

/**
* Returns a transformer that will delay onNext, onError and onComplete emissions unless a view
* become available. getView() is guaranteed to be != null during all emissions. This
* transformer can only be used on application's main thread.
Copy link
Contributor

Choose a reason for hiding this comment

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

I must admit, when I wrote the rx1 version I copied most of the docs.
Actually the main thread restriction is not true it's only recommended to make sure the view is always non null. Attach/Detach happens on the main thread. When code executes on any other thread it's not guaranteed that those actions happen before the view attach/detach state changes. So it may happen that the emission of T happens after the view got detached.

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'm happy to rewrite the comment. The behaviour should be clear from the tests anyway, I hope.


@Test
public void testDeliverLatestToView_SingleItemViewComesAndGoes() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add this test to rx1 as well. I'm actually not sure this is working.

Copy link
Contributor

Choose a reason for hiding this comment

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

For that I've created a new issue. See #138

Copy link
Contributor

@passsy passsy left a comment

Choose a reason for hiding this comment

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

Thank @GrahamBorland!
Fine for me but I want @StefMa to look at this as well

Copy link
Contributor

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

Everything looks good to me 👍
Tests are fine 👍 Good job.

Can you pls adjust the comment (like @passsy mentioned)?
Then I'm fine with it and will happily merge it.

@GrahamBorland
Copy link
Contributor Author

Comment adjusted, thanks.

@passsy passsy merged commit 7556931 into GCX-HCI:master Dec 4, 2017
@GrahamBorland GrahamBorland deleted the feature/rx2-presenter-utils branch December 4, 2017 10:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

3 participants