Color Picker: Drag smoothly

[danielchalmers]

Description

• Makes the color picker fluid
• Adds configurable throttle
• Enables future touch support
• Simplifies the code
• Fixes some niche issues like svg selection
• If drag is disabled, it won't follow the pointer the whole time, but will listen to it going down and up
• Fixes MudColor exception when bound to a null string.

How Has This Been Tested?

visually

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

video7.mp4

video6.mp4

Touch (#8394 required):

video8.mp4

Docs

video8.mp4

Checklist:

• The PR is submitted to the correct branch (dev).
• My code follows the code style of this project.
• I've added relevant tests.

[danielchalmers]

Tests and docs are in progress but please give feedback on if this change is even viable!

[danielchalmers]

@ScarletKuro Please show me how you would replace the timer code with PeriodicTimer

[ScarletKuro]

I will try to PR in the branch on the work. Btw the branch doesn't compile. it would be nice if it would to make sure I didn't break anything with periodic timer.

[danielchalmers]

I will try to PR in the branch on the work. Btw the branch doesn't compile. it would be nice if it would to make sure I didn't break anything with periodic timer.

@ScarletKuro Sure. Is this likely to be accepted? Is the debounce technique ok?

[ScarletKuro]

@ScarletKuro Sure. Is this likely to be accepted? Is the debounce technique ok?

Lets ask @henon. Personally I think the drag smoothness is suppose to be like this out of the box, so I like the change.

[henon]

Debounce looks good

[danielchalmers]

@ScarletKuro ready for you to take a look

[codecov]

Codecov Report

Attention: Patch coverage is 92.30769% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 89.84%. Comparing base (e44b9f3) to head (593a8f5).
Report is 16 commits behind head on dev.

Files	Patch %	Lines
...zor/Components/ColorPicker/MudColorPicker.razor.cs	83.33%	0 Missing and 3 partials ⚠️
...MudBlazor/Utilities/Throttle/ThrottleDispatcher.cs	96.55%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #8576      +/-   ##
==========================================
+ Coverage   89.75%   89.84%   +0.09%     
==========================================
  Files         411      414       +3     
  Lines       11817    11917     +100     
  Branches     2362     2364       +2     
==========================================
+ Hits        10606    10707     +101     
+ Misses        683      678       -5     
- Partials      528      532       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[danielchalmers]

@mikes-gh Could you check these formatting errors?

[mikes-gh]

@mikes-gh Could you check these formatting errors?

Looks like the way you declared the array. Dotnet format puts each element on its own line which is sub optimal. Maybe dont refactor this and you wont have a problem

[danielchalmers]

@mikes-gh Could you check these formatting errors?

Looks like the way you declared the array. Dotnet format puts each element on its own line which is sub optimal. Maybe dont refactor this and you wont have a problem

I was a little concerned that even if I reverted that it would still get flagged because the file was changed, but I will try it later. do you think this format error should be fixed or ignored for now? @mikes-gh

[ScarletKuro]

@danielchalmers added periodic timer, I left some commented code and comments, feel free to polish.
I personally did not find any problem in the implementation. Works great, gave pleasure in removing System.Timers that I hate :)

[danielchalmers]

@ScarletKuro while I have your attention maybe you could check the two tests that I skipped 😜

[ScarletKuro]

@ScarletKuro while I have your attention maybe you could check the two tests that I skipped 😜

Can you describe in few words what's wrong with tests? You need to wait when the timer fires and sets new value?

[danielchalmers]

@ScarletKuro while I have your attention maybe you could check the two tests that I skipped 😜

Can you describe in few words what's wrong with tests? You need to wait when the timer fires and sets new value?

Oh did you change the tests? I'm on my phone and didn't see. Basically the drag test doesn't actually drag and the selector one has the wrong offset because I let its events pass through to the overlay

[ScarletKuro]

Oh did you change the tests? I'm on my phone and didn't see

No, I didn't touch the tests.

[ScarletKuro]

Isn't one of the problems that you send:

new PointerEventArgs { MovementX = x2, MovementY = y2 }

while OnPointerMoveAsync in the MudColorPicker checks if Buttons = 1, but when you initialize PointerEventArgs the default will be 0.

upd: tho it seems like this didn't really fix the test.

[ScarletKuro]

Guys, there is serious problem in the terminology.
If we look at @danielchalmers last video, while he moves mouse we still can see the value getting updated, this is NOT debounce, this is throttle!
Debouncing delays the execution of code until the user stops performing a certain action for a specified amount of time. Throttling limits the execution of your code to once in every specified time interval.
In short, if it was debounce, the value on video should have been updated once the mouse stops.
I got jebaited by it, and called my implementation debounce when it should be throttle. Also because of that confusion the code was doing wrong things.

Now I have renamed everything from debounce to throttle, in code and examples.
The throttle implementation is improved and now it's more smooth on wasm and doesn't have this freeze when the interval value is high.
I also implemented a debounce, even-thought it's not used here I guess it can be useful in future?

[danielchalmers]

Nice catch. Throttle does make a lot more sense for this control and was the original functionality

[ScarletKuro]

posted new code, pls re-review.
@mikes-gh would be nice if you could re-upload the bss docs.

[danielchalmers]

Old property name. And description needs updating

[danielchalmers]

Should we mention the difference in Blazor Server again?

there are occasions like long latency in Blazor Server-Side where this implementation hasn't the expected visual assistance.

[danielchalmers]

@ScarletKuro LGTM

[henon]

The ThrottleDispatcher has one little inaccuracy which I guess doesn't really matter in this case: It will ignore the last invocation.

It would matter if you want to do a throttled value update where you need the last value. Of course, for that you would need a timer that fires the last action that was discarded due to throtteling at the end of the interval.

o is throttled invocation
. is throttled discard
x is additional invocation at the interval end of the last discarded action

o...o...o.  x

[ScarletKuro]

The ThrottleDispatcher has one little inaccuracy which I guess doesn't really matter in this case: It will ignore the last invocation.

Nice catch. But I guess we can fix it later in case it will matter, don't feel like headbanging more on this, I've already lost count of what revision this is.

[henon]

One of the throttle tests is flakey, it produced a count of 9 instead of 10 in the last assertion on my machine when running all tests, not when run individually. What's the correct way to make it reliable?

[Test]
public async Task ThrottleDispatcher_ThrottleAsync()
{
    // Arrange
    var counter = 0;
    var dispatcher = new ThrottleDispatcher(100);
    var tasks = new List<Task>();

    Task Invoke()
    {
        counter++;

        return Task.CompletedTask;
    }

    Task CallThrottleAsyncAfterDelay(int delay)
    {
        Task.Delay(delay).ConfigureAwait(false).GetAwaiter().GetResult();

        return dispatcher.ThrottleAsync(Invoke);
    }

    // Act
    for (var i = 0; i < 20; i++)
    {
        tasks.Add(CallThrottleAsyncAfterDelay(50));
    }

    // Assert
    await Task.WhenAll(tasks);
    counter.Should().Be(10);
}

[ScarletKuro]

One of the throttle tests is flakey, it produced a count of 9 instead of 10 in the last assertion on my machine when running all tests, not when run individually. What's the correct way to make it reliable?

Can you test if this is more stable?

    [Test]
    public async Task ThrottleDispatcher_ThrottleAsync()
    {
        // Arrange
        var counter = 0;
        var dispatcher = new ThrottleDispatcher(100);
        var tasks = new List<Task>();

        async Task Invoke()
        {
            Interlocked.Increment(ref counter);

            await Task.Yield();
        }

        Task CallThrottleAsyncAfterDelay(int delay)
        {
            Task.Delay(delay).ConfigureAwait(false).GetAwaiter().GetResult();

            return dispatcher.ThrottleAsync(Invoke);
        }

        // Act
        for (var i = 0; i < 20; i++)
        {
            tasks.Add(CallThrottleAsyncAfterDelay(50));
        }

        // Assert
        await Task.WhenAll(tasks);
        counter.Should().Be(10);
    }

maybe we could also just add counter.Should().BeInRange(9, 10); it's just some little timing issues that i hate to deal with. I think some tolerance is fine here considering the async nature of the test.

[henon]

Strange, the other one is even more unstable, I can easily reproduce 6 instead of 7 by executing all three a couple of times (already added interlocking, it didn't help). Maybe this is due to a call has been discarded?

 [Test]
 public async Task ThrottleDispatcher_ThrottleDelayAfterExecutionAsync()
 {
     // Arrange
     var counter = 0;
     var dispatcher = new ThrottleDispatcher(100, delayAfterExecution: true);
     var tasks = new List<Task>();

     async Task Invoke()
     {
         Interlocked.Increment(ref counter);
         await Task.Delay(50);
     }

     Task CallThrottleAsyncAfterDelay(int delay)
     {
         Task.Delay(delay).ConfigureAwait(false).GetAwaiter().GetResult();

         return dispatcher.ThrottleAsync(Invoke);
     }

     // Act
     for (var i = 0; i < 20; i++)
     {
         tasks.Add(CallThrottleAsyncAfterDelay(50));
     }

     // Assert
     await Task.WhenAll(tasks);
     counter.Should().Be(7);
 }

[henon]

I know of this problem in my own rate limiter implementations. What I did was to set flexible limits. Like the counter should be greater than 3 instead of exactly 7

[ScarletKuro]

I know of this problem in my own rate limiter implementations. What I did was to set flexible limits. Like the counter should be greater than 3 instead of exactly 7

Yeah, you are right.
Should be fixed in #8612