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
from

Conversation

@ibrahimyilmaz
Copy link

@ibrahimyilmaz ibrahimyilmaz commented Apr 6, 2020

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 Apr 6, 2020
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

This comment has been minimized.

@joffrey-bion

joffrey-bion May 13, 2020
Contributor

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?

This comment has been minimized.

@ibrahimyilmaz

ibrahimyilmaz May 13, 2020
Author

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?

This comment has been minimized.

@joffrey-bion

joffrey-bion May 15, 2020
Contributor

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

This comment has been minimized.

@joffrey-bion

joffrey-bion May 13, 2020
Contributor

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.

This comment has been minimized.

@ibrahimyilmaz

ibrahimyilmaz May 13, 2020
Author

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

This comment has been minimized.

@joffrey-bion

joffrey-bion May 19, 2020
Contributor

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

This comment has been minimized.

@joffrey-bion

joffrey-bion May 19, 2020
Contributor

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.