Add debounce selector #2314
Add debounce selector #2314
Conversation
| @@ -129,7 +151,98 @@ public fun <T> Flow<T>.debounce(timeoutMillis: Long): Flow<T> { | |||
| */ | |||
| @ExperimentalTime | |||
| @FlowPreview | |||
| public fun <T> Flow<T>.debounce(timeout: Duration): Flow<T> = debounce(timeout.toDelayMillis()) | |||
| public fun <T> Flow<T>.debounceWithDuration(timeout: Duration): Flow<T> = debounce(timeout.toDelayMillis()) | |||
There was a problem hiding this comment.
It's not ideal that I am renaming this method from Flow<T>.debounce(timeout: Duration): Flow<T>.
The selector variation method name Flow<T>.debounce(timeout: (T) -> Duration): Flow<T> below was conflicting with Flow<T>.debounce(timeoutMillis: (T) -> Long): Flow<T>. I could just rename only the duration selector variation to e.g. Flow<T>.debounceWithDurationSelector(timeout: (T) -> Duration): Flow<T>, but I felt this would be a better grouping to keep the word duration in the method names for both instead of just one of them.
Please let me know if renaming an existing method concerns.
There was a problem hiding this comment.
The right way to do it is this:
- Keep the Kotlin name
debounce - Add
@JvmName("debounceDuration")to avoid platform declaration clash (we don't useWithor other prepositions in these kind of names).
There was a problem hiding this comment.
@JvmName does not seem to work when a function argument is used. I see the issue mentioned in https://youtrack.jetbrains.com/issue/KT-22119. I pushed with debounceDuration(...).
| while (lastValue !== DONE) { | ||
| select<Unit> { | ||
| // Give a chance to consume lastValue first before onReceiveOrNull receives a new value | ||
| lastValue?.let { value -> |
There was a problem hiding this comment.
I reversed the order of values.onReceiveOrNull and lastValue?.let because when the flow emits "A", "B" at once with 0 timeout like the testZeroDebounceTimeSelector test, values.onReceiveOrNull kept picking it up first and get selected by select<Unit>; therefore, skipping the first value "A".
|
|
||
| if (timeoutMillis == 0L) { | ||
| lastValue = null | ||
| runBlocking { |
There was a problem hiding this comment.
What's the purpose of runBlocking/launch here? It should be just downstream.emit.
There was a problem hiding this comment.
I get the Suspension functions can be called only within coroutine body error. Any suggestions? I used runBlocking to make sure it gets executed before moving on.
There was a problem hiding this comment.
That's exactly what emit is doing.
There was a problem hiding this comment.
Yes, I meant to say that the select {} block does not let me call a suspend function downstream.emit(unboxedValue).
There was a problem hiding this comment.
You should move this code outside of select {} block, that is, you should check conditions and emit if needed first, then do select.
There was a problem hiding this comment.
Thanks, I pushed a fix for this.
|
@elizarov It's ready for another review. |
|
I've restored binary compatibility with the previous version, cleaned up implementation a bit, and reopened it as #2336 (see the last commit for the proper combination of annotation) |
|
@elizarov Thank you for the reviews and for merging it! :) |
Adding support for debounce selector to allow changing debounce time based on the latest emitted value.
Resolves #1216.
Reopening my PR as I accidentally messed up my branch.