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

Fixes #3370. Adds visual highlight for press and hold (for Button, Checkbox, RadioGroup, and Border) #3372

Merged
merged 34 commits into from
Apr 8, 2024

Conversation

tig
Copy link
Collaborator

@tig tig commented Apr 1, 2024

Fixes

Proposed Changes/Todos

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • 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.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@tig
Copy link
Collaborator Author

tig commented Apr 2, 2024

@BDisp give this PR a try.

Bring it down locally and note how i've implemented a UI effect on Button that shows when it's been pressed/released.

Note that if you do a mouse button press on a button, and then move off of it, it does NOT click, but if you move it back (while still holding the button) down and then release, it does click.

I think this is what you want:

  • "prevents many times when accidently press a button and avoiding the clicked after moving the mouse.

...AND is what I want:

  • Makes it so that you can be moving the mouse a little when you try to click (actually not quite working right yet)

C8keSye 1

@BDisp
Copy link
Collaborator

BDisp commented Apr 2, 2024

Yes that fit what I said. It isn't clicking in the same point that matters but in the same object. That's good.

@tig
Copy link
Collaborator Author

tig commented Apr 2, 2024

Yes that fit what I said. It isn't clicking in the same point that matters but in the same object. That's good.

I think this means removing the _processButtonClick related logic from WindowsDriver, and replicating what I've done in this PR with Button into View?

I just did a quick test with this:

image

And Button works perfectly (I think).

@tig
Copy link
Collaborator Author

tig commented Apr 2, 2024

Also, I'd love your thoughts on the "repeat accelaration" I put in.

See how the Accept Count accelerates after 2 seconds of holding?

NQNW3Iz 1

private async Task ProcessContinuousButtonPressedAsync (MouseFlags mouseFlag)
{
// When a user presses-and-holds, start generating pressed events every `startDelay`
// After `iterationsUntilFast` iterations, speed them up to `fastDelay` ms
const int startDelay = 500;
const int iterationsUntilFast = 4;
const int fastDelay = 50;
int iterations = 0;
int delay = startDelay;
while (_isButtonPressed)
{
var me = new MouseEvent
{
X = _pointMove.X,
Y = _pointMove.Y,
Flags = mouseFlag
};
if (Application.WantContinuousButtonPressedView is null)
{
break;
}
if (iterations++ >= iterationsUntilFast)
{
delay = fastDelay;
}
await Task.Delay (delay);
//Debug.WriteLine($"ProcessContinuousButtonPressedAsync: {view}");
if (_isButtonPressed && (mouseFlag & MouseFlags.ReportMousePosition) == 0)
{
Application.Invoke (() => OnMouseEvent (new MouseEventEventArgs (me)));
}
}
}

Ideally this would be configurable, and I don't like the coupling between ConsoleDriver having a reference to Application.WantContinuousButtonPressedView . So I'd like to figure out how to move the logic out of WindowsDriver and into Application (or View if that makes sense).

tig added 10 commits April 2, 2024 14:49
…would benefit.

Note this breaks Charmap because ScrollView blindly grabs mouse on Enter. This will be fixed in gui-cs#3323.
Works, but not optimally. The entire view invert. What would be really nice is if just the radio item under the mouse inverted. We can fix this later.
@tig tig marked this pull request as ready for review April 2, 2024 23:27
@BDisp
Copy link
Collaborator

BDisp commented Apr 3, 2024

Do this issue is happening to you? Click on the Toplevel button and after the Dialog is open click on the Yes button keeping the button pressed. You'll see that the Dialog width increasing before it close.

WindowsTerminal_DQs6FVVFAA.mp4

@BDisp
Copy link
Collaborator

BDisp commented Apr 3, 2024

That also depends of what you are really doing. If you are dragging a view and want always highlighted until releasing it, that ok. But if you are generating a animation of a button press then if the mouse is outside of his bounds then the color is restored to alert that the button isn't selected if the mouse button is released. You also need to decide if you pressing and holding on a button and move the mouse to another button what the effect you want. Restores the previous and highlight the new button?

tig added 3 commits April 3, 2024 21:10
- Started MouseEventEventArgs -> MouseEvent transition - for OnMouseEvent. Partially fixes gui-cs#3029.
- Refactored Appliation.OnMouseEvent and View.OnMouseEvent to match design guidelines.
- Re-impleented highlight, enabling extensibility (see Border).
- Beefed up unit tests
@tig
Copy link
Collaborator Author

tig commented Apr 4, 2024

@BDisp - huge update here. Please take a look!

@BDisp
Copy link
Collaborator

BDisp commented Apr 5, 2024

I now like more the buttons behaviors. The button only get focus if the mouse button is pressed. If I release the mouse button on another button the clicked event isn't processed which I like very much. One animation you maybe can do is only when moving the mouse without pressing any mouse button is highlight the button when the mouse is over it with the focus color and restore the previous color when the mouse is out of it.
Like this bellow.

