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

[BDisp]

Fixes

• Fixes #2489

Proposed Changes/Todos

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

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

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

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

[BDisp]

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

ScrollBarView should be ScrollBar.

I agree, but ScrollBarView is still the old one. When the new one is created it will just be called ScrollBar.

[BDisp]

I have to find out why if I move the scroll with the mouse one line the position changes from 0 to 9. However, returning the position to 0 with the mouse and then clicking on the Position button requires the position to reach 10 for the scroll move 1 line. It's strange, I think it should be the same value in both cases, 9 or 10, don't you think?

[dodexahedron]

Also, line 11 of Scroll.cs is dangerous and we either need to seal the class or not access that property in that way in that spot.

A derived class that has overridden that member will be set by that line, and the consumer can't tell it not to without completely hiding the member, which has a completely different set of consequences.

If the implementation depends on that behavior, then the class needs to be sealed.

If the implementation will be ok as long as a derived class doesn't hide the member (which is not our problem), then it should be handled by setting the backing field - not from a virtual member accessor.

That's the reason for the warning that's being shown by the analyzer for that line as well as line 23 (the call to Add).

[dodexahedron]

There's also an important implication of using the Size struct the way it is in View, as a Nullable<Size>, but that's for addressing elsewhere, as it's not directly related to this specific PR.

@dodexahedron (tagging so it shows up via an easy search in my queue for later performance pass consideration)

[dodexahedron]

Not the fault of this PR, but as I said I would do when we got further along, a while back, I'm now giving a closer look to things like I started talking about in my review.

That stuff, when traced through just a little, ultimately reveals opportunities in the position and size code to avoid a non-trivial amount of heft from things like copies (both implicit and explicit) and possible boxing, as well as to at least somewhat mitigate potential concurrency problems. Most of that would be via ref returns and such, for internal code especially, but potentially also could be exposed to the consumer, as well.

I think it's best for that to be done separately from here, though, because of scope.

[BDisp]

I added highlight effect in the slider. I didn't used the HighlightStyle enum because the hover isn't working.

https://github.com/gui-cs/Terminal.Gui/assets/13117724/17107090-f122-45f1-9bc9-8b5a6c5f088b

PS: I need a ColorSchemeChanged event to intercept the change before DrawContent.

[BDisp]

PS: I need a ColorSchemeChanged event to intercept the change before DrawContent.

Not needed I just override the GetNormalColor. Thanks @tig.

[tig]

This is looking really nice!!!

Have you noticed that if you click and drag outside of the scroll bar's superview, mouse tracking gets lost? See this vid for what I mean.

This doesn't feel right to me.

Slider does this correctly:

[BDisp]

This is looking really nice!!!

Thanks.

Have you noticed that if you click and drag outside of the scroll bar's superview, mouse tracking gets lost? See this vid for what I mean.

Yes I did.

[ ![U6BF1ge 1](https://private-user-images.githubusercontent.com/585482/342404879-b3f73af7-2604-4643-ab7b-bb445d6ab9e8.gif?> This doesn't feel right to me.

I think this is correct. The scroll does not scroll because the X axis of the outer frame is smaller than the X axis of the scroll frame. That's why you don't see it move to the right.

Slider does this correctly:

The slider you see moves vertically because you are dealing with the Y axis and not the X axis.

[ ![yyjZMyf 1](https://private-user-images.githubusercontent.com/585482/342404371-a596dac4-2e9d-4225-80f6-48ef65ec4055.gif?

[tig]

I think this is correct. The scroll does not scroll because the X axis of the outer frame is smaller than the X axis of the scroll frame. That's why you don't see it move to the right.

I'm not explaining myself well.

If you move the mouse really fast outside of the view that hosts the scroll bar, mouse tracking within the scroll bar stops.

[BDisp]

I'm not explaining myself well.

If you move the mouse really fast outside of the view that hosts the scroll bar, mouse tracking within the scroll bar stops.

Ah maybe because the MouseGrabView become null and loses the mouse tracking? I'll see that later. I cannot test that now. Can you please test the same with the vertical scroll orientation? Thanks.

[dodexahedron]

I think this is correct. The scroll does not scroll because the X axis of the outer frame is smaller than the X axis of the scroll frame. That's why you don't see it move to the right.

I'm not explaining myself well.

If you move the mouse really fast outside of the view that hosts the scroll bar, mouse tracking within the scroll bar stops.

Good catch.

Man, it would be awesome if we had some automated UI testing to add this sort of thing to. Sucks only having 24 hours in a day, huh? 😅

[BDisp]

I'm not explaining myself well.

If you move the mouse really fast outside of the view that hosts the scroll bar, mouse tracking within the scroll bar stops.

@tig this is already fixed in this commit https://github.com/gui-cs/Terminal.Gui/pull/3498/commits/9b896573ece09018ffe7502157209395b046ed7d.

[BDisp]

I'm not explaining myself well. If you move the mouse really fast outside of the view that hosts the scroll bar, mouse tracking within the scroll bar stops.

@tig this is already fixed in this commit 9b89657.

The only issue is when the mouse go outside of the top stopping responding. This is only happens with WT. With conhost works well. I already opened an issue in https://github.com/microsoft/terminal/issues/17744.

[BDisp]

@tig when you have time can you please starting review this before I continue further. Thanks.

[BDisp]

@tig do you remember any change that may cause unit test failing on macOS? It's also failing in the v2_develop branch.

[xUnit.net 00:01:26.21]     UICatalog.Tests.ScenarioTests.All_Scenarios_Quit_And_Init_Shutdown_Properly(scenarioType: typeof(UICatalog.Scenarios.DatePickers)) [FAIL]
[xUnit.net 00:01:26.21]       'Date Picker' failed to Quit with Esc after 1500ms and 0 iterations. Force quit.
[xUnit.net 00:01:26.21]       Stack Trace:
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/UnitTests/UICatalog/ScenarioTests.cs(109,0): at UICatalog.Tests.ScenarioTests.<>c__DisplayClass3_0.<All_Scenarios_Quit_And_Init_Shutdown_Properly>g__ForceCloseCallback|1()
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Application/MainLoop.cs(379,0): at Terminal.Gui.MainLoop.RunTimers()
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Application/MainLoop.cs(269,0): at Terminal.Gui.MainLoop.RunIteration()
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Application/Application.Run.cs(587,0): at Terminal.Gui.Application.RunIteration(RunState& state, Boolean& firstIteration)
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Application/Application.Run.cs(561,0): at Terminal.Gui.Application.RunLoop(RunState state)
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Application/Application.Run.cs(439,0): at Terminal.Gui.Application.Run(Toplevel view, Func`2 errorHandler)
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/UICatalog/Scenarios/DatePickers.cs(23,0): at UICatalog.Scenarios.DatePickers.Main()
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/UnitTests/UICatalog/ScenarioTests.cs(48,0): at UICatalog.Tests.ScenarioTests.All_Scenarios_Quit_And_Init_Shutdown_Properly(Type scenarioType)
[xUnit.net 00:01:26.21]            at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
[xUnit.net 00:01:26.21]            at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
[xUnit.net 00:01:26.21]       Output:
[xUnit.net 00:01:26.21]         Running Scenario 'UICatalog.Scenarios.DatePickers'
[xUnit.net 00:01:26.21]         Initialized 'Terminal.Gui.CursesDriver'
[xUnit.net 00:01:26.22]   Finished:    UnitTests
[xUnit.net 00:01:26.22] Canceling due to test failure..

[tig]

@tig do you remember any change that may cause unit test failing on macOS? It's also failing in the v2_develop branch.

[xUnit.net 00:01:26.21]     UICatalog.Tests.ScenarioTests.All_Scenarios_Quit_And_Init_Shutdown_Properly(scenarioType: typeof(UICatalog.Scenarios.DatePickers)) [FAIL]
[xUnit.net 00:01:26.21]       'Date Picker' failed to Quit with Esc after 1500ms and 0 iterations. Force quit.
[xUnit.net 00:01:26.21]       Stack Trace:
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/UnitTests/UICatalog/ScenarioTests.cs(109,0): at UICatalog.Tests.ScenarioTests.<>c__DisplayClass3_0.<All_Scenarios_Quit_And_Init_Shutdown_Properly>g__ForceCloseCallback|1()
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Application/MainLoop.cs(379,0): at Terminal.Gui.MainLoop.RunTimers()
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Application/MainLoop.cs(269,0): at Terminal.Gui.MainLoop.RunIteration()
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Application/Application.Run.cs(587,0): at Terminal.Gui.Application.RunIteration(RunState& state, Boolean& firstIteration)
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Application/Application.Run.cs(561,0): at Terminal.Gui.Application.RunLoop(RunState state)
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/Terminal.Gui/Application/Application.Run.cs(439,0): at Terminal.Gui.Application.Run(Toplevel view, Func`2 errorHandler)
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/UICatalog/Scenarios/DatePickers.cs(23,0): at UICatalog.Scenarios.DatePickers.Main()
[xUnit.net 00:01:26.21]         /Users/runner/work/Terminal.Gui/Terminal.Gui/UnitTests/UICatalog/ScenarioTests.cs(48,0): at UICatalog.Tests.ScenarioTests.All_Scenarios_Quit_And_Init_Shutdown_Properly(Type scenarioType)
[xUnit.net 00:01:26.21]            at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
[xUnit.net 00:01:26.21]            at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
[xUnit.net 00:01:26.21]       Output:
[xUnit.net 00:01:26.21]         Running Scenario 'UICatalog.Scenarios.DatePickers'
[xUnit.net 00:01:26.21]         Initialized 'Terminal.Gui.CursesDriver'
[xUnit.net 00:01:26.22]   Finished:    UnitTests
[xUnit.net 00:01:26.22] Canceling due to test failure..

Seems ok now?

[BDisp]

Seems ok now?

Yes, did you do something to fix it?

[tig]

Seems ok now?

Yes, did you do something to fix it?

I don't think so