gui-cs / TerminalGuiDesigner

Forms Designer for Terminal.Gui (aka gui.cs)
MIT License
423 stars 28 forks source link

Update to latest v2 branch to 2.0.0-v2-develop.2168 #292

Closed tznind closed 2 weeks ago

tznind commented 2 months ago

Current broken code from changes in v2 are:

QA Testing

As noted we should now target nuget package

https://www.nuget.org/packages/Terminal.Gui/2.0.0-prealpha.216

Blocker issues:

Currently only 12 failing tests and all are the result of:

tznind commented 3 weeks ago

@tig / @BDisp the keyboard handling seems to have changed and is no longer behaving itself with regards to handled and order of execution.

For example

Expected behavior:

Actual behavior:

Here is the relevant code:

  private void ListView_KeyPress(object? sender, Key key)
  {
      // if user types in some text change the focus to the text box to enable searching
      var c = (char)key;

      // backspace or letter/numbers
      if (key == Key.Backspace || char.IsLetterOrDigit(c))
      {
          this.searchBox?.FocusFirst(TabBehavior.TabStop);
          this.searchBox?.OnKeyDown(key);
          key.Handled = true;
      }
  }

Another example is when editing a property.

The screen flashes (property save change but then re opens again).

Only way to use keyboard to finish selection is if you manually tab to the button and complete selection with 'space' on the Button.

If you could tell me whether this is a bug in Terminal.Gui or something I am doing wrong that would be super appreciated.

Relevant code is primarily in: TerminalGuiDesigner\src\UI\Windows\BigListBox.cs

BDisp commented 3 weeks ago

Can ou provide a unit test for the TG? @tig is on the https://github.com/gui-cs/Terminal.Gui/issues/3655 and maybe he can see what is going on. Thanks.

tznind commented 3 weeks ago

Ok I have fixed by switching to invoking OnProcessKeyDown instead of OnKeyDown for TextField

And have standardized on KeyDown for all event handlers.

I think previously if you Handled=true on KeyDown you would not then get a KeyUp too. Now you still do. Either that or it was a side effect that it worked.

Having all be KeyDown makes things better.

tig commented 3 weeks ago

Ok I have fixed by switching to invoking OnProcessKeyDown instead of OnKeyDown for TextField

You should invoke NewKeyDownEvent. But only if you are certain the key is directly handled by the View's KeyBindings or virtual overrides.

OnKeyDown/OnProcessKeyDown should not be public virtual but protected virtual. This is a bug that needs to be fixed.

Ideally, to simulate key events, you go through Application.OnKeyDown. If you don't, then any Application-scoped keybindings a view registered will not be fired.

And have standardized on KeyDown for all event handlers.

I think previously if you Handled=true on KeyDown you would not then get a KeyUp too. Now you still do. Either that or it was a side effect that it worked.

Having all be KeyDown makes things better.

