Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 38 commits into
base: v2_develop
Choose a base branch
from

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented May 23, 2024

Fixes

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 BDisp requested a review from tig as a code owner May 23, 2024 21:46
@BDisp BDisp marked this pull request as draft May 23, 2024 21:46
@BDisp
Copy link
Collaborator Author

BDisp commented May 23, 2024

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

@tig
Copy link
Collaborator

tig commented May 23, 2024

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

@BDisp
Copy link
Collaborator Author

BDisp commented May 24, 2024

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

WindowsTerminal_or6sJpC3c5

@BDisp
Copy link
Collaborator Author

BDisp commented May 24, 2024

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

Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really slick!!

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

Terminal.Gui/Views/Scroll.cs Outdated Show resolved Hide resolved
Terminal.Gui/Views/Scroll.cs Outdated Show resolved Hide resolved
Terminal.Gui/Views/Scroll.cs Outdated Show resolved Hide resolved
@tig
Copy link
Collaborator

tig commented May 24, 2024

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

image

@BDisp
Copy link
Collaborator Author

BDisp commented May 24, 2024

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
Copy link
Collaborator Author

BDisp commented May 25, 2024

@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
Copy link
Collaborator

tig commented May 25, 2024

@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
Copy link
Collaborator

tig commented May 25, 2024

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.

Terminal.Gui/Views/Scroll.cs Outdated Show resolved Hide resolved
Terminal.Gui/Views/Scroll.cs Outdated Show resolved Hide resolved
@tig
Copy link
Collaborator

tig commented May 25, 2024

I just submitted another PR with quesitons / comments inline.

@tig
Copy link
Collaborator

tig commented May 25, 2024

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

ScrollBarView should be ScrollBar.

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 22, 2024

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.

Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Terminal.Gui/Views/ScrollBarView.cs Outdated Show resolved Hide resolved
UICatalog/KeyBindingsDialog.cs Outdated Show resolved Hide resolved
Terminal.Gui/Views/Scroll.cs Outdated Show resolved Hide resolved
@BDisp
Copy link
Collaborator Author

BDisp commented Jun 23, 2024

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

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

@tig
Copy link
Collaborator

tig commented Jun 24, 2024

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.

U6BF1ge 1

This doesn't feel right to me.

Slider does this correctly:

yyjZMyf 1

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 24, 2024

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

@tig
Copy link
Collaborator

tig commented Jun 24, 2024

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
Copy link
Collaborator Author

BDisp commented Jun 24, 2024

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
Copy link
Collaborator

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

Comment on lines +21 to +22
Width = Dim.Auto (DimAutoStyle.Content, 1);
Height = Dim.Auto (DimAutoStyle.Content, 1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tig if a derived view want to constraint the Width and the Height to avoid the user set inappropriate values, how that can be done, if they aren't virtual?

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 16, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create new ScrollBar based on a new Scroll and remove ScrollBarView/ScrollView
3 participants