Fixes #2489. Build scrolling into View.

[BDisp]

Fixes

• Fixes Create new ScrollBar based on a new Scroll and remove ScrollBarView/ScrollView #2489

Proposed Changes/Todos

• Added ViewScrollBar.cs as partial View
• Implemented handling for layout and mouse with adornments.
• Implementing the built-in scroll bar on the views that already was using scroll bar.
• TextView need to have an additional column for the cursor on the last column be always visible and now the word wrap behaves differently on edit and read-only mode.

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

[tig]

Oooh! I'm excited to dive into this.

[tig]

Not done reviewing yet, but here's some initial thoughts.

What you're doing here is GREAT.

[tig]

I think:

• the v2 class should be named ScrollBar
• An instance of ScrollBar should act 100% independently of whether there is "another scrollbar" or not. Any logic that coordinates two scrollbars should be outside of the ScrollBar class.
• Logic for "auto hiding" scroll bars should NOT be in ScrollBar. External code should be able to just do scrollBar.Visible =.
• The public API ScrollBar exposes should be primarily "set these properties and subscribe to these events"; Normal use should not require overriding methods.

[BDisp]

I think:

• the v2 class should be named ScrollBar

I agree and I'll do it i this PR.

• An instance of ScrollBar should act 100% independently of whether there is "another scrollbar" or not. Any logic that coordinates two scrollbars should be outside of the ScrollBar class.

To do this, simply start each view that wants to use separate scroll bars such as ScrollBarType = ScrollBarType.Vertical or ScrollBarType = ScrollBarType.Horizontal and manipulate what each one should do differently from the other.

• Logic for "auto hiding" scroll bars should NOT be in ScrollBar. External code should be able to just do scrollBar.Visible =.

The Visible property is actually the one that manipulates its visibility and its value is constantly changed whenever its values are resized or reset. Therefore, in my opinion it should only be used to manipulate the visibility imposed by auto-hide. But the user may not want to draw the scroll bar even if it is necessary or want to draw it even if it is not necessary. To enforce this condition, there are the AutoHideScrollBars and ShowScrollIndicator properties. If AutoHideScrollBars is false you can manipulate the visibility at will using the ShowScrollIndicator property. That's why the Visible property is reserved for the scroll bar so that it can be manipulated at will when the AutoHideScrollBars property is true. Therefore, it is not worth manipulating the Visible property directly.

• The public API ScrollBar exposes should be primarily "set these properties and subscribe to these events"; Normal use should not require overriding methods.

Yes, the API should explain how to use the scroll bar in a view. Of course, TextView and TableView are more complex examples because they already have automatic management of these adjustments defined, as they can be used without a scroll bar.
With this PR it is now possible to use negative limits so that a view does not need to implement left and top offsets, but only provides the size of the columns and rows. See the ScrollBarBuiltIn scenario.

[BDisp]

the v2 class should be named ScrollBar

Done.

[tig]

The superView that has a ScrollBar in it (which will usually be Padding) can do

_scrollBar.VisibleChanged += ScrollBar_VisibleChanged

And update it's state as appropriate. I still don't think this property is needed.

[BDisp]

The VisibleChanged event is really not necessary and I will remove it in this PR.

[BDisp]

The ShowScrollIndicator is necessary as I explained before because the Visible property is reserved for the scroll bar to handle auto-hide if it is true.

Edit:
Do you want I still maintain the comment in the line 136?

[BDisp]

The VisibleChanged event is really not necessary and I will remove it in this PR.

I only need to use the Parent.VisibleChanged and Parent.EnabledChanged to set the scroll bar visibility and inability if the superview has changed that properties.
Done.

[tig]

Ok, I got through all/most of it and played with the changes locally.

This is just fantastic stuff! It is going to really simplify the API.

I view what you've done so far as a proof-of-concept prototype that absolutely proves we can make this work elegantly.

Now, the challenge is to take all you've learned, step back, and engineer the most elegant new implementation possible.

Great work @BDisp!!!

[tig]

I am confused here.

Why is this not just Parent.Bounds?

[BDisp]

Because the Parent.Bounds may have negative location. Don't forget that this PR now allow using negative bounds location and thus the dimension is also changed. The ContentArea is only the available space inside adornments used to set the driver clip area.

[tig]

I'm confused.

Can you please update the API docs for Bounds to represent what you've done here?

[BDisp]

I'm still working on this and I didn't decided well what I'm going to do. I'm testing the ScrollBar on Adornments and on ContentArea but he behaves differently related with dimensions.

