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

Partially Adresses #2491. Refactors how Focus works #3627

Merged
merged 105 commits into from
Aug 29, 2024

Conversation

tig
Copy link
Collaborator

@tig tig commented Jul 25, 2024

This PR paves the way for fixing #2491 by addressing deep design and implementation problems with how Focus currently works.

This PR used to be way more ambitions... it was going to eventually fully enable any View to work Overlapped, removing the need for Toplevel and IsOverlappedContainer. That will now come later.

Fixes

Proposed Changes/Todos

  • Discovered serious issues with how HasFocus, OnEnter/OnLeave, etc... …work in some edge cases. This will require re-visiting the design at a deep level and fixing some long-standing but ignored issues such as how OnEnter/OnLeave don't follow proper cancelation design. Also, there's a need for keeping track of the old focus state of a tree of subviews when that tree loses focus; FocusDireciton is a hack that causes tons of confusion. See https://github.com/tig/Terminal.Gui/blob/8e70e2ae8faafab7cb8305ec6dbbd552c7bf3a43/docfx/docs/navigation.md
  • Figure out how ViewArrangement.Overlapped will work. Use TabIndexes and all the View.NextView focus stuff for navigation (instead of the code in OverlappedMoveNext). Use the Subview ordering (for just the subviews with ViewArrangement.Overlapped` set) to manage the z-order.
  • Rewrite focus engine
  • Dozens of new, primitive unit tests
  • Updated migration doc
  • Massive code cleanup including making most of View #nullable enable
  • View Experiments scenario -> Navigation Tester
  • TabGroup navigation doesn't work completely right with nested groups. These are edge cases that I may tackle later.
  • Fix TabView Scenario - above item may address
  • Fix Wizards - above item may address

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 V2 2491 arrangement overlapped Partially Fixes #2491. Adds ViewArrangement.Overlapped, refactors and beefs up Application and Toplevel Jul 25, 2024
tig added 4 commits August 1, 2024 06:08
…work in some edge cases.

This will require re-visiting the design at a deep level and fixing some long-standing but ignored issues such as how OnEnter/OnLeave don't follow proper cancelation design. Also, there's a need for keeping track of the old focus state of a tree of subviews when that tree loses focus; FocusDireciton is a hack that causes tons of confusion.
@tig
Copy link
Collaborator Author

tig commented Aug 1, 2024

@BDisp I would very much appreciate you reviewing the new Navigation.md overview document in this PR and providing feedback before I go further in implementation. You have a ton of experience with the current system for Focus and keyboard navigation.

You may also want to pull down the latest code and play with ViewExperiments. It's not fully working but is showing some progress.

Thanks.

Link here: https://github.com/tig/Terminal.Gui/blob/8e70e2ae8faafab7cb8305ec6dbbd552c7bf3a43/docfx/docs/navigation.md

@tig tig changed the title Partially Fixes #2491. Adds ViewArrangement.Overlapped, refactors and beefs up Application and Toplevel Partially Fixes #2491. Refactors how Focus works Aug 2, 2024
tig added 2 commits August 13, 2024 15:59
Progess on thinking through new design, but not working yet.
@BDisp
Copy link
Collaborator

BDisp commented Aug 13, 2024

Change the title please, otherwise it will close the issue when merged.

@tig tig changed the title Partially Fixes #2491. Refactors how Focus works Partially Adresses #2491. Refactors how Focus works Aug 13, 2024
@tig
Copy link
Collaborator Author

tig commented Aug 28, 2024

Oh. Yeah, that happens in v2_develop as well.

Do you think it's a bug on the Dim.Auto (DimAutoStyle.Content)?

We're missing SetLayoutNeeded or LayoutSubviews somewhere.

@tig
Copy link
Collaborator Author

tig commented Aug 28, 2024

Oh. Yeah, that happens in v2_develop as well.

Do you think it's a bug on the Dim.Auto (DimAutoStyle.Content)?

We're missing SetLayoutNeeded or LayoutSubviews somewhere.

IIRC, I think I suspected it was someting to do with SubViewsNeedsLayout not getting set right. The logic of all of that is pretty complex.

@dodexahedron
Copy link
Collaborator

dodexahedron commented Aug 29, 2024

I'm pretty tired of working on this. It was a massive effort... but well worth it IMO.

I'm bummed about what's left:

  • TabGroup navigation doesn't work completely right with nested groups. These are edge cases that I may tackle later.
  • Fix TabView Scenario - above item may address
  • Fix Wizards - above item may address

I'm bummed because I want to work on something else for a bit.

At the same time, this is such a big PR I really want it merged.

Any objections to me leaving the above broken (I'll create Issues to track) for a bit?

Feel you, I do.

...Why does that sound so much worse than "I feel you?" I half wanted to put a 😏 on it.1

Footnotes

  1. Yeah I'm clearly 12. I mean it's right there in my name. 😅

@dodexahedron
Copy link
Collaborator

Oh forgot to say...

Yeah, I'll take a final pass over it after all your revisions, in case anything significant sticks out, which I'm starting as soon as I grab a source of caffeine.

Huge it definitely is, so it'll likely be a couple hours before I'm finished, simply to just read it all.

Loving the work. Focus was one of my biggest gripes from the before times, and this is a huge improvement.

Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

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

I'm not finding any show-stoppers after getting down to TabView.cs.

But it's time for me to hit the hay.

Biggest issues I do see are some pretty obvious performance issues like lines 323 through 325 of ComboBox.cs, where a collection is enumerated multiple times (up to 6 in the worst case). To be fair, that isn't newly broken, so I didn't comment on that and other similar things in-line that weren't actually changed in this PR aside from formatting.

But.

Can you at least drop a // PERF: Fix this comment or something at the top of OnHasFocusChanged on Line 308 of ComboBox.cs so it doesn't get overlooked later? That one in particular is just being needlessly cruel to the runtime.

There was one little syntax clarity tip I dropped in there, though, to demonstrate an alternative.

I'm gonna say, at this point, unless you want me to finish with the rest tomorrow, which is almost entirely test code, that it looks a'ight to me, with both the "@dodexahedron is bailing for the night" and the "@tig's bored of this one" understanding. 😅

@tig
Copy link
Collaborator Author

tig commented Aug 29, 2024

yYsChMT 3

@tig
Copy link
Collaborator Author

tig commented Aug 29, 2024

Fixed some overlapped stuff while on a flight. Not really in scope for this PR, but making progress against #2491.

Lots to do to make it all work exactly right, but its promising. With far less code than was required to support overlapped in v1.

rrRYBQH 1

@tig tig merged commit 56d9c59 into gui-cs:v2_develop Aug 29, 2024
4 checks passed
@@ -35,7 +35,7 @@ dotnet run
* [API Documentation](https://gui-cs.github.io/Terminal.GuiV2Docs/api/Terminal.Gui.html)
* [Documentation Home](https://gui-cs.github.io/Terminal.GuiV2Docs)

_The Documentation matches the most recent Nuget release from the `v2_develop` branch. The documentation for v1 is here: ([![Version](https://img.shields.io/nuget/v/Terminal.Gui.svg)](https://www.nuget.org/packages/Terminal.Gui))_
The above documentation matches the most recent Nuget release from the `v2_develop` branch. Get the [v1 documentation here](This is the v2 API documentation. For v1 go here: https://gui-cs.github.io/Terminal.Gui/api/Terminal.Gui.html)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The markdown is still broken.

Format for a URL is [text you want](url)

[v1 documentation here](This is the v2 API documentation. For v1 go here: https://gui-cs.github.io/Terminal.Gui/api/Terminal.Gui.html) is invalid markdown

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