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 TabView. #3828

Merged
merged 18 commits into from
Nov 21, 2024
Merged

Fixes TabView. #3828

merged 18 commits into from
Nov 21, 2024

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Nov 18, 2024

Fixes

  • Fixes #_____

Proposed Changes/Todos

  • Remove internal layout and draw.

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 BDisp requested a review from tig as a code owner November 18, 2024 23:27
@BDisp
Copy link
Collaborator Author

BDisp commented Nov 19, 2024

@tig you have to understand that the _contentView field is necessary. First it doesn't have to do with the old ContentView as the v1 and finally it's needed to accommodate the Tab.View which can be a view that hold others views but also can be only one view, like the TextField that can't be resized with Dim.Fill for the Height property because it's constrain to 1. So, the best solution was renamed it to _containerView. If I'm wrong please let me know.

@tig
Copy link
Collaborator

tig commented Nov 19, 2024

@tig you have to understand that the _contentView field is necessary. First it doesn't have to do with the old ContentView as the v1 and finally it's needed to accommodate the Tab.View which can be a view that hold others views but also can be only one view, like the TextField that can't be resized with Dim.Fill for the Height property because it's constrain to 1. So, the best solution was renamed it to _containerView. If I'm wrong please let me know.

Have you looked at the re-write I started here

It approaches the design very differently. I'd be happy to riff with you on how to finish that.

@tig
Copy link
Collaborator

tig commented Nov 19, 2024

Ooops. Never mind that link. I can't find the PR i meant. Hold a sec.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 19, 2024

Here's the reason why we need _containerView where the hi is a TextField and the TabView is using his border as also the Tabs and the _containerView. Without it we can't set the TextField border the same way. I know we can workaround it by insert padding thickness but the TabView isn't aware of this necessity.

┌───────┐
│╭────╮ │
││Tab1│ │
││    ╰►│
││hi   ││
││     ││
││     ││
││     ││
││     ││
││     ││
│└─────┘│
└───────┘

@tig
Copy link
Collaborator

tig commented Nov 19, 2024

I have to run now. I'd like to show you the re-write I started to show that there's a completely different way of making TabView work that's much simpler, peformant, and clean.

I agree that the containerView is needed for the OLD design. But it is not needed if the design is different.

I'll try to clean up my screw up later today.

@tig
Copy link
Collaborator

tig commented Nov 19, 2024

@BDisp Please see this: #3832

If you want to keep trying to fix the existing class, that's fine. However, I'd much rather our joint energy go into a new design. It is not a priority for me to work on this right now, and I'd be more than happy for you to take what I started in #3832 and finish it. If you choose not to, and just hack the current design to work slightly better, then at some point in the future when I have the time/motivation I'll finish the new design.

FWIW, the visual and focus issues you are trying to fix are just the tip of the iceberg for problems with the existing code. To illustrate this, note the performance of TabViewExample:

v2_develop:
image

v2-TabViewV2
image

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 19, 2024

Yes, yours really faster because I'm using the OnSubviewLayout method instead of reusing the unchanged previous tabs. But I prefer you continue with it because you're using scrolling and you have more experience with it. Good work.

@tig
Copy link
Collaborator

tig commented Nov 19, 2024

Yes, yours really faster because I'm using the OnSubviewLayout method instead of reusing the unchanged previous tabs.

The existing code is mixing layout and draw in a very deep way. It's not as simple as you say. It is a fundamentally broken design (for v2... for v1, it was the best we could do).

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 19, 2024

The existing code is mixing layout and draw in a very deep way. It's not as simple as you say. It is a fundamentally broken design (for v2... for v1, it was the best we could do).

Yes, I agree the existent code in the v2_develop is mixing that but I think you didn't look at my PR to check that it isn't already using layout and draw directly from the TabView.

@tig
Copy link
Collaborator

tig commented Nov 19, 2024

The existing code is mixing layout and draw in a very deep way. It's not as simple as you say. It is a fundamentally broken design (for v2... for v1, it was the best we could do).

Yes, I agree the existent code in the v2_develop is mixing that but I think you didn't look at my PR to check that it isn't already using layout and draw directly from the TabView.

You're right, I didn't. I just looked at the code. Nice work:

image

@tig
Copy link
Collaborator

tig commented Nov 19, 2024

Apologies for not actually running this before. I had assumed you had seen my PR for a rewrite and thus didn't look at this closely. Then I figured out that I screwed that rewite PR up and it was missing.

I'm good with getting this merged asap as it addresses my biggest issues. But I still want to explore a rewrite at some point (not urgent/important!)

@@ -33,5 +33,7 @@ P P
output
);
TestHelpers.AssertDriverAttributesAre ("0", output, null, view.GetNormalColor ());

((FakeDriver)Application.Driver!).End ();
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this for?

This should not be needed???

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, because sometimes it causes failure in the With_Subview_Using_PosFunc unit test. See https://github.com/gui-cs/Terminal.Gui/actions/runs/11903293490/job/33169963032. The reason is a racing condition where the SetupFakeDriver was resizing the terminal to (5,5) and when the above unit test is running the terminal size isn't what it's expected and corrupt the expected value.

  Failed Terminal.Gui.LayoutTests.DimAutoTests.With_Subview_Using_PosFunc [3 ms]
  Error Message:
   Assert.Equal() Failure: Values differ
Expected: 25
Actual:   20
  Stack Trace:
     at Terminal.Gui.LayoutTests.DimAutoTests.With_Subview_Using_PosFunc() in /home/runner/work/Terminal.Gui/Terminal.Gui/UnitTests/View/Layout/Dim.AutoTests.PosTypes.cs:line 647
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Copy link
Collaborator

Choose a reason for hiding this comment

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

[SetupFakeDriver] has a bug then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably, I'll take a look on it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I fixed with fabe34f.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 19, 2024

I already set the TabStop = TabBehavior.TabStop of the Tab class to allow to be navigate, but when I'm in the Interactive Tab which has two TextField, pressing the cursor up key doesn't not navigate to the TabRowView that is a TabView subview. But on the Tab2 where has a TextField, pressing the cursor up key it navigate to the TabRowView. Why these different behaviors?
Does this is related with the #3831?

WindowsTerminal_pHMHIRPFgo

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 19, 2024

If you press the cursor down on the second TextField it doesn't also navigate to the next view of the superview.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 20, 2024

Before 6cb8478 commit:

Captura de ecrã 2024-11-20 123805

After:

Captura de ecrã 2024-11-20 124213

There isn't very big differences but would be great if you add a LayoutCompleteCount in the benchmark, please. Thanks.

@tig tig merged commit 2fe4961 into gui-cs:v2_develop Nov 21, 2024
4 checks passed
@BDisp BDisp deleted the v2_tabview-fix branch November 21, 2024 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants