Preps for and partially Fixes #2491 - Refactors Toplevel and Application Navigation

[tig]

This is another PR paving the way for fixing

• Get rid of Toplevel - Introduce Runnable and Overlapped instead #2491

It is a fork of the branch in

• Partially Adresses #2491. Refactors how Focus works #3627

at the point where I had done a ton of refactoring, organization, code-clean up, and introduction of primitives required for #2491.

In order to prepare for addressing #2491 I moved a lot of code around here. In addition, I enabled nullability in big places, and re-organized View as well as Application to make more understandable and maintainable. As a result, this PR is BIG.

I want to get this merged because finishing #3627 is going to take a ton of work and I've been coding too much and need to take a break. At the same time, I don't want all the work I did to sit latent, especially since it moves a bunch of code around and will cause merge hell if not integrated quickly.

At the point where this PR's branch starts all unit tests pass and the new code paths are only active in these situations:

• View.TabStop is set to something other than TabBehavior.TabStop
• View.Arrangment is set to ViewArrangement.Overlapped

Fixes

• Refactors and cleans up existing Application and Toplevel code without removing existing Toplevel or Overlapped functionality.
• Makes Application, ConsoleDriver, and Toplevel #nullable enable
• Documents the hell out of existing code and adds more, lower-level, unit tests
• Creates ViewArrangement.Overlapped and modifies navigation code to support overlapped views that have this flag set.
• Creates TabBehavior enum and changes View.TabStop to be of this type.
• Fixes a bunch of issues in View.Navigation without breaking existing functionality.
• Modernizes navigation keys with CM:
  • Next/Prev TabStop: Tab and Tab.WithShift
  • Next/Prev TabGroup F6 and F6.WithShift (matches Windows, should work on all platforms)
• Partially Fixes Get rid of Toplevel - Introduce Runnable and Overlapped instead #2491

Proposed Changes/Todos

• Refactor Application into smaller files
• Make Application use #nullable enable
• Make ConsoleDriver use #nullable enable
• Review existing Overlapped and Toplevel unit tests. Add more as a way of learning.
• Moved view navigation out of Toplevel and into Application (via ApplicationNavigation static class)
• Moved OverlappedTop navigation out of Toplevel and into ApplicationOverlapped static class
• Make ViewExperiments be a good scenario for exercising all of the above and other stuff for Get rid of Toplevel - Introduce Runnable and Overlapped instead #2491.

For #3627 and beyond...

• 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.
• Attempt to implement IRunnable and replace Toplevel with that
• ...

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

[BDisp]

The View Experiments scenario should also move through the tab groups by pressing the F6. But it isn't navigating between the AdornmentsEditor and the Test Frame.

[BDisp]

I think that a Shortcut which now the StatusBar is using, must only have only a Key for each subview.

[tig]

The View Experiments scenario should also move through the tab groups by pressing the F6. But it isn't navigating between the AdornmentsEditor and the Test Frame.

Yep. This is why I stopped and forked so I can take the time to make it right in the other PR.

This does not effect use cases without nested TabGroups.

[tig]

I think that a Shortcut which now the StatusBar is using, must only have only a Key for each subview.

Not sure what you mean. Regardless can you please open a separate issue?

[BDisp]

Not sure what you mean. Regardless can you please open a separate issue?

I wrote a unit tests with a status bar above that reproduces this.

[tig]

Not sure what you mean. Regardless can you please open a separate issue?

I wrote a unit tests with a status bar above that reproduces this.

Great catch! Thank you!

Here's a better, more focused unit test based on the one you provided. Current is now internal which lets us have tests like this that don't require AutoInitShutdown/Begin. The nice thing about this is that if a test like this fails it won't cause others to fail. Where if any AutoInitShutdown test fails, it's likely to corrupt other tests making diagnosis difficult.

    [Fact]
    public void Changing_Key_Removes_Previous ()
    {
        var newActionCount = 0;

        Shortcut shortcut = new Shortcut (Key.N.WithCtrl, "New", () => newActionCount++);
        Application.Current = new Toplevel ();
        Application.Current.Add (shortcut);

        Assert.Equal (0, newActionCount);
        Assert.True (Application.OnKeyDown (Key.N.WithCtrl));
        Assert.False (Application.OnKeyDown (Key.W.WithCtrl));
        Assert.Equal (1, newActionCount);

        shortcut.Key = Key.W.WithCtrl;
        Assert.False (Application.OnKeyDown (Key.N.WithCtrl));
        Assert.True (Application.OnKeyDown (Key.W.WithCtrl));
        Assert.Equal (2, newActionCount);

        Application.Current.Dispose ();
    }

[tig]

Not sure what you mean. Regardless can you please open a separate issue?

I wrote a unit tests with a status bar above that reproduces this.

Great catch! Thank you!

Here's a better, more focused unit test based on the one you provided. Current is now internal which lets us have tests like this that don't require AutoInitShutdown/Begin. The nice thing about this is that if a test like this fails it won't cause others to fail. Where if any AutoInitShutdown test fails, it's likely to corrupt other tests making diagnosis difficult.

    [Fact]
    public void Changing_Key_Removes_Previous ()
    {
        var newActionCount = 0;

        Shortcut shortcut = new Shortcut (Key.N.WithCtrl, "New", () => newActionCount++);
        Application.Current = new Toplevel ();
        Application.Current.Add (shortcut);

        Assert.Equal (0, newActionCount);
        Assert.True (Application.OnKeyDown (Key.N.WithCtrl));
        Assert.False (Application.OnKeyDown (Key.W.WithCtrl));
        Assert.Equal (1, newActionCount);

        shortcut.Key = Key.W.WithCtrl;
        Assert.False (Application.OnKeyDown (Key.N.WithCtrl));
        Assert.True (Application.OnKeyDown (Key.W.WithCtrl));
        Assert.Equal (2, newActionCount);

        Application.Current.Dispose ();
    }

An even better version of the test:

   [Fact]
   public void Key_Changing_Removes_Previous ()
   {
       Shortcut shortcut = new Shortcut ();

       shortcut.Key = Key.A;
       Assert.Contains (Key.A, shortcut.KeyBindings.Bindings.Keys);

       shortcut.Key = Key.B;
       Assert.DoesNotContain (Key.A, shortcut.KeyBindings.Bindings.Keys);
       Assert.Contains (Key.B, shortcut.KeyBindings.Bindings.Keys);
   }

[BDisp]

@tig after the creation of the CheckStyle enum I already saw some bug on getting the right bool value for bool properties, like the following:

IsTopLevel = _frmMenuDetails.CkbIsTopLevel?.State == CheckState.UnChecked,
HasSubMenu = _frmMenuDetails.CkbSubMenu?.State == CheckState.UnChecked,

As you can see if any of the properties are false they will be set incorrectly as true because the State will always be equal to CheckState.UnChecked.

[tig]

@tig after the creation of the CheckStyle enum I already saw some bug on getting the right bool value for bool properties, like the following:

IsTopLevel = _frmMenuDetails.CkbIsTopLevel?.State == CheckState.UnChecked,

HasSubMenu = _frmMenuDetails.CkbSubMenu?.State == CheckState.UnChecked,

As you can see if any of the properties are false they will be set incorrectly as true because the State will always be equal to CheckState.UnChecked.

I obviously made some mistakes.

Not sure why you are commenting here instead of opening a new Issue though.

[BDisp]

I obviously made some mistakes.

Not sure why you are commenting here instead of opening a new Issue though.

Because their fix make more sense to be done in this branch as it has already fixes for shortcuts. But I'm still investigating why shortcuts menus aren't working on Window.