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 sample operator to Flow, which can sample a Flow with the emissions of another Flow #1153

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

adibfara
Copy link

@adibfara adibfara commented Apr 28, 2019

The aim of this PR is to bring the ability to sample a flow with the emissions from another flow. I've a extension methods to Flow:

Flow<T>.sampleBy(sampler:ReceiveChannel<R>) which samples a flow whenever the receive channel emits.

Notes:

  1. The current sample has been refactored to use the sampleBy method with a simple flow of an interval.
  2. All the current test cases for sample(period) have been added for both the operators (in a new file called SampleByTest).
  3. The operator can still be called sample, but I think sampleBy is more readable/understandable.

Example of sampleBy with another Flow:

flow {
    repeat(10) {
        emit(it)
        delay(50)
    }
}.sampleBy(flow {
    repeat(10) {
        delay(100)
        emit(it)
    }
})

produces 0, 2, 4, 6, 8.

@fvasco
Copy link
Contributor

fvasco commented Apr 28, 2019

Each ReceiveChannel can be converted to a Flow, why overload this operator for both types?
Is Flow<T>.sampleBy(sampler: Flow<R>): Flow<T> enough?

@adibfara
Copy link
Author

@fvasco That makes sense. I removed the channel operator with its unit tests since they can easily be converted to flows.

@adibfara adibfara changed the title Add sampleBy operator to Flow, to be able to sample a Flow with another Flow or ReceiveChannel Add sampleBy operator to Flow, to be able to sample a Flow with another Flow Apr 28, 2019
@adibfara adibfara changed the title Add sampleBy operator to Flow, to be able to sample a Flow with another Flow Add sampleBy operator to Flow, which can sample a Flow with the emissions of another Flow Apr 28, 2019
@elizarov
Copy link
Contributor

Can you, please, clarify what is the use-case for sampleBy? In what kind of circumstance and applications it might be needed?

@adibfara
Copy link
Author

sampleBy operator is the more generalized version of the sample operator, which is not bound to time. The user of this operator can specify any kind of flow emission for the sampling to work. Its application is synchronizing (and restrict) events from an observable source, with another.
image
RxJava implementation of sample

One simple use case for this operator would be the Refresh Button in Gmail when there's new mails available:

  1. A flow which emits data whenever the data is updated

  2. A Refresh button that exposes a flow for its click events.

  3. We want our UI to be updated according to the latest received data, whenever the user presses the refresh button.

These two flows cannot be:

  1. combinedLatest with each other, since If the user has clicked on the refresh button, on every emission of data afterwards, the page would suddenly get updated.

  2. zip with each other, aside from the previous problem, if the user clicks on the refresh button multiple times, the UI would not get updated if the data is not received in the mean time..

# Conflicts:
#	binary-compatibility-validator/reference-public-api/kotlinx-coroutines-core.txt
#	kotlinx-coroutines-core/common/src/flow/operators/Delay.kt
- remove old sample implementation
- remove old sample unit tests and add them as the new ones
@adibfara adibfara changed the title Add sampleBy operator to Flow, which can sample a Flow with the emissions of another Flow Add sample operator to Flow, which can sample a Flow with the emissions of another Flow Jun 8, 2019
@adibfara
Copy link
Author

adibfara commented Jun 8, 2019

I renamed the operator to sample, since sampleBy was kind of a new name (it made no sense, since If we are doing that, other operators such as flatMap should become flatMapBy).

Also, I've updated the code to use the new flowScope and renamed the the unit test functions to match the original ones.

- remove old sample implementation
- remove old sample unit tests and add them as the new ones
@adibfara
Copy link
Author

adibfara commented Jun 9, 2019

@elizarov does this PR need further improvements? I'm asking since it becomes a stale/conflicting branch after a few days.

@elizarov
Copy link
Contributor

elizarov commented Jun 9, 2019

We have not reached consensus yet on whether we should add sampleBy to the core library in the first place, so please hold on.

@qwwdfsad qwwdfsad force-pushed the develop branch 3 times, most recently from 4a49830 to aff8202 Compare March 10, 2020 17:27
adibfara and others added 6 commits July 12, 2020 08:18
Update core coroutines
# Conflicts:
#	kotlinx-coroutines-core/api/kotlinx-coroutines-core.api
#	kotlinx-coroutines-core/common/src/flow/operators/Delay.kt
#	kotlinx-coroutines-core/common/test/flow/operators/SampleTest.kt
# Conflicts:
#	kotlinx-coroutines-core/api/kotlinx-coroutines-core.api
# Conflicts:
#	kotlinx-coroutines-core/api/kotlinx-coroutines-core.api
#	kotlinx-coroutines-core/common/src/flow/operators/Delay.kt
@adibfara
Copy link
Author

@elizarov Since this PR is still open (which is appreciated, after 4 years 😅), I've updated it with master so we can decide on merging or closing it.
Please do let me know if it needs anything else to make it reach the decision.

Just to reiterate this PR's purpose, it generalizes the sample operator to accept a flow as its input (instead of just a time), so users can sample their flows with other flows (e.g. emit my last value when the other emits).

Thanks.

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.

4 participants