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

When the CanFocus property is set to false, the Accepting event is executed 3 times #3905

Closed
luizfernandonb opened this issue Feb 4, 2025 · 6 comments · Fixed by #3921
Closed
Labels
Milestone

Comments

@luizfernandonb
Copy link

To Reproduce
Steps to reproduce the behavior:

  1. Copy the code below
var window = new Window();
window.Title = "test1";

var btn = new Button();
btn.Title = "test2";
btn.X = Pos.Center();
btn.Y = Pos.Center();
btn.CanFocus = false;
btn.Accepting += (_, _) => {
    Debug.WriteLine("accept");
};

window.Add(btn);

Application.Init();
Application.Run(window);
Application.Shutdown();
  1. Run in your IDE or text editor

Expected behavior
The event will only be executed once

Screenshots
When executing the code above I have this in the Visual Studio output:
Image

If I comment out the CanFocus property code, I have this in the Visual Studio output:
Image

Desktop (please complete the following information):

  • OS: Windows 11 24H2 26100.2894
  • Browser: Microsoft Edge
  • Version: 132.0.2957.140

Additional context
It's in v2
Image

@tig tig added this to the V2 Beta milestone Feb 18, 2025
@tig
Copy link
Collaborator

tig commented Feb 18, 2025

I'll look at this asap. thanks!

@tig
Copy link
Collaborator

tig commented Feb 25, 2025

There is confusion here (and a bug):

First, the name of the event is Accepting, not Accept. This is intentional. It indicates the event is being raised because the user is IN THE PROCESS of accepting something. It is a cancelable event, meaning if it is not cancelled, other things will process it.

If you do not indicate that you handled the Accepting event (by setting e.Cancel = true), here's what happens:

  • If there's a peer view that's Button with IsDefault == true Command.Accept will be invoked (not your case).
  • If there's a SuperView, Command.Accept will be invoked on it (your case).

You can fix this in your code like this:

btn.Accepting += (_, e) => {
    // If you do something with Accepting, you should indicate you've handled it
    e.Cancel = true;
    Debug.WriteLine("accept");
};

That said, the correct behavior for your test app shouldn't be THREE invocations of your handler. It should be one.

I am working on a fix.

tig added a commit to tig/Terminal.Gui that referenced this issue Feb 25, 2025
@tig
Copy link
Collaborator

tig commented Feb 25, 2025

FWIW, if you change your code to

var window = new Window();
window.Title = "test1";

var btn = new Button();
btn.Title = "_test2";
btn.X = Pos.Center();
btn.Y = Pos.Center();
btn.CanFocus = false;
btn.Accepting += (_, _) => {
    Debug.WriteLine("accept");
};

window.Add(btn);

Application.Init();
Application.Run(window);
Application.Shutdown();

And the user presses Alt-T we get only a single "accept".

IOW, the bug is in mouse handling. Which is I why I had to do this to reproduce it in a unit test:

        Application.RaiseMouseEvent (
                                     new MouseEventArgs ()
                                     {
                                         ScreenPosition = btnFrame.Location,
                                         Flags = MouseFlags.Button1Pressed
                                     });

        Application.RaiseMouseEvent (
                                     new MouseEventArgs ()
                                     {
                                         ScreenPosition = btnFrame.Location,
                                         Flags = MouseFlags.Button1Released
                                     });

        Application.RaiseMouseEvent (
                                     new MouseEventArgs ()
                                     {
                                         ScreenPosition = btnFrame.Location,
                                         Flags = MouseFlags.Button1Clicked
                                     });

Instead of just doing a Button1Clicked event... Because this is what drivers actually send on a mouse click:

  • Press
  • Release
  • Click

Still working on it...

@tig
Copy link
Collaborator

tig commented Feb 25, 2025

Another clue:

button.CanFocus = false;
button.HighlightStyle = HighlightStyle.None;

Disabling the highlight style on the button hides the issue.

We have a set of fairly convoluted logic in View.Mouse.cs around handling mouse-grab situations. E.g.

  // Post-Conditions
  if (HighlightStyle != HighlightStyle.None || WantContinuousButtonPressed)
  {
      if (WhenGrabbedHandlePressed (mouseEvent))
      {
          return mouseEvent.Handled;
      }

      if (WhenGrabbedHandleReleased (mouseEvent))
      {
          return mouseEvent.Handled;
      }

      if (WhenGrabbedHandleClicked (mouseEvent))
      {
          return mouseEvent.Handled;
      }
  }

@tig
Copy link
Collaborator

tig commented Feb 25, 2025

Ok, the bug is DEEP and I have decided not worth fixing.

The logic for dealing with mouse events is simply too fragile and convoluted.

