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

[FluentInputBase] Use Debouncer instead of PeriodicTimer for debouncing ValueChanged handler with ImmediateDelay. #2042

Merged

Conversation

ApacheTech
Copy link
Contributor

Pull Request

📖 Description

This pull request applies the use of the internal Debouncer instead of the PeriodicTimer to debounce values. A live demonstration has also been added to the FluentSearch page to show how Immediate can be used, as opposed to the oninput method.

🎫 Issues

#2030: Using the FluentSearch input box, but likely any debounce with ImmediateDelay, you can reliably force it to throw an exception, by firing the debounce while the PeriodicTimer is resetting itself. The exception shows that the CancellationToken used to reset the timer is set to cancelled, as the timer tries to tick.

👩‍💻 Reviewer Notes

The only code-flow change I've had to make is to push the invocation of the ValueChanged handler back to the Dispatcher, within FluentInputBase.cs. This is a thread safety precaution, as the use of the Debouncer takes the invocation of UI state changes off-thread.

📑 Test Plan

Because this is an implementation swap, any unit tests that cover the debouncing process will still pass, as intended. No further coverage necessary. Systems tests added by way of a working live example.

✅ Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new component
  • I have modified an existing component
  • I have validated the Unit Tests for an existing component

⏭ Next Steps

It would be nice to expose the Debouncer as public, and promote its use as a simple and effective debouncer, for most situations.

@ApacheTech
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link
Collaborator

@dvoituron dvoituron left a comment

Choose a reason for hiding this comment

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

Great job. Many thanks.

Could you update two details before merging? :-)

@vnbaaij vnbaaij requested a review from dvoituron May 15, 2024 08:14
@vnbaaij vnbaaij enabled auto-merge (squash) May 15, 2024 08:14
@vnbaaij vnbaaij merged commit 5dff173 into microsoft:dev May 15, 2024
2 checks passed
@vnbaaij vnbaaij added this to the v4.7.3 milestone May 15, 2024
vnbaaij pushed a commit that referenced this pull request May 15, 2024
…uncing `ValueChanged` handler with `ImmediateDelay`. (#2042)

* fix(#2030):  debounce using ImmediateDelay can throw an exception because of a race condition with PeriodicTimer.

* feat: add `FluentSearch` demo for immediate use with debounce

* chore: minor code clean for consistency
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.

None yet

3 participants