Windows icon indicating copy to clipboard operation
Windows copied to clipboard

DispatcherQueueTimerExtensions.Debounce does not work with leading edge action

Open huynhsontung opened this issue 2 years ago • 5 comments

Describe the bug

Debounce(immediate: true) does not work with a default timer. When a timer is created, IsRepeating is set to true by default. This breaks the logic of Debounce when immediate: true. Setting IsRepeating to false on the timer fixes this issue.

Steps to reproduce

  1. Create a DispatcherQueueTimer using DispatcherQueue.GetForCurrentThread().CreateTimer()
  2. Use Debounce() on that timer with immediate: true
  3. The action is only triggered once and never again.

Expected behavior

Subsequent actions should trigger once the timer expires just like without the immediate flag.

Windows Build Number

  • [ ] Windows 10 1809 (Build 17763)
  • [ ] Windows 10 1903 (Build 18362)
  • [ ] Windows 10 1909 (Build 18363)
  • [ ] Windows 10 2004 (Build 19041)
  • [ ] Windows 10 20H2 (Build 19042)
  • [ ] Windows 10 21H1 (Build 19043)
  • [X] Windows 11 21H2 (Build 22000)
  • [ ] Other (specify)

App minimum and target SDK version

  • [ ] Windows 10, version 1809 (Build 17763)
  • [X] Windows 10, version 1903 (Build 18362)
  • [ ] Windows 10, version 1909 (Build 18363)
  • [ ] Windows 10, version 2004 (Build 19041)
  • [ ] Other (specify)

Visual Studio Version

2022

Help us help you

Yes, I'd like to be assigned to work on this item.

huynhsontung avatar Jul 05 '23 09:07 huynhsontung

Hello huynhsontung, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

ghost avatar Jul 05 '23 09:07 ghost

Thanks for the issue @huynhsontung, moved this to the new repo. Seems like we just have a couple of simple tests and not one for this scenario: https://github.com/CommunityToolkit/Windows/blob/main/components/Extensions/tests/DispatcherQueueTimerExtensionTests.cs

In immediate mode though, there should be only one firing of the event at the start. It'll only fire again if a request comes in after the elapsed period of time. For my clarity, is that not what you're observing? Or you're saying that it never fires again after the first time?

The docs don't say what the default should be, I filed an issue: https://github.com/MicrosoftDocs/winrt-api/issues/2386

I would have thought IsRepeating was false by default, as DispatcherTimer didn't have it. I wonder if this was a change in the WASDK version, which platform are you seeing this behavior on? If the two platforms are different defaults, then that seems like a regression we should file against the platform?

In either case, seems like we should add some tests here for this scenario to our matrix to be sure.

michael-hawker avatar Jul 14 '23 17:07 michael-hawker

Hello @michael-hawker,

In immediate mode, it never fires again after the first time.

I am developing for UWP (min version 18362, target version 22000) but I can confirm that IsRepeating is true by default for both UWP and WASDK. I can work on a patch for this issue from our end.

huynhsontung avatar Jul 14 '23 21:07 huynhsontung

I'm surprised IsRepeating is true. I wonder if we missed this fact when we converted these from DispatcherTimer helpers to DispatcherQueueTimer helpers?

Is there value in these if IsRepeating is true? Do we just have to require/document that IsRepeating should be turned off for this function to work properly you think? (Or I suppose we can have the function turn it off itself if required...)

michael-hawker avatar Jul 14 '23 23:07 michael-hawker

I think the feature should continue to work regardless of IsRepeating setting. Users may rely on IsRepeating for their own timer logic. Like its non-immediate mode, we can handle the DispatcherQueueTimer.Tick event for triggering action as well in this case.

huynhsontung avatar Jul 15 '23 07:07 huynhsontung