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

• Scroll class
• ScrolBar class

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.

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

Important question in-line.

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

WindowsTerminal_rJyd5R3RZY.mp4

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

[dodexahedron]

Took a look at it. Doesn't quite do what you were hoping, but fix is simple. :)

[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? 😅