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 #3918 and #3913 - Accepting behavior #3921

Merged
merged 9 commits into from
Feb 26, 2025

Conversation

tig
Copy link
Collaborator

@tig tig commented Feb 25, 2025

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 tig changed the title Fixes #3918, and #3913 Fixes #3918 and #3913 - Accepting behavior Feb 25, 2025
@tig tig requested review from BDisp and tznind February 26, 2025 13:41
BDisp
BDisp previously requested changes Feb 26, 2025
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.

Seems that Button's CanFocus false isn't working as expected.

Copy link
Collaborator

@tznind tznind left a comment

Choose a reason for hiding this comment

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

Hmn, so do I understand correctly that it is a new requirement for all button click handlers is for user to e.cancel in the handler? Otherwise any default buttons nearby may get click handler (now called Accepting) called?

We should double check there are no IsDefault issues with our existing dialogs like MessageBox and the Query one - if they have defaults we will need this new esc bit

I also think this is going to be confusing for new users. But happy to merge and see how it goes.

@BDisp
Copy link
Collaborator

BDisp commented Feb 26, 2025

I agree with @tznind. This will be confusing for new users. My suggestion is to add a new event called Accepted which will be raised if the Accepting event wasn't been handled and so, not canceled. In this case most of the calls to the Accepting event will be replaced by the Accepted event.

@tig
Copy link
Collaborator Author

tig commented Feb 26, 2025

Hmn, so do I understand correctly that it is a new requirement for all button click handlers is for user to e.cancel in the handler? Otherwise any default buttons nearby may get click handler (now called Accepting) called?

Correct.

Note, previously, e.Handed/e.Cancel actually did either nothing or was non-deterministic. So while it was "easier to use" it was actually deeply broken.

We should double check there are no IsDefault issues with our existing dialogs like MessageBox and the Query one - if they have defaults we will need this new esc bit

I already did this when I implemented all this.

I also think this is going to be confusing for new users. But happy to merge and see how it goes.

The crux of this is the making Accepting universal and getting rid of explicit Clicked events.

Things are now super consistent and deterministic, but there IS a bit of mental load required to "get it".

I'm happy to discuss ways of mitigating this. Thots:

  • Add an Accepted event that is raised AFTER and can't be cancelled.
  • Re-introduce Clicked. I hate this idea.

Others?

@tig tig merged commit 7ba6d63 into gui-cs:v2_develop Feb 26, 2025
4 checks passed
@tig tig mentioned this pull request Feb 26, 2025
tig added a commit that referenced this pull request 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* API doc updates (#3928)

---------

Co-authored-by: Tonttu <[email protected]>
Co-authored-by: BDisp <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants