gui-cs / Terminal.Gui

Cross Platform Terminal UI toolkit for .NET
MIT License
9.68k stars 689 forks source link

LayoutSubviews should be restricted to the `ContentSize` and not `Viewport.Size` #3453

Closed BDisp closed 6 months ago

BDisp commented 6 months ago

@tig I think this wasn't happening before.

WindowsTerminal_J3BeGnIiw5

tig commented 6 months ago

@tig I think this wasn't happening before.

Uncheck ClearContentOnly and ClipContentOnly I have them on by default to illustrate how they work.

tig commented 6 months ago

See the API docs for these here:

https://gui-cs.github.io/Terminal.GuiV2Docs/api/Terminal.Gui.ViewportSettings.html

BDisp commented 6 months ago

What I want to get across with this post is that recalculating subview positions should only change when the ContentSize changes and not when the viewport size changes. Of course I'm referring to non-absolute Pos/Dim.

dodexahedron commented 6 months ago

That doesn't sound right to me. 🤔

If something is centered or otherwise relative to anything but top left, its position will (or should, anyway) change if the viewport changes, no?

tig commented 6 months ago

What I want to get across with this post is that recalculating subview positions should only change when the ContentSize changes and not when the viewport size changes. Of course I'm referring to non-absolute Pos/Dim.

Right you are.

Not sure how that happened or how I missed a unit test for it. I'll fix asap.

tig commented 6 months ago

That doesn't sound right to me. 🤔

If something is centered or otherwise relative to anything but top left, its position will (or should, anyway) change if the viewport changes, no?

Layout is tied to the "Content Area" which is defined by View.ContentSize. A subview with X = Pos.Center() will be at the center of ContentSize.Width.

By default Viewport.Size is the same as ContentSize. In that case, you are right.

However, the whole point of ContentSize is to enable a virtual content area that is independent from the Viewport. In this scenario, we set ContentSize to 60x40 and thus X = Pos.Center() should NOT be centered in Viewport.

tig commented 6 months ago

What I want to get across with this post is that recalculating subview positions should only change when the ContentSize changes and not when the viewport size changes. Of course I'm referring to non-absolute Pos/Dim.

Right you are.

Not sure how that happened or how I missed a unit test for it. I'll fix asap.

I see what I did. In cleaning up some old Dim.Auto code I had in LayoutSubviews I goofed. This is correct:

image

I'll fix this asap and add more unit tests!

1jQnEjm 1

dodexahedron commented 6 months ago

By default Viewport.Size is the same as ContentSize. In that case, you are right.

Yeah, that was the thought process involved there.

The rest makes sense and sounds good to me, as well. 👍

BDisp commented 6 months ago

I'll fix this asap and add more unit tests!

How about enable FineCodeCoverage again? This way we could have statistics on the APIs whose unit tests remain to be created. Of course, we have to exclude unit testing of operating system-specific drivers for now.