Fixes #2489. Build scrolling into View.

[BDisp]

Fixes

• Fixes #2489

Proposed Changes/Todos

• [x] Added ViewScrollBar.cs as partial View
• [x] Implemented handling for layout and mouse with adornments.
• [x] Implementing the built-in scroll bar on the views that already was using scroll bar.
• [x] 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:

• [x] I've named my PR in the form of "Fixes #issue. Terse description."
• [x] 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.
• [x] My code follows the Terminal.Gui library design guidelines
• [x] I ran dotnet test before commit
• [x] I have made corresponding changes to the API documentation (using /// style comments)
• [x] My changes generate no new warnings
• [x] I have checked my code and corrected any poor grammar or misspellings
• [x] I conducted basic QA to assure all features are working

[tig]

Oooh! I'm excited to dive into this.

[BDisp]

I broken the scroll bar on the ComboBox with the commit https://github.com/gui-cs/Terminal.Gui/pull/3254/commits/5e3be5fe5cd2df05dea91ccbb9b552d4cdb18021.

The commit https://github.com/gui-cs/Terminal.Gui/pull/3254/commits/53b3e81427da9e1255151fd93308fd79af5766d9 fix it.

[BDisp]

I prefer auto-hide as true by default because if a scroll bar isn't needed then don't show, unless if we really want showing them always. See this effect:

https://github.com/gui-cs/Terminal.Gui/assets/13117724/51f9d0fd-5b29-4624-8e89-66a4191d306d

[BDisp]

Please, when using AssertDriverContentsWithFrameAre on unit tests use the @"", otherwise R# will break it.

[dodexahedron]

Please, when using AssertDriverContentsWithFrameAre on unit tests use the @"", otherwise R# will break it.

I think I see what might have happened for you. It looks like trailing whitespace in some lines of those strings was erroneously removed, but only when it's a raw string literal inside a method call - not when it's assigned to a variable or constant.

Looks like a bug, to me. I'll file an issue report with JetBrains about that. I ran into the same thing on certain tests, which is why some were already left as verbatim strings instead of raw strings.

You can also disable the formatter for a section of code by using these comments around such sections:

// @formatter:off
...
// @formatter:on

Since it doesn't nuke the spaces if the raw string is assigned to a variable or constant, I suspect the issue is that it is incorrectly applying the rules of method call parameters, which enforce max of 1 consecutive space.

I'll get a hold of them on Monday and see if there's already a known way to make that behave how we want or if it's a bug.

[BDisp]

Advice to never use this code if you are separately assigning the values and checking equality for both properties because only one is changed at a time.

            ContentOffset = ContentOffset with { X = -LeftColumn };
            ContentOffset = ContentOffset with { Y = -TopRow };

Instead assign the value once and the equality check is always right.

            ContentOffset = new Point (-LeftColumn, -TopRow);

I don't trust very much in the keyword with in these situations.

[dodexahedron]

Advice to never use this code if you are separately assigning the values and checking equality for both properties because only one is changed at a time.

            ContentOffset = ContentOffset with { X = -LeftColumn };
            ContentOffset = ContentOffset with { Y = -TopRow };

Instead assign the value once and the equality check is always right.

            ContentOffset = new Point (-LeftColumn, -TopRow);

I don't trust very much in the keyword with in these situations.

That's not proper usage of with anyway.

If you are changing all parts of a value, then use the constructor (the analyzer should even give you that tip).

Otherwise, if you're not assigning everything, you do it all at once, as if you were using an initializer. Doing it in a single assignment is the important part, whether you use with or new. with is non-destructive mutation, which is a shortcut for copying the instance and assigning the specified values in an initializer. Exact constructors, when they exist, are always preferred when you'll be setting everything.

For the above example, this would be the correct with expression:

ContentOffset = ContentOffset with { X = -LeftColumn, Y = -TopRow };

But, again, since both values are being changed, the constructor is preferred anyway.

[dodexahedron]

An important thing about with, however, is that it is not only legal on structs, but also on record classes.

Regardless of whether it is used on a reference or value type, however, it will always result in a new instance. It is a by-value shallow copy operation, whenever it is used on any type, value or reference. Therefore, you will have a new object with all members copied by value. So value type members will be actual copies of the values, and reference type members will be copies of the references, meaning those will still point to the same objects as the original instance.

No explicit constructors declared on the type are executed to create the copies, with one exception.

The way it is done, under the hood, is that a copy constructor is generated at compile time, and that is called to create the new instance. For structs, this cannot be overridden. For records, it can be overridden by providing your own copy constructor (a constructor taking only an instance of the same type), if you want to customize how the copy is made (such as providing a deep copy, for example).

[dodexahedron]

One other thing that is a perk to using with when appropriate:

If the value type you are mutating is not mutable, it prevents ephemeral changes to values from not actually changing the value on the struct, as shown here:

SomeImmutableStruct x = new {SomeImmutableProperty = 0};
x.SomeImmutableProperty = 1;
Console.WriteLine(x.SomeImmutableProperty); // Prints 0

Line 2 creates a new SomeImmutableStruct and then just forgets about it, and x goes unchanged.

Re-assigning x is necessary, whether using a constructor or with. Which one you do is only relevant if the behavior of the constructor is important. Otherwise, it's just a matter of style. If it's got a bunch of properties and you only need to change one of them, with is much more concise and has very clear intent.

[dodexahedron]

Here's the documentation for the with operator if you are curious.

It's a pretty short one because it really is as simple as what I said here already. :)

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/with-expression

[dodexahedron]

Oh. It also works on anonymous types, apparently (just learned that from the doc). Though I don't think we use those anywhere anyway outside of Tuples. Interesting anyway. 🤷‍♂️

[dodexahedron]

One thing I also like to use it for, when the situation arises, is to make a quick copy of something with the same values. You can do that by providing empty brackets, like x with {}. Gives you a new instance with all values the same. Even on value types, which would copy implicitly, I kinda like doing that anyway because it makes the intent explicit. Analyzers may point it out as redundant (which it is in that situation), so auto-cleanup might remove the empty with statement.

[BDisp]

Assert.Equal ($"Combine(View(Width,FrameView(){f1.Frame})-Absolute(2))", v1.Width.ToString ());

@dodexahedron this kind of unit test is dangerous because it not guarantee that the test is really right. In this case it's a Dim test and it only print the Frame that both really must have the same value, but if it's using to compare some calculations than can diverge from the Frame value, it'll never fail, even they aren't right. But of course that you already know that :-)