KeyDown/KeyUp are decoupled at the Application and View level in v2. KeyUp is generated by the driver (simulated on drivers where the platform doesn't actually support it).

tznind commented 2 weeks ago

Only failing test is now crashing in init step as described in https://github.com/gui-cs/Terminal.Gui/issues/3665

BDisp commented 2 weeks ago

When I run the two tests separately I get no failures. When I run them together by selecting the file in the Test Explorer, sometimes both fail, sometimes one fails. It must be some object that is not being properly disposed when the other test starts. Any idea what could be happening? Some method that is initializing Application but not disposing of it completely before starting the next test.

tznind commented 2 weeks ago

The error is in Tests base class but that is the base class for every test class and only TableViewTests has this issue.

Ah, now I see there is this attribute

[Parallelizable( ParallelScope.Children )]

Removing it fix problem. I guess that make sense we can only have 1 Application at once.

BDisp commented 2 weeks ago

Removing it fix problem. I guess that make sense we can only have 1 Application at once.

Yes, TG is a singleton API.

BDisp commented 2 weeks ago

I'm still having others unit tests failures:

image

tznind commented 2 weeks ago

Designing the MenuBar is currently not working great in this nuget package version of Terminal.Gui

I've updated comments on PR as this is a deal breaker for merging this.

I've raised worst case which is: https://github.com/gui-cs/Terminal.Gui/issues/3667

But there is also this unrelated one

Global Exception
Index was outside the bounds of the array.
   at Terminal.Gui.MenuBar.NextMenu(Boolean isSubMenu, Boolean ignoreUseSubM
enusSingleFrame)
   at Terminal.Gui.Menu.<BeginInit>b__15_0()
   at Terminal.Gui.View.<>c__DisplayClass263_0.<AddCommand>b__0(CommandConte
xt ctx)
   at Terminal.Gui.View.InvokeCommand(Command command, Key key, Nullable`1 k
eyBinding)
   at Terminal.Gui.View.InvokeKeyBindings(Key key, KeyBindingScope scope)
   at Terminal.Gui.View.OnInvokingKeyBindings(Key keyEvent, KeyBindingScope 
scope)
   at Terminal.Gui.Menu.OnInvokingKeyBindings(Key keyEvent, KeyBindingScope 
scope)
   at Terminal.Gui.View.NewKeyDownEvent(Key keyEvent)
   at Terminal.Gui.View.NewKeyDownEvent(Key keyEvent)
   at Terminal.Gui.Application.OnKeyDown(Key keyEvent)
   at Terminal.Gui.Application.Driver_KeyDown(Object sender, Key e)
   at Terminal.Gui.MainLoop.RunIteration()
   at Terminal.Gui.Application.RunIteration(RunState& state, Boolean& firstI
teration)
   at Terminal.Gui.Application.RunLoop(RunState state)
   at Terminal.Gui.Application.Run(Toplevel view, Func`2 errorHandler
BDisp commented 2 weeks ago

Can you test with the PR https://github.com/gui-cs/Terminal.Gui/pull/3663 and see if it's sill failing please. Thanks.

tznind commented 2 weeks ago

Can you test with the PR gui-cs/Terminal.Gui#3663 and see if it's sill failing please. Thanks.

Not sure how to do that short of adding Terminal.Gui as a git submodule. I can wait and just assume that this will fix though - it looks about right and I notice that it does not always occur that it misbehaves.

BDisp commented 2 weeks ago

But are you calling FindDeepestView in TGD?

tznind commented 2 weeks ago

I do call FindDeepestView in TGD but I think in this case it just a simple null reference because MenuBar does not have Margin

BDisp commented 2 weeks ago

I do call FindDeepestView in TGD but I think in this case it just a simple null reference because MenuBar does not have Margin

All views have adornments (Margin, Border and Padding). So, MenuBar also have. Adornments don't have adornments and if you call FindDeepestView by sending, by mistake, the MenuBar.Margin in the parameter, it may throw that error. See what start parameter object is.

tznind commented 2 weeks ago

In debugger the view being looked at is MenuBar and it does not have Margin, from call stack it is also not originating in TGD code: image

BDisp commented 2 weeks ago

This test doesn't fail in the TG:

    [Fact]
    public void MenuBar_Has_Adornments ()
    {
        var mb = new MenuBar ();
        Assert.NotNull (mb.Margin);
        Assert.NotNull (mb.Border);
        Assert.NotNull (mb.Padding);
    }
BDisp commented 2 weeks ago

Another explanation is you're using a MenuBar that was already disposed but not set to null yet. Now TG only dispose the subviews and the adornments when Dispose method is called.

BDisp commented 2 weeks ago

I'm still having others unit tests failures:

image

Forget. Only after deleting the bin and obj folders from TGD and UnitTests, the unit tests now pass.

tznind commented 2 weeks ago

@tig , @BDisp I have begun testing this, it needs to be stable and working to go into v2.

Hopefully this will not throw up any show stoppers in Terminal.Gui library that would require bugfixes and hence bumping nuget version (and taking in everything else that's added since 2168 - with all the breaking changes and re-testing that that would incur).

As a real world test I am updating nfirestore-cli which is my Firestore Tui that I wrote in v2.

I have added checkboxes for all the issues I discover in OP above under QA Testing header - for those that I can repro directly with standalone program I will raise issue in main library and link the checkbox.

tznind commented 2 weeks ago

@BDisp / @tig I've done a lot of testing and fixed the issues I've found. Do either of you want to take it for a spin and kick the wheels - see if you can find any other issues?

If not lets get it merged!