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.

Sign up for GitHub

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

Jump to bottom

Add debounce selector #2148

Closed
wants to merge 9 commits into from
Closed

Add debounce selector #2148

wants to merge 9 commits into from

Conversation

mkano9
Copy link
Contributor

@mkano9 mkano9 commented Jul 19, 2020

Adding support for debounce selector to allow changing debounce time based on the latest emitted value.

Resolves #1216.

@mkano9
Copy link
Contributor Author

mkano9 commented Aug 2, 2020

Hi @fvasco, any updates?

@fvasco
Copy link
Contributor

fvasco commented Aug 4, 2020

Hi, @mkano9,
I haven't updates for you, I'm sorry.

If you think there is an issue with the current implementation, please consider to write a consistent test case and file a new issue.

@akhbulatov
Copy link

+1 to this feature. Any progress?

@elizarov
Copy link
Contributor

elizarov commented Sep 25, 2020 •
edited
Loading

We've discussed the design aspects of this change in the team. Green light to the addition itself, but here are some minor adjustments that are needed:

  • The function with selector shall throw an IllegalArgumentException if a negative timeout is returned by a selector at any time.
  • The support for zero timeouts shall be added and it should work without conflation. A non-conflated channel should be used internally in all cases and that is fine.
  • The above changes to timeout behavior shall be consistent with the version without a selector, too (it shall start support zero timeouts returning this, but throw on negative ones).
  • There needs to be an @ExpertimentalTime version that is overloaded for (T) -> Duration selector, too.

Can you please adjust your PR correspondingly?

@mkano9
Copy link
Contributor Author

mkano9 commented Sep 26, 2020

Hi @elizarov, thanks for the feedback. I updated the PR.

elizarov
elizarov suggested changes Oct 1, 2020
View reviewed changes
kotlinx-coroutines-core/common/src/flow/operators/Delay.kt Outdated
@@ -16,7 +16,7 @@ import kotlin.time.*

/**
* Returns a flow that mirrors the original flow, but filters out values
* that are followed by the newer values within the given [timeout][timeoutMillis].
* that are followed by the newer values within the given [timeoutMillis].
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, leave this part of the docs as before (it is consistent with how sample is documented, too)

kotlinx-coroutines-core/common/src/flow/operators/Delay.kt Outdated
*
* @param timeoutMillisSelector [T] is the emitted value and the return value is timeout in milliseconds.
*/
@ExperimentalTime
Copy link
Contributor

@elizarov elizarov Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This one should not have @ExperimentalTime annotation, but there should be another version with (T) -> Duration selector that has one.

kotlinx-coroutines-core/common/src/flow/operators/Delay.kt Outdated
val timeoutMillis = timeoutMillisSelector(unboxedValue)
require(timeoutMillis >= 0L) { "Debounce timeout should not be negative" }
// set timeout when lastValue != null
onTimeout(timeoutMillis) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A case with timeoutMillis == 0 should be explicitly optimized here -- emit it immediately instead of installing onTimeout case.

@mkano9 mkano9 mentioned this pull request Oct 18, 2020
Closed
@mkano9
Copy link
Contributor Author

mkano9 commented Oct 18, 2020

I accidentally merged with master and made further mess. I opened a new PR #2314 and closing this one.

@mkano9 mkano9 closed this Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fvasco fvasco fvasco left review comments

@Ezfero Ezfero Ezfero left review comments

@elizarov elizarov elizarov requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mkano9 @fvasco @akhbulatov @elizarov @Ezfero @MasoodFallahpoor