@tznind and I are working on addressing this as part of

Related

I'm closing this as "By Design; Won't Fix".

@luizfernandonb - You have several ways of dealing with this:

  1. Set e.Cancel = true in your Accepting handler. This is the correct fix for your code as posted.
  2. Turn off highlighting on your button (btn.HighlightStyle = HighlightStyle.None)

I have added more unit tests as part of #3921.

@luizfernandonb
Copy link
Author

Thanks!

tig added a commit that referenced this issue Feb 26, 2025
* Fixed #3905, #3918

* Tweaked Generic

* Label code cleanup

* Clean up.

* Clean up.

* Clean up2.
tig added a commit that referenced this issue Feb 28, 2025
* Moved scripts

* testing versions

* Rune extensions micro-optimizations (#3910)

* Add benchmarks for potentially optimizable RuneExtensions

* Add new RuneExtensions.DecodeSurrogatePair benchmark implementation

Avoids intermediate heap array allocations which is especially nice when the rune is not surrogate pair because then array heap allocations are completely avoided.

* Enable nullable reference types in RuneExtensions

* Make RuneExtensions.MaxUnicodeCodePoint readonly

Makes sure no one can accidentally change the value. Ideally would be const value.

* Optimize RuneExtensions.DecodeSurrogatePair

* Remove duplicate Rune.GetUnicodeCategory call

* Add new RuneExtensions.IsSurrogatePair benchmark implementation

Avoids intermediate heap allocations by using stack allocated buffer.

* Optimize RuneExtensions.IsSurrogatePair

* Add RuneExtensions.GetEncodingLength tests

* Optimize RuneExtensions.GetEncodingLength

* Optimize RuneExtensions.Encode

* Print encoding name in benchmark results

* Rename variable to better match return description

* Add RuneExtensions.EncodeSurrogatePair benchmark

---------

Co-authored-by: Tig <tig@users.noreply.github.com>

* Reduce func allocations (#3919)

* Replace Region.Contains LINQ lambdas with foreach loop

Removes the lambda func allocations caused by captured outer variables.

* Replace LineCanvas.Has LINQ lambda with foreach loop

* Fix LineCanvas.GetMap intersects array nullability

It should be enough to add null-forgiving operator somewhere in the LINQ query to make the final result non-null. No need to shove the nullability further down the line to complicate things. :)

* Replace LineCanvas.All LINQ lambda with foreach loop

* Replace Region.Intersect LINQ lambdas and list allocation with foreach loop and rented array

* Use stackalloc buffer in Region.Intersect when max 8 rectangles

* Fix LineCanvas.GetCellMap intersects array nullability

* Remove leftover LineCanvas.GetRuneForIntersects null-conditional operators

* Remove leftover IntersectionRuneResolver.GetRuneForIntersects null-conditional operators

* PosAlign.CalculateMinDimension: calculate sum during loop

No need to first put the dimensions in a list and then sum the list when you can just add to sum while looping through dimensions.

* PosAlign.CalculateMinDimension: Remove intermediate list and related filter func allocation

* TextFormatter.GetRuneWidth: Remove intermediate list and related sum func allocation

* ReadOnlySpan refactor preparation for GetCellMap rewrite

* LineCanvas.GetCellMap: Reuse intersection list outside nested loops

GetCellMap would not benefit much from array pool because IntersectionDefinition is not struct. This  change eliminates majority of the rest of Func<,> allocations. As a bonus IntersectionDefinition[] allocations dropped nicely.

* Refactor local method UseRounded

* Wrap too long list of method parameters

* Region: Consistent location for #nullable enable

---------

Co-authored-by: Tig <tig@users.noreply.github.com>

* Fixes #3918 and #3913 - `Accepting` behavior (#3921)

* Fixed #3905, #3918

* Tweaked Generic

* Label code cleanup

* Clean up.

* Clean up.

* Clean up2.

* Fixes #3839, #3922 - CM Glyphs not working (#3923)

* fixed

* Moved Glyphs to ThemeScope

* Removed test code

* Fixed nav (#3926)

* Fixes #3881. PositionCursor broke with recent ConsoleDriver changes. (#3927)

* Reduce IntersectionType[] allocations (#3924)

* Eliminate LineCanvas.Has params array allocation

Inline ReadOnlySpan arguments do not incur heap allocation compared to regular arrays.

* Allocate once LineCanvas.Exactly corner intersection arrays

---------

Co-authored-by: Tig <tig@users.noreply.github.com>

* API doc updates (#3928)

---------

Co-authored-by: Tonttu <15074459+TheTonttu@users.noreply.github.com>
Co-authored-by: BDisp <bd.bdisp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants