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

tickerFlow implementation #1908

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

tickerFlow implementation #1908

wants to merge 5 commits into from

Conversation

ibrahimyilmaz
Copy link

This pull request contains basic implementation of the ticker with Flow.

I have seen this issue(#540) maybe this can contribute to that issue(plan)

İbrahim Yilmaz added 3 commits April 6, 2020 21:27
testZeroInitialDelay added to test 0 initialDelay.
testDoNotReceiveAfterCancel testcase fixed.
@ibrahimyilmaz ibrahimyilmaz changed the title Flow based Ticker implemented. Flow based Ticker implementation. Apr 7, 2020
@ibrahimyilmaz ibrahimyilmaz changed the title Flow based Ticker implementation. Flow-Based Ticker implementation. Apr 7, 2020
@ibrahimyilmaz ibrahimyilmaz changed the title Flow-Based Ticker implementation. tickerFlow implementation Apr 7, 2020
*/
public fun tickerFlow(
period: Long,
initialDelay: Long = period
Copy link
Contributor

Choose a reason for hiding this comment

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

To follow the example of functions like withTimeout and debounce, I think we should add the Millis suffix to these parameter names, and maybe provide an overload using kotlin.time.Duration?

Copy link
Author

Choose a reason for hiding this comment

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

Good point for naming and we have .receiveAsFlow() extension function.

Directly we can use kotlinx-coroutine's ticker. Also this can be better idea to have single implementation.
What do you think about it?

Copy link
Contributor

@joffrey-bion joffrey-bion May 15, 2020

Choose a reason for hiding this comment

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

It could indeed be done this way, but that would couple it to JVM platform 😥
I am not sure a channel would be necessary behind the scenes to implement it, but I will let the maintainers comment on this.

EDIT: moved part of this comment to the relevant conversation

val timer = Timer()
timer.schedule(initialDelay, period) {
offer(Unit)
}
Copy link
Contributor

@joffrey-bion joffrey-bion May 13, 2020

Choose a reason for hiding this comment

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

This implementation uses a background thread and is JVM-specific.
Couldn't we simply use a loop with delay() and make it multiplatform? Or am I naive here?
I fail to see the benefit of running another thread here.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! very good point.
Also ticker implementation can be moved to commonMain.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see now why a concurrent behaviour is necessary and would like to backtrack a bit: we can't use a simple loop with delays because otherwise the ticker delay is impacted by the execution time of the collector code, which is probably not desirable in this case.

That's why we do need some concurrent process (a coroutine?) to send the ticks.
This still doesn't mean we have to use a thread and Java's timer.
There may be a multiplatform way to do this.

I'll let the maintainers of the lib comment on this, though

mode: TickerMode = TickerMode.FIXED_PERIOD
): Flow<Unit> {
require(delayMillis > 0)
return ticker(delayMillis, initialDelayMillis, context, mode).receiveAsFlow()
Copy link
Contributor

Choose a reason for hiding this comment

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

Because the channel is hidden here, we probably want to handle cancellation properly.
Using consumeAsFlow() instead of receiveAsFlow() should do the trick I think.

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.

2 participants