[BDisp]

After doing a cleanup code R# re-adds type name on object initializers. What I need to do?

Before:

        _contentOffset = new (-Math.Abs (offset.X), -Math.Abs (offset.Y));
        _contentView.Frame = new (_contentOffset, _contentSize);

After:

        _contentOffset = new Point (-Math.Abs (offset.X), -Math.Abs (offset.Y));
        _contentView.Frame = new Rectangle (_contentOffset, _contentSize);

[dodexahedron]

Please, when using AssertDriverContentsWithFrameAre on unit tests use the @"", otherwise R# will break it.

I think I see what might have happened for you. It looks like trailing whitespace in some lines of those strings was erroneously removed, but only when it's a raw string literal inside a method call - not when it's assigned to a variable or constant.

Looks like a bug, to me. I'll file an issue report with JetBrains about that. I ran into the same thing on certain tests, which is why some were already left as verbatim strings instead of raw strings.

You can also disable the formatter for a section of code by using these comments around such sections:

// @formatter:off
...
// @formatter:on

Since it doesn't nuke the spaces if the raw string is assigned to a variable or constant, I suspect the issue is that it is incorrectly applying the rules of method call parameters, which enforce max of 1 consecutive space.

I'll get a hold of them on Monday and see if there's already a known way to make that behave how we want or if it's a bug.

Following up on this.

They are already aware of the bug and have it marked critical. So it'll likely be getting fixed soon.

[BDisp]

After doing a cleanup code R# re-adds type name on object initializers. What I need to do?

Before:

        _contentOffset = new (-Math.Abs (offset.X), -Math.Abs (offset.Y));
        _contentView.Frame = new (_contentOffset, _contentSize);

After:

        _contentOffset = new Point (-Math.Abs (offset.X), -Math.Abs (offset.Y));
        _contentView.Frame = new Rectangle (_contentOffset, _contentSize);

