Terminal.Gui
Terminal.Gui copied to clipboard
Partially Adresses #2491. Refactors how `Focus` works
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
- New design and implementation for how
Focusworks. - Partially Addresses #2491
- Fixes #3669
- Fixes #3645
Proposed Changes/Todos
- [x] 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
- [x] Figure out how
ViewArrangement.Overlappedwill work. UseTabIndexesand all theView.NextViewfocus stuff for navigation (instead of the code inOverlappedMoveNext). Use theSubviewordering (for just the subviews with ViewArrangement.Overlapped` set) to manage the z-order. - [x] Rewrite focus engine
- [x] Dozens of new, primitive unit tests
- [x] Updated migration doc
- [x] Massive code cleanup including making most of
View#nullable enable - [x]
View Experimentsscenario ->Navigation Tester - [x] TabGroup navigation doesn't work completely right with nested groups. These are edge cases that I may tackle later.
- [x] Fix
TabViewScenario - above item may address - [ ] Fix
Wizards- above item may address
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-Dto automatically reformat your files before committing. - [x] My code follows the Terminal.Gui library design guidelines
- [x] I ran
dotnet testbefore 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
@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
Change the title please, otherwise it will close the issue when merged.
I know this is a WIP but don't forget nullability context! In the top few lines of the classes I can already see places that need those annotations.
I know this is a WIP but don't forget nullability context! In the top few lines of the classes I can already see places that need those annotations.
Question: how does it work with partials? I'm resisting adding it to View.Navigation.cs in this PR because I don't want to totally upgrade all of View in this PR.
I know this is a WIP but don't forget nullability context! In the top few lines of the classes I can already see places that need those annotations.
Question: how does it work with partials? I'm resisting adding it to
View.Navigation.csin this PR because I don't want to totally upgrade all of View in this PR.
It's a per-file directive or, if paired with #nullable restore, can be limited to specific sections of code.
The effect on other parts of the class in other files is the same as a class in a file that has it turned on being accessed/used from another class in a file with it turned off.
There are also some even more granular ways to use it, for times when you want to be able to use the annotations but not get warnings in that file or get the warnings but not use the annotations.
Here are all the possible uses of the directive (from here):
- #nullable disable: Sets the nullable annotation and warning contexts to disabled.
- #nullable enable: Sets the nullable annotation and warning contexts to enabled.
- #nullable restore: Restores the nullable annotation and warning contexts to project settings.
- #nullable disable annotations: Sets the nullable annotation context to disabled.
- #nullable enable annotations: Sets the nullable annotation context to enabled.
- #nullable restore annotations: Restores the nullable annotation context to project settings.
- #nullable disable warnings: Sets the nullable warning context to disabled.
- #nullable enable warnings: Sets the nullable warning context to enabled.
- #nullable restore warnings: Restores the nullable warning context to project settings.
I found out about the annotations and warnings forms of it fairly recently, actually. Handy for places you want to gradually ease into it.
But IMO it makes the most sense to just turn it on for the whole project at this point, and people can just address warnings as they touch files or opt out rather than opt in, if they need to for some reason.
To silence the warnings in the build, so we only see them at design time in the IDE, you can add -nowarn:nullable to the build command line in CI or in the release build configuration in the csproj via <NoWarn>nullable</NoWarn> That would turn the current Release configuration in the csproj into this, to preserve existing suppressions and add suppression for nullability in release builds:
<PropertyGroup Condition=" '$(Configuration)' == 'Release' ">
<Optimize>true</Optimize>
<VersionSuffix></VersionSuffix>
<NoWarn>$(NoWarn);nullable</NoWarn>
</PropertyGroup>
Another option, which is a half-way means of doing it project-wide, would be to set the <Nullable> element in the project to annotations or warnings, to set it to one of those partial modes project-wide.
The nullable migration strategies document is quite helpful for understanding the best ways to ease into fully embracing nullability context.
But IMO it makes the most sense to just turn it on for the whole project at this point, and people can just address warnings as they touch files or opt out rather than opt in, if they need to for some reason.
Agree. But only after this PR is merged, ok?
Ok, @tig, @tznind, and @dodexahedron - this is ready for review!
Stuff I'm still working on:
- [x] I want to make
ViewExperiments.csbeNavigationTester.cs - [ ]
TabGroupnavigation doesn't work completely right with nested groups. These are edge cases that I may tackle later.
Ok, @tig, @tznind, and @dodexahedron - this is ready for review!
Stuff I'm still working on:
- [ ] I want to make
ViewExperiments.csbeNavigationTester.cs- [ ]
TabGroupnavigation doesn't work completely right with nested groups. These are edge cases that I may tackle later.
Recommend you read the new API docs and the migration guide and navigation.md as part of your review.
Will have a look in a little bit, once I get back home. π
But IMO it makes the most sense to just turn it on for the whole project at this point, and people can just address warnings as they touch files or opt out rather than opt in, if they need to for some reason.
Agree. But only after this PR is merged, ok?
I've got no problem with that.
I've long been operating with a .csproj.user file that has it turned on globally, anyway, among a few other analysis-related things I have turned up to 11. You should see the warning counts. π
They are somewhat higher than the limit of how high I can count (enjoy).
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.
- [x] Fix
TabViewScenario - 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?
@BDisp I think this fixes the crash you mentioned on the menu border.
That better but causes select the view under the Border.
There is another bug with this by only resizing the border height after click and moving another view.
@tig I sent you this PR https://github.com/tig/Terminal.Gui/pull/40 with suggestion for remove the ClearOnVisibleFalse property.
There is another bug with this by only resizing the border height after click and moving another view.
No bug. You're not actually selecting the Window by clicking on the Border. Click within the padding.
@tig I refer when I colapse the slip the height doesn't update until I move the mouse to another view.
@tig I refer when I colapse the slip the height doesn't update until I move the mouse to another view.
Oh. Yeah, that happens in v2_develop as well.
Oh. Yeah, that happens in v2_develop as well.
Do you think it's a bug on the Dim.Auto (DimAutoStyle.Content)?
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.
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.
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:
- [x] TabGroup navigation doesn't work completely right with nested groups. These are edge cases that I may tackle later.
- [x] Fix
TabViewScenario - above item may address- [ ] Fix
Wizards- above item may addressI'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.[^d12]
[^d12]: Yeah I'm clearly 12. I mean it's right there in my name. π
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.
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.