[tig]

I'm still working on this and I didn't decided well what I'm going to do. I'm testing the ScrollBar on Adornments and on ContentArea but he behaves differently related with dimensions.

Ok, I'm going to avoid thinking deeply about Bounds v ContentArea until you tell me it's time. For me, the best way to gain clarity on things like this is to write the API docs as clearly as possible. I understand you're less comfy about this, but I encourage you to give it your best shot. Happy to riff with you on it if that helps!

[tig]

What would you say to the API looking like this:

...
Source = new ListWrapper (_categories),
HorizontalScrollBar.Enabled = true,
VerticalScrollBar.Enabled = true,
...

?

[BDisp]

Why Enabled? This makes sense if it was deactivated before, being visible, but without the user being able to interact with it. For me, just one statement is simpler, such as CategoryList.Padding.ScrollBarType = ScrollBarType.Both; in case we want to put it in Padding, of course.

[tig]

Why Enabled? This makes sense if it was deactivated before, being visible, but without the user being able to interact with it. For me, just one statement is simpler, such as CategoryList.Padding.ScrollBarType = ScrollBarType.Both; in case we want to put it in Padding, of course.

Because Enabled is perfectly suited for this, and already exists. Why add (or not remove) API surface area if there's already a primitive that is there?

[BDisp]

For me Enabled=true is toggle from grayed to normal color, that why I asked. If I remember when I created that property was for that propose.

[tig]

Ahh... Yes. the docs say "Gets or sets a value indicating whether this can respond to user interaction."

I don't think we should change that. Your question is great.

We need two things:

1. A way for a View subclass to say "Scrollbars make no sense for me." I was thinking Enabled would be good for that. It's not.

Ideas for what this mechanism could be:

• On View: public ScrollBar HorizontalScrollBar - If the property is null then duh. The question here is who's responsible for creating the ScrollBar instance? Lots of choices.
• On View: public ScrollBarType ScrollBarType - yuk. I just don't like adding more and more enums.

Other ideas?

2. A way for a user of a View subclass (or the subclass itself) to say "make the the H or V scrollbar be visible".

This should be the Visible property on ScrollBar. For example _myView.HorizontalScrollBar.Visible = true.

[tig]

AutoHide should be a property on ScrollBar, not View.

This will provide better encapsulation and will enable the automatic behavior work on either the horizontal or vertical (or both).

[BDisp]

But I'm not exposing the ScrollBar in the View class. That's why the property exists in the View class.

[tig]

Right. I get it now.

I think the name of this property should be AutoShowScrollBars and it should be false by default.

When it gets set to true this logic should get executed (in Padding, IMO):

Parent.VerticalScrollBar.Visible = Parent.ShouldTheVerticalScrollbarBeVisible();
Parent.HoriztonalScrollBar.Visible = Parent.ShouldTheHorizontalScrollbarBeVisible();;

When it gets set to false this logic should get executed (in Padding, IMO):

Parent.VerticalScrollBar.Visible = false;
Parent.HoriztonalScrollBar.Visible = false;

Note ShouldTheHorizontalScrollbarBeVisible is psuedo-code for some protocol that lets Padding ask the Parent the question. It could be a virtual method on View or it could be an event, or it could be a property on Padding that the Parent sets.

If either scrollbar has Enabled == false these calls will (and should) do nothing. Because, by definition of Enabled, the view effectively does not exist. This would be the case for a View where having scrollbars does not make sense (e.g. Button).

[BDisp]

But instead of Enabled=false why not Visible=false. Why you want to show a grayed ScrollBar?. This is really make to much confusion to me and I have others broken code to try to fix. If a ScrollBar doesn't make sense on a view I think is better force set on a override Visible to false.

[tig]

But instead of Enabled=false why not Visible=false. Why you want to show a grayed ScrollBar?. This is really make to much confusion to me and I have others broken code to try to fix. If a ScrollBar doesn't make sense on a view I think is better force set on a override Visible to false.

I was not remembering what Enabled did correctly. My bad. See my comment above about needing two things: A way for a View to say "I don't do scroll bars" and a way for scroll bars to be made visible/notvisible IF they're possible.

[BDisp]

Actually any view can have ScrollBar. Let the user choose what he want.

[tig]

Same comment as above. This is too tightly coupled.

[tig]

That said, this piece of code is central to making all this work. At some point, ScrollBar needs to have a protocol with the view whose content is being scrolled.

What is all the information this protocol needs to convey:

• Focus (both can and has)
• The size of the content.
• The size of the visible window into the content
• ...

What else? Let's get this list flushed out and then design a concise protocol (a set of properties, methods, interfaces, etc...) that cleanly encapsulates them.

[BDisp]

Same comment as above. This is too tightly coupled.

This is not a coupling case. It is only to ensure that the Parent is focused if an adornment is clicked if the Parent can be focused.

[tig]

Same comment as above. This is too tightly coupled.

This is not a coupling case. It is only to ensure that the Parent is focused if an adornment is clicked if the Parent can be focused.

I don't see how you can say this is NOT a coupling case. The code above (in ScrollbarView) has the word Adornment in it. That is coupling.

I believe it is possible for ScrollBar to have NO knowledge of Adornment (other than what it inherits from View, of course).

[BDisp]

But the ScrollBar need to access the information about the position and size of the parent. Remember the ScrollBar now will be part of the View class.
But the fact of the ScrollBar can be added on any view sub-class, including adornments, the latest commits broken something.

[BDisp]

git checkout v2_develop ./Terminal.Gui/Views/ScrollBarView.cs ./Terminal.Gui/Views/ScrollView.cs

But I also renamed ScrollBarViewTests to ScrollBarTests and I have various changes on both files. If I checkout them from the v2_develop branch I'll have a bunch of conflicts, right? I only renamed because you said on a comment.

[BDisp]

I've looked at it closely. A view referencing it's SuperView.Bounds is not a problem. A view that knows about Parent.Bounds is bad, unless it is a class derived from Adornment.

Many of the calculations of the ScrollBar is based of the size of the SuperView. Thus it need to check it to decide it need to scroll or not. With the implementations of negative bounds a ScrollBar thay is on the ContentArea will have much more problems to calculate the size because the SuperView bounds will have a higher value than the visible ContentArea and the ScrollBar must only use the visible ContentArea size. But because the ScrollBar dimensions use Dim() or Dim(1) it will overflow the visible ContentArea size. I hope you can understand this and you may some better suggestion. With Adornments this isn't a problem because they aren't use negative bounds.

[BDisp]

Now I'm thinking that it was the ScrollView that should be renamed ScrollBar and not the ScrollBarView. I will reverse this when this PR is more solid.
See the ScrollBars scenari which have ScrollBar on all Adornments and ContentArea with mouse handling. For negative bounds we have to use Pos/Dim absolute values to position the ScrollBar. I hope you understand why I need to access to the SuperView.

[tig]

Many of the calculations of the ScrollBar is based of the size of the SuperView.

No, this is not strictly true.

The right statement is:

Nany of the calculations of the ScrollBar need to use information known by the SuperView when the ScrollBar is used w/in Padding."

In an earlier post I listed exactly what those things (there are 7 of them...unless I'm missing something, which is possible) are.

1. The position and dimensions of the ScrollBar: ScrollBar.X/Y/Height/Width.
2. The size of the virtual content: ScrollBar.Size
3. The position w/in the virtual content that scrollbar is pointing to: ScrollBar.Position
4. The size of the visible area: Parent.Bounds.Size.
5. The offset into the virtual content that the (0,0) of the visible area resides at. Parent.Bounds.Location
6. Whether the parent has focus or not. SuperView.HasFocus
7. The ColorScheme of the parent. SuperView.ColorScheme

These are just data. They can be provided to ScrollBar in many different ways. One way is by having ScrollBar access SuperView, which you keep insisting is the only way.

The way I think will make the new ScrollBar simpler and more flexible is to ensure ScrollBar supports an API (a set of properties/methods) that let those things be known to ScrollBar.

1, 2, and 3 are already handled this way. 6 and 7 can still work via SuperView because that's how most other views work for focus and colorschemes.

For 4: You could add a public Size VisibleArea to ScrollBar.

For 5: You could add a public Point ContentOffset to ScrollBar.

[BDisp]

I already reverted the rename of the ScrollBarView and ScrollBarViewTests.

[BDisp]

I'm reopen this because I'm convinced that the SetRelativeLayout should handled the location offset, although until now it wasn't never used. This is only to proof that and not for be merged, of course.

WindowsTerminal_ECSMSxAgT4.mp4

[BDisp]

Your design is flawed. For example, notice how when I scroll the Centered Button moves relative to the other subviews in the Content area.

Yes you are right. That was because PosCombine was offsetting twice, left and right targets. I already fixed.

Also , try scrolling with mouse with the cursor over one of the subviews in the Virtual Demo View; scrolling stops. I ensured this all works.

You are right. I didn't worry about that. The only point I wanted to transmit was the offsetting is possible during the layout, instead of using always the location (0,0) and then doing the scrolling after.

I addressed all of this in a primitive way in my PR.

Yes and I really appreciated your work, but in my opinion I think you underestimated the negative location. It's possible to handle with Positive and Negative ContentOffset. If it's positive then the view will be draw scrolled to the right/bottom and if it's negative the view will be scrolled to left/top.

I feel scrolling should work great without scrollbars. Scrollbars, if visible should just provide a visible indication of where the content is scrolled and an additional way of scrolling. Your design is built around Scrollbars. Mine is completely abstracted from whether they exist or not.

Well that isn't completely right. This PR has code for scrollbars and without scrollbars handling as well. Please see better.

What you've done is take the existing ScrollBarView and ScrollView code, which has always been overly complicated because it tried to hack around limitations in v1's View design, and forced that code into View. You've done an admirable job of making it mostly work, but it is a bad design.

Well if you are talking about I'm using negative location, I think that is the right approach, unless the others API are also wrong.

I revisited the core design and ensured the v2 View drawing and layout code deal with a virtual content area intrinsically. An example: In my design if ContentSize has not been set, then there's no scrolling; ContentSize is assumed to be == Viewport.Size. If the dev sets ContentSize to be bigger than Viewport.Size things just work cleanly. In your code, the dev has to set UseContentOffset and set ContentSize. And then your ContentArea also has a Size, but it's not the same as ContentSize, which is just confusing and hard to explain.

My model is pretty simple, comparatively:

The use the UseContentOffset property is for the views that implemented the scrollbars but don't want to use the ContentOffset to re-draw because they have his own offsetting handing. Thus the ContentOffset and the ContentSize will only be used by the scrollbars and key navigation if the view doesn't handle them.
I was wrong when I said that the ContentArea.Size isn't the same as the ContentSize and I apologize for that. I didn't test before an view that has subviews and I didn't realized my mistake. Now my code is modified reflecting the right behavior, which is the ContentArea.Size is always the same as the ContentSize if UseContentOffset is true.
My approach is more generic to views and prepared to use scrollbars or not.

• A view has content. The content is bound by (0,0) in the top-left and ContentSize for width & height. There's no need for a property for this.

Before you are considered using negative bounds and now you are completely against. But now I'm not talking about negative location. Now I'll talking about positive offset. Imagine you want to draw the ContentArea from (2,3) and of course the ContentOffset will be equal to X=2,Y=3. Since you are always bounds to (0,0) you can't doing what I'm proposing.

• What is visible to the user is the Viewport which is bound by the inner rectangle of Padding. The Viewport has a location which describes where in the content it is positioned (it's location). The Viewport has a size, which describes the number of columns and rows visible.

As I said above the Viewport is the visible area to the user, but if the location (2,3) is still visible to the user you can't start the redraw at that location.

• Changing Viewport.Location changes its location within the content (e.g. the start of the content the user sees).

You see here. So it's always equal or greater than zero but always starting at (0,0) of the Viewport. This is a limitation.

• Changing Viewport.Size changes it's size (eg. how much of the content the user sees). This will also change the Frame size appropriately.
• ContentSize is the same as Viewport.Size until a dev changes it. Once he/she does, scrolling magically is enabled and adjusting Viewport works as expected.
• The Viewport can be larger, smaller, or the same size as ContentSize. If larger, zoom scenarios are enabled.

I agree.

• There's no need for devs to worry about ScrollBars if they don't want them.

Well it seems you are killing for good the use of scrollbars. Please don't use them on your applications tools, like the VS2022. It's bad.

I also think you're trying to do too much. For example, I don't think the logic for what a page is should be built into view. For example, not all use-cases define "page" as the height of the visible content.

For me a page size is always the visible content area size. This is very useful for users navigate faster horizontal and vertical for each visible content area and thus never loose anything on viewing them.

[BDisp]

Some more improvements.

WindowsTerminal_MGm6mXDBDO.mp4

WindowsTerminal_Jl0v1eUPWC.mp4

[tig]

I am still unsure why you are spending time continuing to work on this instead of helping get #3323 right/done.

[BDisp]

I am still unsure why you are spending time continuing to work on this instead of helping get #3323 right/done.

I'm just trying to prove that the layout supports negative locations. I'm not trying to impose anything or ask for anything. Pretend this doesn't exist. I'm busy with other things but of course I will collaborate on #3323. Comment in #3323 what you specifically want me to collaborate on and I'll try to help as best I can.