devenv_WpuVIZcfyb.mp4

@BDisp
Copy link
Collaborator

BDisp commented Apr 5, 2024

This is weird. If the window cannot move, it must be prevented from moving.

WindowsTerminal_I6opvFlJk8.mp4

@BDisp
Copy link
Collaborator

BDisp commented Apr 5, 2024

This code work but you have to override the OnDrawContent on the Button.cs:

    private bool _isMouseEnter;

    /// <inheritdoc />
    protected internal override bool OnMouseEnter (MouseEvent mouseEvent)
    {
        if (mouseEvent.Flags == MouseFlags.ReportMousePosition)
        {
            _isMouseEnter = true;
            SetNeedsDisplay ();
        }

        return base.OnMouseEnter (mouseEvent);
    }

    /// <inheritdoc />
    protected internal override bool OnMouseLeave (MouseEvent mouseEvent)
    {
        if (mouseEvent.Flags == MouseFlags.ReportMousePosition)
        {
            _isMouseEnter = false;
            SetNeedsDisplay ();
        }

        return base.OnMouseLeave (mouseEvent);
    }

    /// <inheritdoc />
    public override void OnDrawContent (Rectangle contentArea)
    {
        TextFormatter?.Draw (
                             BoundsToScreen (contentArea),
                             HasFocus || (_isMouseEnter && CanFocus && !HasFocus) ? GetFocusColor () : GetNormalColor (),
                             HasFocus ? ColorScheme.HotFocus : GetHotNormalColor (),
                             Rectangle.Empty
                            );
    }
WindowsTerminal_6GaJHEo6os.mp4

Copy link
Collaborator

@BDisp BDisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@BDisp
Copy link
Collaborator

BDisp commented Apr 5, 2024

@tig I hope you like the tig#31. So any view that want this feature only need to set HighlightOnMouseEnter property to true.

Comment on lines 1699 to 1700
// Only delay if there's not a view wanting continuous button presses
if (Application.WantContinuousButtonPressedView is null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessary because this async method is only called if the _isButtonDoubleClicked or _isOneFingerDoubleClicked is true. So, even a view which WantContinuousButtonPressed is true may want a ButtonTripleClicked event. Another detail, for a click, double click or triple click event occurs, it means that were also button released each time, which will generate the clicked events.
Am I missing something?

Comment on lines 1702 to 1703
// QUESTION: Why 300ms?
await Task.Delay (300);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PR #3386 I decreased to 200 to not wait so long.

tig added 2 commits April 8, 2024 10:16
Added Color.GetHighlightColor. Simple implementation for now. Needs to be more robust.
@tig tig changed the title Fixes #3370. Support press-and-hold on Button Fixes #3370. Adds visual highlight for press and hold (for Button, Checkbox, RadioGroup, and Border) Apr 8, 2024
@tig
Copy link
Collaborator Author

tig commented Apr 8, 2024

This is weird. If the window cannot move, it must be prevented from moving.

WindowsTerminal_I6opvFlJk8.mp4

This is caused my lame attempt to have the Dialog that holds the ColorPicker be positioned near the button:

image

The right fix is twofold:

  • Enable any view to be run as a Modal
  • Use Dim.Auto

I've put a temporary fix in this PR.

@tig tig merged commit f1bc42a into gui-cs:v2_develop Apr 8, 2024
1 check passed
@dodexahedron
Copy link
Collaborator

This feature is a nice touch. 👍

@dodexahedron
Copy link
Collaborator

A thought while I'm waiting for my PC to update and boot that this brought up. And maybe it's already there, but I can't test on my phone...

Is behavior like many graphical apps have, where pressing and holding alt will highlight or underline all the hotkeys of elements on screen, if not already displayed (including bindings, potentially) already in there or easy to bolt onto this? 🤔

I mean yeah of course a consumer could do it themselves but it's a nice feature to have in a UI library.

If not there already, maybe a vNext feature?

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jun 18, 2024

Or... Thinking about things that are strictly features and not core framework API...

Have we ever considered having a separate "toolkit" library for features as seems to be pretty common in various libraries these days?

Basically the modular approach, that even .net itself now uses.

🤷‍♂️ Just brainstorming.

@dodexahedron
Copy link
Collaborator

That approach is still easy to have an all-in-one with, as well. You just include all the modules in an all in one library project and publish single file and 💥: there's your all-in-one.

@tig
Copy link
Collaborator Author

tig commented Jun 18, 2024

where pressing and holding alt will highlight or underline all the hotkeys of elements on screen,

Sadly, getting just Alt key presses is a struggle. It barely works on WindowsDriver and seems impossible on *nix.

I am excited for #2612 though!

@dodexahedron
Copy link
Collaborator

Figures 😆

And yes, I love the improvements!

Feels like things are getting shored up nicely. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants