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

[BDisp]

Fixes

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

Proposed Changes/Todos

• Add Scroll class and unit tests
• Add ScrollSlider class and unit tests
• Add ScrolBar class and unit tests
• Add ScrollButton class
• Add @tig View.ScrollBars
• Remove ScrollBarView
• Remove ScrollView

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]

I would appreciate the Scroll class being reviewed before moving forward with the ScollBar class. Thanks.

[tig]

Ooooh! I'm super excited to review this! Will do asap.

[BDisp]

The width/height of the Scroll is not limited to 1 but only to what the user wants.

[BDisp]

@tig how about you creating a new Content or Viewport box in the AdornmentsEditor to set the foreground and background colors. Instead of using Top, Right, Bottom and Left, it can use the Axis, ContentSize or whatever. What do you think?

[tig]

Really slick!!

I've got a PR with some changes that you might like. I'll submit it vs. making a lot of comments.

[tig]

@tig how about you creating a new Content or Viewport box in the AdornmentsEditor to set the foreground and background colors. Instead of using Top, Right, Bottom and Left, it can use the Axis, ContentSize or whatever. What do you think?

You mean being able to set the Normal.Background/Forground color for the View itself? Like the colorpicker in Content Scrolling does?

[BDisp]

You mean being able to set the Normal.Background/Forground color for the View itself? Like the colorpicker in Content Scrolling does?

Yes or for the view passed on the ViewToEdit property. That's more or less the idea.

[BDisp]

@tig did you reorganize by placing the backfields before the properties by hand, or through some automatic system? I normally use R#'s Ctrl+E+C and it's not reformatting that way.

[tig]

@tig did you reorganize by placing the backfields before the properties by hand, or through some automatic system? I normally use R#'s Ctrl+E+C and it's not reformatting that way.

By hand. It's still broken.

[tig]

You mean being able to set the Normal.Background/Forground color for the View itself? Like the colorpicker in Content Scrolling does?

Yes or for the view passed on the ViewToEdit property. That's more or less the idea.

Greet idea. I'll do it as part of #3376 where I've already completely re-done AdornmentEditor. I'm going to rename it to ViewEditor as well. I also want to integrate ViewEditor into AllViewsTester because I use that a lot to audit view behaviors and being able to tweak settings live is super useful.

I also added the abilty to just click on any view in the Scenario and ViewToEdit will be set to that view.

[tig]

I just submitted another PR with quesitons / comments inline.

[tig]

IMO, View sub-classes should not have the word "View" in them as a general principle. `

ScrollBarView should be ScrollBar.

[tig]

I'm trying to understand this.

In v1 there was a similarly named construct that was super confusing to me.

In the ContentScrolling work I did, I thought I addressed the user scenario that drove this with

/// <summary>
///     If set, <see cref="View.Viewport"/><c>.X</c> can be set values greater than <see cref="View.GetContentSize ()"/>
///     <c>.Width</c> enabling scrolling beyond the right
///     of the content area.
/// </summary>
/// <remarks>
///     <para>
///         When not set, <see cref="View.Viewport"/><c>.X</c> is constrained to <see cref="View.GetContentSize ()"/>
///         <c>.Width - 1</c>.
///         This means the last column of the content will remain visible even if there is an attempt to scroll the
///         Viewport past the last column.
///     </para>
///     <para>
///         The practical effect of this is that the last column of the content will always be visible.
///     </para>
/// </remarks>
AllowXGreaterThanContentWidth = 4,

/// <summary>
///     If set, <see cref="View.Viewport"/><c>.Y</c> can be set values greater than <see cref="View.GetContentSize ()"/>
///     <c>.Height</c> enabling scrolling beyond the right
///     of the content area.
/// </summary>
/// <remarks>
///     <para>
///         When not set, <see cref="View.Viewport"/><c>.Y</c> is constrained to <see cref="View.GetContentSize ()"/>
///         <c>.Height - 1</c>.
///         This means the last row of the content will remain visible even if there is an attempt to scroll the Viewport
///         past the last row.
///     </para>
///     <para>
///         The practical effect of this is that the last row of the content will always be visible.
///     </para>
/// </remarks>
AllowYGreaterThanContentHeight = 8,

Specifically:

The practical effect of this is that the last column of the content will always be visible.

Can you verify we are talking about the same end-user scenario:

"As a user, when using a ScrollBar to scroll through content, the last row/column of the content never scrolls beyond the visible area."

[BDisp]

Specifically:

The practical effect of this is that the last column of the content will always be visible.

Correct.

Can you verify we are talking about the same end-user scenario:

I think so.

"As a user, when using a ScrollBar to scroll through content, the last row/column of the content never scrolls beyond the visible area."

Correct. But imagine if a user want to use a scroll bar without a View. But I may not being dealing with this right. What do you propose?

[tig]

Correct.

But imagine if a user want to use a scroll bar without a View. But I may not being dealing with this right. What do you propose?

If someone wants to use a scroll bar as a standalone View to control another View, and they want to ensure some content never gets scrolled out of visibility, they can code that up as needed. There's no need for ScrollBar to have that functionality.

I propose we don't have a KeepContentInViewport-like property. AllowYGreaterThanContentHeight should work for most mainline scenarios.

[BDisp]

@tig this is how I pretend to implement the scroll bars into the View.

[BDisp]

@tig this is how I pretend to implement the scroll bars into the View.

Damn, I committed without run the unit tests and now I see that instantiate the scroll bars on the initialization of the View will increase the number of view not yet disposed. Do you know the best way to workaround this?

[tig]

@tig this is how I pretend to implement the scroll bars into the View.

Damn, I committed without run the unit tests and now I see that instantiate the scroll bars on the initialization of the View will increase the number of view not yet disposed. Do you know the best way to workaround this?

Haha! I was literally working on same thing this morning. I was up late last night thinking about this ;-).

See how I used Lazy<T> in my PR....

[BDisp]

I think it's working much better now. When you have time please test it. Thanks.

[tig]

I would like to maintaining the ShowScrollIndicator property because it will indicates that the visibility is false due the AutoHideScrollBar is true (visible if size is greater than the content size or invisible otherwise) and thus this check get => _showScrollIndicator && Visible; is crucial to to get the real state if the scroll visibility. If you don't agree please let me know.

I think ShowScrollIndicator is misnamed. "Indicator" should be "Bar", and "Show" is a verb, not a noun, so it's very confusing.

I do think you're correct that we need something more than just using Visible as I did in my prototype.

Here's a suggestion:

Add to ViewportSettings:

public enum ViewportSettings
{
...
    /// <summary>
    ///     If set, the vertical scroll bar (see <see cref="View.HorizontalScrollBar"/>) will be enabled and automatically made visible
    ///     when the dimension of the <see cref="View.Viewport"/> is smaller than the dimension of <see cref="View.GetContentSize()"/>.
    /// </summary>
    EnableHorizontalScrollBar = 64,

    /// <summary>
    ///     If set, the vertical scroll bar (see <see cref="View.VerticalScrollBar"/>) will be enabled and automatically made visible
    ///     when the dimension of the <see cref="View.Viewport"/> is smaller than the dimension of <see cref="View.GetContentSize()"/>.
    /// </summary>
    EnableVerticalScrollBar = 128,

    /// <summary>
    ///     If set, the horizontal and vertical scroll bars (see cref="View.HorizontalScrollBar"/> and <see cref="View.VerticalScrollBar"/>)
    ///     will be enabled and automatically made visible when the dimension of the <see cref="View.Viewport"/> is smaller than the
    ///     dimension of <see cref="View.GetContentSize()"/>.
    /// </summary>
    EnableScrollBars = EnableHorizontalScrollBar | EnableVerticalScrollBar

This moves the automatic behavior out of ScrollBar and into View and encapsulates that logic in View.ScrollBars.cs and consolodates content scrolling functionality.

If these flags are NOT set, then the developer can use view.Horizontal/VerticalScrollbar = true/false to manage visibility themselves.

What do you think of this idea?

[tig]

Can I just say how much I love the work @BDisp is doing in this PR? Love. Love. Love.

[tig]

@BDisp see my comment here:

• Merge Responder into View #464

When you nuke the old ScrollView etc..., can you please take care of IsOverriden too?