MudBlazor / MudBlazor

Blazor Component Library based on Material design with an emphasis on ease of use. Mainly written in C# with Javascript kept to a bare minimum it empowers .NET developers to easily debug it if needed.
http://mudblazor.com
MIT License
8.03k stars 1.28k forks source link

Color Picker: Drag smoothly #8576

Closed danielchalmers closed 6 months ago

danielchalmers commented 6 months ago

Description

How Has This Been Tested?

visually

Types of changes

https://github.com/MudBlazor/MudBlazor/assets/7112040/d8d0d4cf-2aea-4bf5-87b8-539c56cf8939

https://github.com/MudBlazor/MudBlazor/assets/7112040/357749f4-6b0c-45fd-b8dc-14e6c76eb16d

Touch (#8394 required):

https://github.com/MudBlazor/MudBlazor/assets/7112040/b897be61-fff0-47d9-ba88-3f254efca99b

Docs

https://github.com/MudBlazor/MudBlazor/assets/7112040/b4954ae7-15db-41eb-be7f-bb6ea32ab5d5

Checklist:

danielchalmers commented 6 months ago

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

danielchalmers commented 6 months ago

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

ScarletKuro commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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.

danielchalmers commented 6 months ago

@ScarletKuro ready for you to take a look

codecov[bot] commented 6 months ago

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 :warning:
...MudBlazor/Utilities/Throttle/ThrottleDispatcher.cs 96.55% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

danielchalmers commented 6 months ago

@mikes-gh Could you check these formatting errors?

mikes-gh commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@ScarletKuro while I have your attention maybe you could check the two tests that I skipped šŸ˜œ

ScarletKuro commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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

No, I didn't touch the tests.

ScarletKuro commented 6 months ago

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.

mikes-gh commented 6 months ago

@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

Just return it to what it was before you refactored the array initialisation and it will pass. Unfortunately, the dotnet format is running on all files ATM so keeping it will just red cross all PRs. I can alternatively just turn it off but I think its doing a good job of keeping the code well formatted for now.

danielchalmers commented 6 months ago

@mikes-gh Reverting it worked and it now passes the code quality. I am getting a "Check JS is ES6 compliant" build error now though...

danielchalmers commented 6 months ago

@ScarletKuro is the PeriodicWorker supposed to update every period, or the period after all movement has stopped? It's updating every 100ms by default even if you are constantly moving the mouse.

I don't think this is a bad thing, just checking to confirm that's the intent.

ScarletKuro commented 6 months ago

is the PeriodicWorker supposed to update every period

Yes, it's updating every period. If you don't need that I don't think you need heavy Timer or PeriodicTimer just to debounce only when "cursor" movement is happening (or throttle would work too), You can write such mechanism without timers with TPL or semaphores or combination of cancellationTokenSource etc etc etc, there are a lot of possibilities to achieve this and the code is usually not complicated.

mikes-gh commented 6 months ago

@danielchalmers Its probably worth testing this on a higher latency BSS version of the Docs. Maybe we can upload it to MudBlazorTest when its ready.

ScarletKuro commented 6 months ago

Yes, it's updating every period. If you don't need that I don't think you need heavy Timer or PeriodicTimer just to debounce only when "cursor" movement is happening (or throttle would work too), You can write such mechanism without timers with TPL or semaphores or combination of cancellationTokenSource etc etc etc, there are a lot of possibilities to achieve this and the code is usually not complicated.

updated my post

danielchalmers commented 6 months ago

@danielchalmers Its probably worth testing this on a higher latency BSS version of the Docs. Maybe we can upload it to MudBlazorTest when its ready.

@mikes-gh Yes please, could you help me with that

danielchalmers commented 6 months ago

@ScarletKuro It's complaining that PeriodicWorker isn't convered by tests

ScarletKuro commented 6 months ago

@ScarletKuro It's complaining that PeriodicWorker isn't convered by tests

I mean, do you want to use this in your solution? If yes I have no problem to cover it with tests.

danielchalmers commented 6 months ago

@ScarletKuro It's complaining that PeriodicWorker isn't convered by tests

I mean, do you want to use this in your solution? If yes I have no problem to cover it with tests.

@ScarletKuro Whatever you think is best

mikes-gh commented 6 months ago

@danielchalmers Its probably worth testing this on a higher latency BSS version of the Docs. Maybe we can upload it to MudBlazorTest when its ready.

@mikes-gh Yes please, could you help me with that

Sure, let me know when you have finished. It's not a big job to deploy but it's not something I want to do on every commit as its manual.

danielchalmers commented 6 months ago

@danielchalmers Its probably worth testing this on a higher latency BSS version of the Docs. Maybe we can upload it to MudBlazorTest when its ready.

@mikes-gh Yes please, could you help me with that

Sure, let me know when you have finished. It's not a big job to deploy but it's not something I want to do on every commit as its manual.

@mikes-gh I believe I'm done with it unless anyone finds something in the review

ScarletKuro commented 6 months ago

@ScarletKuro Whatever you think is best

I think you need a real debounce not periodic tasks and not denounce that is based on a timer.

Changes:

  1. DebounceInterval type from double to int (ms should not be floating type).
  2. Used ParameterState, it was exactly created for this usage cases, to not make boilerplate code of variables and comparing them yourself.
  3. Added OnInitialized, it looks like your debounce was not initializing until you set a new interval, I noticed this in examples like debounce wasn't actually working.
  4. Added tests for the debounce function.
danielchalmers commented 6 months ago

I'll leave it up to @ScarletKuro now. Ready to merge on my end. Really liking the new code

ScarletKuro commented 6 months ago

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 commented 6 months ago

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

ScarletKuro commented 6 months ago

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

danielchalmers commented 6 months ago

@ScarletKuro LGTM

ScarletKuro commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 https://github.com/MudBlazor/MudBlazor/pull/8612