Fixes 4101 - Remove continous press code from Application to View concern instead

[tznind]

Fixes

• Fixes #4101

Proposed Changes/Todos

• Remove application layer WantContinuousButtonPressed
• Make it part of View as subcomponent IMouseHeldDown
• Use timeout event rather than spawning mouse events
• Use MouseGrabView so we capture the release

TODO:

• [x] Move to seperate files
• [x] Follow conventions for events
• [x] Make all views that have established expectations of WantContinuousButtonPressed use the event instead
  • [x] Button
  • [x] Date Picker
  • [x] NumericUpDown
  • [x] ScrollBar
• [x] Logging
• [x] Tests

* maybe - it could be non linear e.g. slow then fast. Having an interface means users can write their own implementation if needed - putting tick speed on interface is clutter for inheritor.

Pull Request checklist:

• [x] I've named my PR in the form of "Fixes #issue. Terse description."
• [x] My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
• [x] My code follows the Terminal.Gui library design guidelines
• [x] I ran dotnet test before commit
• [x] I have made corresponding changes to the API documentation (using /// style comments)
• [x] My changes generate no new warnings
• [x] I have checked my code and corrected any poor grammar or misspellings
• [x] I conducted basic QA to assure all features are working

[tznind]

Ok here is the current state.

I think the mouse grab logic is resulting in extra clicks on mouse move.

Bugs

• Moving mouse results in extra clicks
• Button goes down when clicked and never pops back up again

We could use the root application mouse event to detect mouse release but that feels janky. I need to confirm that grabbing the mouse creates extra events and understand why those events raise accepting.

[tznind]

@tig is this the correct way to implement cancelable event pattern you wrote? cbb20ffe3cc22836fd78bac25c4923924d00f483

I read docs and followed patterns in Accepting event. Let me know if this was not the approach as described in docs.

One thing about docs that struck me is that one of the examples is for Application and therefore has a static function which confused me and made me think maybe I was supposed to have static function too - till I realised it was just coincidence.

[BDisp]

Ok here is the current state.

I think the mouse grab logic is resulting in extra clicks on mouse move.

Bugs

• Moving mouse results in extra clicks

Probably you're considering a button pressed when it isn't pressed and generating a click event. Pay attention about EventFlags.

• Button goes down when clicked and never pops back up again

Without handle the ButtonReleased you can´t cancelled a continuous button pressed.

We could use the root application mouse event to detect mouse release but that feels janky. I need to confirm that grabbing the mouse creates extra events and understand why those events raise accepting.

It's mandatory to captured a ButtonReleased to cancel immediately the continuous button pressed.

[tznind]

We do cancel continuous events correctly.

Because of mouse grab we get all mouse events and any time it is not pressed we clear state and timer.

~The bug seems to be a side effect of a Button grabbing the mouse.~

~I might try to create a standalone reproduction on dev branch.~

Ah actually it could be we are double or triple etc registering the timeout. Probably Start needs to be null op if already down

    public void Start () 
     { 
+         if (_down) return;
         _down = true; 
         Application.GrabMouse (_host);

[tznind]

Ok fixed it, the issue was there was more 'phantom' click generating code inside View.Mouse.cs itself too. Have removed and now works exclusively on events.

Still it needs a delay otherwise it looks like clicking repeatedly makes it go up too fast.

[BDisp]

Ok fixed it, the issue was there was more 'phantom' click generating code inside View.Mouse.cs itself too. Have removed and now works exclusively on events.

Yes is true. Good to know you removed them.

Still it needs a delay otherwise it looks like clicking repeatedly makes it go up too fast.

For that reason it's a good idea to start the continuous button pressing with 3 speed phases, more or less, or configurable. Start with a low speed during n time, then increase the speed a little and after n time increase the speed to go up too fast. @tig done something like that in the WindowsDriver.

[BDisp]

@tznind, using v2win and v2net, do you know why if we long press mouse button on a Button, when the button is released the click event doesn't occur and it should. We should,'t rely in timers for mouse event, unless we're using continuous press, of course. For mouse we should trust on the terminal mouse events. Thanks.

[tznind]

Hmn not sure, is this an issue in dev? Can raise seperate issue?

[BDisp]

Hmn not sure, is this an issue in dev? Can raise seperate issue?

In v2_develop. Issue opened in #4138. Thanks.

[tznind]

WantMousePositionReports seems redundant. It filters mouse events and discards MouseFlags.ReportMousePosition upon entry.

Typically this kind of thing would be achieved just by responding to the events you care about not having a bool flag to suppress them 😕

[tznind]

I have expanded this to abstract away the subcomponents of Application that are needed to make this functionality work. Namely:

• TimedEvents (already had abstraction)
• MouseGrab (new)

This allows tests that were previously required FakeDriver etc to work instead as unit tests e.g.

        // Setup components for mouse held down
        var timed = new TimedEvents ();
        var grab = new MouseGrabHandler ();
        view.MouseHeldDown = new MouseHeldDown (view, timed, grab);

        // Register callback for what to do when the mouse is held down
        var clickedCount = 0;
        view.MouseHeldDown.MouseIsHeldDownTick += (_, _) => clickedCount++;

        // Mouse is currently not held down so should be no timers running
        Assert.Empty(timed.Timeouts);

        // When mouse is held down
        me.Flags = pressed;
        view.NewMouseEvent (me);
        Assert.Equal (0, clickedCount);
        me.Handled = false;

        // A timer should begin
        var t = Assert.Single (timed.Timeouts);

        // Invoke the timer
        t.Value.Callback.Invoke ();

        // Event should have been raised
        Assert.Equal (1, clickedCount);
        Assert.NotEmpty(timed.Timeouts);

[tznind]

@tig I am going to leave WantContinousButtonPressed in for now.

I want to revisit this and add logarithmic repeat events too but was thinking that would be nicer in timed events as a general concept. But it looks quite hairy to modify so doing it as a followup makes sense I think.

[tznind]

Looks like test failure is in EscSeqUtilsTests

For some reason it has an application loop in it - seems to be hanging in there somewhere.

I will look into it, DecodeEscSeq_Multiple_Tests is an 800 line test that seems to include incredibly low level stuff and Application core loop stuff.

        Application.Iteration += (s, a) =>
                                 {
                                     if (_actionStarted)
                                     {
                                         // set Application.WantContinuousButtonPressedView to null
                                         view.WantContinuousButtonPressed = false;

                                         Application.RaiseMouseEvent (new () { Position = new (0, 0), Flags = 0 });

                                         Application.RequestStop ();
                                     }
                                 };

        Application.Run (top);

[tznind]

Ok this is ready, theres some strange stuff I removed in EscSeqUtilsTests

It seems like WantsContinousPress was being passed into the EscSeqUtils parsing code. I removed the application driver stuff in that test and confirmed it all still works in CursesDriver on WSL etc.

[tznind]

Closing because code is now in #4173 along with the smooth increments instead of fixed.