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

DispatcherQueueTimerExtensions bug #4293

Closed
2 of 12 tasks
matheus-inacio opened this issue Oct 2, 2021 · 3 comments · Fixed by #4294
Closed
2 of 12 tasks

DispatcherQueueTimerExtensions bug #4293

matheus-inacio opened this issue Oct 2, 2021 · 3 comments · Fixed by #4294
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior Completed 🔥

Comments

@matheus-inacio
Copy link

Describe the bug

The Debounce extension method is not correctly updating the Action in the ConcurrentDictionary. This is causing the debounce extension to trigger the oldest version of the action in the 'chain' instead of the latest.

Steps to Reproduce

I've made a small test in order to confirm the bug:

[TestClass]
public class DebounceTests
{
	private static DispatcherQueueTimer _debounceTimer;
	private string _searchedTerm;
	private double _typingInterval = 200;

	[ClassInitialize]
	public static async Task InitializeAsync(TestContext context)
	{
		await ThreadHelper.ExecuteOnUIThread(() =>
		{
			_debounceTimer = DispatcherQueue.GetForCurrentThread().CreateTimer();
		});
	}

	private Task DelayAsync(double milliSeconds) => Task.Delay(TimeSpan.FromMilliseconds(milliSeconds));

	[TestMethod]
	public async Task ShouldTriggerTheLatestAction()
	{
		TriggerDebounce("T");
		await DelayAsync(_typingInterval);

		TriggerDebounce("Te");
		await DelayAsync(_typingInterval);

		TriggerDebounce("Tes");
		await DelayAsync(_typingInterval);

		TriggerDebounce("Test");

		await DelayAsync(800); // Waits debounce time

		_searchedTerm.Should().Be("Test"); // <- Is failing with "T"
	}

	private void TriggerDebounce(string value)
	{
		_debounceTimer.Debounce(() =>
			{
				_searchedTerm = value;
			},
			TimeSpan.FromSeconds(0.6));
	}
}

Expected behavior

As i understand, the debounce should trigger the latest Action passed to the method.

Screenshots

Test failing:

image

Environment

NuGet Package(s): v7.1.0
Package Version(s): v7.1.0

Windows 10 Build Number:

  • Fall Creators Update (16299)
  • April 2018 Update (17134)
  • October 2018 Update (17763)
  • May 2019 Update (18362)
  • May 2020 Update (19041)
  • Insider Build ({build_number})

App min and target version:

  • Fall Creators Update (16299)
  • April 2018 Update (17134)
  • October 2018 Update (17763)
  • May 2019 Update (18362)
  • May 2020 Update (19041)
  • Insider Build ({build_number})
@matheus-inacio matheus-inacio added the bug 🐛 An unexpected issue that highlights incorrect behavior label Oct 2, 2021
@ghost ghost added the needs triage 🔍 label Oct 2, 2021
@ghost
Copy link

ghost commented Oct 2, 2021

Hello matheus-inacio, 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 🙌

@michael-hawker
Copy link
Member

Nice catch @matheus-inacio, not sure how we haven't caught this. Mind adding your nice set of tests to our test suite as well for the future in your #4294 PR? Thanks!

@matheus-inacio
Copy link
Author

Nice catch @matheus-inacio, not sure how we haven't caught this. Mind adding your nice set of tests to our test suite as well for the future in your #4294 PR? Thanks!

Done

@ghost ghost closed this as completed in #4294 Oct 7, 2021
ghost pushed a commit that referenced this issue Oct 7, 2021
#4294)

## Fixes #4293

## PR Type

What kind of change does this PR introduce?

Bugfix

## What is the current behavior?

The debounce extension was incorrectly updating the action in the ConcurrentList.

According to documentation the value passed in the `updateValueFactory` function, of the `AddOrUpdate` method, is the current value in the dictionary.

https://docs.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2.addorupdate?view=net-5.0

![image](https://user-images.githubusercontent.com/29243277/135733394-f7c4d682-9c24-45cd-af4d-69721a7c6383.png)

Therefore the current behavior is maintaining the old action and not updating with the new passed action.

```csharp
_debounceInstances.AddOrUpdate(timer, action, (k, v) => v);
```

## What is the new behavior?

Changed so that the action in the `_ debounceInstances` Dictionary is updated with the latest version.

## PR Checklist

Please check if your PR fulfills the following requirements: <!-- and remove the ones that are not applicable to the current PR -->

- [x] Tested code with current [supported SDKs](../#supported)
- [x] Contains **NO** breaking changes
- [x]  Tests for the changes have been added
- [x]  Header has been added to all new source files (run build/UpdateHeaders.bat)

## Other information

PS: The debuonce extension is referenced in the `CanvasPathGeometryPage` class (although this changes does not seem to break it).
@ghost ghost added Completed 🔥 and removed In-PR 🚀 labels Oct 7, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2021
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior Completed 🔥
Projects
None yet
2 participants