@dodexahedron can you add some configuration so R# doesn't change this please.

[dodexahedron]

After doing a cleanup code R# re-adds type name on object initializers. What I need to do? Before:

        _contentOffset = new (-Math.Abs (offset.X), -Math.Abs (offset.Y));
        _contentView.Frame = new (_contentOffset, _contentSize);

After:

        _contentOffset = new Point (-Math.Abs (offset.X), -Math.Abs (offset.Y));
        _contentView.Frame = new Rectangle (_contentOffset, _contentSize);

@dodexahedron can you add some configuration so R# doesn't change this please.

Did the change in #3279 take care of this for you, @BDisp?

The settings were changed to always prefer target-typed new whenever possible, and changed the warning behavior to only warn when it ISN'T being done that way (so opposite of what it was doing to you).

[BDisp]

This test sometimes fails.

[xUnit.net 00:01:02.49]     Terminal.Gui.InputTests.EscSeqUtilsTests.DecodeEscSeq_Tests [FAIL]
[xUnit.net 00:01:02.49]       Assert.Null() Failure: Value is not null
[xUnit.net 00:01:02.49]       Expected: null
[xUnit.net 00:01:02.49]       Actual:   View(){X=0,Y=0,Width=80,Height=25}
[xUnit.net 00:01:02.49]       Stack Trace:
[xUnit.net 00:01:02.49]         D:\a\Terminal.Gui\Terminal.Gui\UnitTests\Input\EscSeqUtilsTests.cs(774,0): at Terminal.Gui.InputTests.EscSeqUtilsTests.DecodeEscSeq_Tests()
[xUnit.net 00:01:02.49]            at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
[xUnit.net 00:01:02.49]            at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
[xUnit.net 00:01:02.50]   Finished:    UnitTests
[xUnit.net 00:01:02.50] Canceling due to test failure...
  Failed Terminal.Gui.InputTests.EscSeqUtilsTests.DecodeEscSeq_Tests [9 s]
  Error Message:
   Assert.Null() Failure: Value is not null
Expected: null
Actual:   View(){X=0,Y=0,Width=80,Height=25}
  Stack Trace:
     at Terminal.Gui.InputTests.EscSeqUtilsTests.DecodeEscSeq_Tests() in D:\a\Terminal.Gui\Terminal.Gui\UnitTests\Input\EscSeqUtilsTests.cs:line 774
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Test Run Failed.
Total tests: 5857
     Passed: 5856
     Failed: 1
 Total time: 1.1381 Minutes
       _VSTestConsole:
         MSB4181: The "VSTestTask" task returned false but did not log an error.
     3>Done Building Project "D:\a\Terminal.Gui\Terminal.Gui\UnitTests\UnitTests.csproj" (VSTest target(s)) -- FAILED.
     1>Done Building Project "D:\a\Terminal.Gui\Terminal.Gui\Terminal.sln" (VSTest target(s)) -- FAILED.

[BDisp]

@BDisp My recommendation re: #3254 is:

• [ ] Start with a fresh branch based on this branch (effectively abandoning Fixes #2489. Build scrolling into View. #3254).

I already did much changes here and I have no discernment to start again.

• git checkout tig-v2_3273_FOUND-finddeepestview git checkout -b v2_3295-Fixes-Add-Scrolling-To-View
• [ ] Create a new "Scroll in View" Scenario based on Adornments.cs that can be used to experiment and test.

I think the best working scenario for this is recovering the scroll bar on the views that used them before and was broken, like the TextView, ListView, TableView, TreeView, etc... and I implemented on all of them.

• [ ] Create a new view ScrollBar. Make it just render something distinct; do not try to move old ScrollBarView code over yet.
• [ ] Add public ScrollBar HoriztontalScrollBar and public ScrollBar VerticalScrollBar to View

In my opinion the best way to manage the scroll bars is by not exposing them directly to the users and using a flag to enable them. Imagine a user can do on the HorizontalScrollBar = new ScrollBarView { Oreintation = Orientation.Vertical } and thus we will have two vertical scroll bars.

• [ ] Change Padding.BeginInit to Add(Parent.HoriztonalScrollBar, Parent.VerticalScrollBar)

I think we only should add to the superview if the scroll bars are enable instead of they always was added without needed.

• [ ] Change Padding.LayoutStarted to position the scrollbars in Padding

I swearer that I did try only using the superview LayoutStarted event but sometimes the necessary information wasn't get before the scroll bar are drawn.

• [ ] Start testing/playing with ScrollBar.OnMouseClick to ensure it's working properly.

Until now they are working correctly.

THEN, start moving ScrollBarView code over to ScrollBar.

This will ensure you are utilizing all the Adornment stuff I've finally gotten working (I think) correctly in this PR. This will also help find places where I got it wrong.

All the changes you have made to the Adornment and the ViewLayout are now working correctly and confirmed.

As you find issues, submit them as PRs to this branch.

My intention is only at least help you find the best solution for you implement your own code for this.

[BDisp]

The Character Map scenario now always show the last item with or without horizontal scroll bar.

[BDisp]

I hope you understand this explanation. Please say if you don't agree.

    // BUGBUG: v2 - Why can't we get rid of this and just use Visible?
    // We need this property to distinguish from Visible value and the original value of this property
    /// <summary>
    /// Gets or sets the visibility for the vertical or horizontal scroll indicator.
    /// If <see cref="AutoHideScrollBars"/> is true the visibility will be handled as needed,
    /// no matter ShowScrollIndicator is true or false. It's a flag that indicates the original state.
    /// If <see cref="AutoHideScrollBars"/> is false and ShowScrollIndicator is false it'll
    /// be not visible, otherwise it will be always visible even it isn't needed.
    /// </summary>
    /// <value><c>true</c> if show vertical or horizontal scroll indicator; otherwise, <c>false</c>.</value>
    public bool ShowScrollIndicator

[tig]

I'm confused as to why you are actively working on this PR. I thought you were going to re-do this based on what I come up with in #3323?

[BDisp]

I'm confused as to why you are actively working on this PR. I thought you were going to re-do this based on what I come up with in #3323?

It's only to proof to my self that this is working and also for you leverage some of the code you think useful. At least the views where I implemented the scroll bars. Do you think this is really badly? I don't pretend that you merge anything of this but at least to check if you are get more or less he same result and even better than this. For me the final result are the views where the scroll bars worked well before continue to work after the #3323.

[tig]

Gotcha.

I do not intend to touch ScrollBarView or ScrollView. I don't expect to break them either.

I will build a new Scenario that proves the new "virtual content area built into View" will work well with keyboard and mouse wheel input (no scroll bars). And I plan on removing the scroll bars from charmap as another example for now.

[BDisp]

@dodexahedron I think there is some bug with the with keyword. See bellow where the mouse is over the Y property expecting a int type from the Point ContentOffset and it show {Center} which is the View.Pos Y property. But it really only assign the value to the Point.Y.

[BDisp]

Mouse support for scrolling including wheel.

With scrollbars:

https://github.com/gui-cs/Terminal.Gui/assets/13117724/f21d8fda-af58-4d65-9c84-04c5c180dae4

Without scrollbars:

https://github.com/gui-cs/Terminal.Gui/assets/13117724/10b91f94-4f98-404e-800c-2a01a6bbd7da

[BDisp]

Gotcha.

I do not intend to touch ScrollBarView or ScrollView. I don't expect to break them either.

I will build a new Scenario that proves the new "virtual content area built into View" will work well with keyboard and mouse wheel input (no scroll bars). And I plan on removing the scroll bars from charmap as another example for now.

Do you mean like this https://github.com/gui-cs/Terminal.Gui/pull/3254#issuecomment-2012209251?

[tig]

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

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.

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

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.

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.

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:

• 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.
• 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.
  • Changing Viewport.Location changes its location within the content (e.g. the start of the content the user sees).
  • 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.
• There's no need for devs to worry about ScrollBars if they don't want them.

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.

[BDisp]

You are right. I'm closing this because it's buggy.

[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.

https://github.com/gui-cs/Terminal.Gui/assets/13117724/5a0a9c30-981a-42fb-a394-80319eb4df7f