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 #3169. Bounds -> Viewport: Content Scrolling in View #3323

Merged
merged 107 commits into from
Apr 16, 2024

Conversation

tig
Copy link
Collaborator

@tig tig commented Mar 14, 2024

This is a big, far-reaching PR that addresses some fundamental issues in v1 that I originally laid out here:

The Conceptual Documentation for Layout and Drawing has been improved to reflect the new design. Start there.

Things I'm proud of:

  • The new "Viewport" model is intuitive and easy to explain. You'll note the updated API docs are consistent and use plain-English that relate to the actual API terms like "Viewport", "ContentSize", etc...
  • My vision for Adornment is coming to fruition. It took a bunch of work but Adornments can now have subviews and it all works (and has unit tests to back it up). Note that in the new "Virtual Content" Scenario the settings UI for Viewports is actually a set of subviews w/in Padding.
  • Porting v1 code is easy. For example in this PR it took me about 20 minutes to get ListView using Viewport.Y instead of the old ListView._top. It took another 20 min to remove un-needed code (e.g. ListView.ScrollUp/Down/Left/Right were replaced with ListView.ScrollHoriztonal/ScrollVertical.
  • Things are more testable. The previous scroll functionality in ScrollView depended on unit tests that compared Draw output. Now the core functionality is tested with real unit tests.

Things I'm not proud of:

  • I feel like I frustrated @bidisp as he was trying to solve similar issues by trying to massage the old ScrollView code to work well with subviews of Adornments etc...
  • We're still not done with Adornments. Specifically Focus-related things do not work.
  • I'm worried I effed up how I rebased v2_develop into this and it's going to make other's have merge hell after this gets merged.

Fixes

Proposed Changes/Todos

  • Try different Bounds names. E.g. Viewport
  • Fix existing Draw/Layout code that isn't quite correct w.r.t. honoring Viewport (e.g. View.Move)
  • Enable virtual content area (Viewport.Location can be non-zero).
  • Add a new Scenario and unit tests to ensure complex scenarios with Adornments etc... work.
  • Revamp Charmap to use built-in capabilty; add a temporary 'fake' scrollbar impl based on the updated Button that supports continuous button presses.
  • Implement the equivalent of KeepContentInViewport -> ViewportSettings.AllowXGreaterThanContentWidth
  • Refactor ListView to use Viewport.Location and ContentSize instead of _top and _left

Changes that can be done after this is merged to finish the new system

  • Replace ScrollBarView with a new view, ScrollBar that is a composotion of 2 Button objects and another new view called Scroll
  • Implement the equivalent of AutoShowScrollbars
  • Update other built-in Views that support scrollling to take advantage of the new stuff

@BDisp
Copy link
Collaborator

BDisp commented Mar 14, 2024

Why Viewport and not ContentArea?

@dodexahedron
Copy link
Collaborator

I think both are perfectly valid.

ViewPort and ContentArea are both terms used in various UI frameworks.

For what it's worth, however, WPF uses the term Bounds for this property on controls.

@BDisp
Copy link
Collaborator

BDisp commented Mar 14, 2024

I also think the Bounds is also a perfect for this property. But I don't why we discuss what the name is to use and suddenly the others opinions doesn't matter.

@tig
Copy link
Collaborator Author

tig commented Mar 15, 2024

I also think the Bounds is also a perfect for this property. But I don't why we discuss what the name is to use and suddenly the others opinions doesn't matter.

I meant to mark this PR as draft.

I'm experimenting.

I have said for a long time that

  • Bounds is a bad name. It lacks any descriptiveness.
  • It is stupid that Bounds is a Rectangle and not a Size given it's current use.

I have also argued View needs 3 pieces of data to enable built-in scrolling:

  1. Something that describes the size of the visible area (a Size).
  2. Something that describes the size of the content (virtual content) (a Size)
  3. Something that describes where the top-left of the visible area is within the content (a Point).

One idea is to combine 1) and 3) into a single Rectangle. This could be named Bounds, ContentArea, or ViewPort.

Another idea is to have separate properties for 1) and 3) (a Point and a Size).

The size of the content needs to be a Size and could be named ContentSize, VirtualContentSize, etc...

In this PR, I decided to run an experiment of what it would be like to keep 1) and 3) a single Rectangle property.

I researched other UI frameworks and saw the term Viewport was used and that its definition in those frameworks seemed to fit what we're looking for perfectly.

I wanted to like ContentArea, but I struggle with because the word "Area" means SIZE, not SIZE & LOCATION (at least to me).

So, this PR is an experiment. Nothing's decided and renaming Viewport to ContentArea or something else will not be hard.

Next step for me here is to add public Size ContentSize and get scrolling working with just the keyboard.

Note, there IS another part of all this that needs to be figured out: What do we do about Clip. As you know, I am a fan of us making clipping far more sophisticated....an irregular region;

Clearly, clipping only applies to Viewport (aka Bounds). IMO, by default, when View.OnDrawContent is called, the Clip should be set to Viewport and attempts do draw outside of the Viewport should not do anything.

I have continued to ignore this because the one disadvantage of Clip is it hides poorly written drawing code and code that does things like Clear too frequently. But that will come later...

@tig tig marked this pull request as draft March 15, 2024 00:55
@BDisp
Copy link
Collaborator

BDisp commented Mar 18, 2024

I meant to mark this PR as draft.

I'm experimenting.

I have said for a long time that

  • Bounds is a bad name. It lacks any descriptiveness.
  • It is stupid that Bounds is a Rectangle and not a Size given it's current use.

System.Windows.Forms use it, see https://learn.microsoft.com/en-us/dotnet/api/system.windows.forms.control.bounds?view=windowsdesktop-8.0.

It's really a Rectangle that until now was always using the Location.Empty but in the #3254 is now possible to use negative location on the his getter property, by using the UseContentOffset and the ContentOffset properties.
Note that it's possible scrolling with or without scroll bars. See the ScrollBars and the Scrooling Without ScrollBars scenarios.

I have also argued View needs 3 pieces of data to enable built-in scrolling:

  1. Something that describes the size of the visible area (a Size).

That is what the GetVisibleContentArea method does to using by the ClipToBounds method and on others views, that use scrolling.
Note that for the TextFormatter?.Draw we need to pass the real location and size, which the location is negative or not and the size is incremented by the negative location or not. Otherwise, it won't redraw as expected considering the Alignment, VerticalAlignment, etc...

  1. Something that describes the size of the content (virtual content) (a Size)

ContentSize is the right property to use.

  1. Something that describes where the top-left of the visible area is within the content (a Point).

If you are meaning the visible area of the content ia always Point.Empty.

One idea is to combine 1) and 3) into a single Rectangle. This could be named Bounds, ContentArea, or ViewPort.

On my PR I use the GetVisibleContentArea method for that, but it can be a read-only property too, as VisibleContentArea. We need to decide if the ContentArea is all the Rectangle with negative locations and incremented size, or the visible area used for clipping the region where the contents can be re-draw.

Another idea is to have separate properties for 1) and 3) (a Point and a Size).

The size of the content needs to be a Size and could be named ContentSize, VirtualContentSize, etc...

My PR use ContentSize for Size and ContentOffset for Point. Note that the ContentSize is a fixed value until the user want to change it. What it will change the Width and Height of the Rectangle is the ContentOffset. e.g. A view has a Width=10, Height=10, ContentSize.Width=20, ContentSize.Height=20, ContentOffset.X=-4, ContentOffset.Y=-6. Considering the ContentArea as the real and not the visible region it will return ContentArea(X=-4, Y=-6, Width=16, Height=14).
Note that Adornments will never use the ContentOffset neither the ContentSize, obviously.

In this PR, I decided to run an experiment of what it would be like to keep 1) and 3) a single Rectangle property.

  1. should be a Rectangle that is combined with the ContentOffset for Location and the Frame.Size, Adornment.Thickness and the ContentOffset for the Size.

I researched other UI frameworks and saw the term Viewport was used and that its definition in those frameworks seemed to fit what we're looking for perfectly.

The name is irrelevant and must be you to decide because you are the mentor of this project.

I wanted to like ContentArea, but I struggle with because the word "Area" means SIZE, not SIZE & LOCATION (at least to me).

Any area is something with a size, no matter is rectangle, square, etc..., but with a location too. But decide by your head and whatever you decide is fine to me, only hopping you understood all my explanations so far.

So, this PR is an experiment. Nothing's decided and renaming Viewport to ContentArea or something else will not be hard.

Next step for me here is to add public Size ContentSize and get scrolling working with just the keyboard.

Leverage what I did so far because it will no diverge so much.

Note, there IS another part of all this that needs to be figured out: What do we do about Clip. As you know, I am a fan of us making clipping far more sophisticated....an irregular region;

Well that's a real challenger, may be passing an array of rectangles?

Clearly, clipping only applies to Viewport (aka Bounds). IMO, by default, when View.OnDrawContent is called, the Clip should be set to Viewport and attempts do draw outside of the Viewport should not do anything.

That's also my opinion.

I have continued to ignore this because the one disadvantage of Clip is it hides poorly written drawing code and code that does things like Clear too frequently. But that will come later...

Sometimes the use of the Clear too frequently is because the View is forcing only using the Normal or Focus colors. If a view want to use a custom color other than them, have to override the View.OnDrawContent to do that.

@tig
Copy link
Collaborator Author

tig commented Mar 18, 2024

I meant to mark this PR as draft.
I'm experimenting.
I have said for a long time that

  • Bounds is a bad name. It lacks any descriptiveness.
  • It is stupid that Bounds is a Rectangle and not a Size given it's current use.

System.Windows.Forms use it, see https://learn.microsoft.com/en-us/dotnet/api/system.windows.forms.control.bounds?view=windowsdesktop-8.0.

No, Window Forms does NOT use Bounds for the same thing. In Windows Forms Bounds is equivalent to our Frame:

Gets or sets the size and location of the control including its nonclient elements, in pixels, relative to the parent control.

This is partially why I have long not liked the name.

It's really a Rectangle that until now was always using the Location.Empty but in the #3254 is now possible to use negative location on the his getter property, by using the UseContentOffset and the ContentOffset properties. Note that it's possible scrolling with or without scroll bars. See the ScrollBars and the Scrooling Without ScrollBars scenarios.

FWIW, I started this PR because you told me you wanted me to, here:

#3295 (comment)

I'm approaching this with as fresh a perspective as I can, asking myself the most fundamental questions and trying to ignore legacy thinking. I find that when I do that, it helps my brain get clarity.

I have also argued View needs 3 pieces of data to enable built-in scrolling:

  1. Something that describes the size of the visible area (a Size).

That is what the GetVisibleContentArea method does to using by the ClipToBounds method and on others views, that use scrolling. Note that for the TextFormatter?.Draw we need to pass the real location and size, which the location is negative or not and the size is incremented by the negative location or not. Otherwise, it won't redraw as expected considering the Alignment, VerticalAlignment, etc...

I'm in the middle of thinking this through right now (by actually changing the code and API docs to see the impact). If you look at the current code in this branch you'll see this API doc paragraph:

The <see cref="Viewport"/> Location and Size indicate what part of the View's virtual content area, defined by <see cref="ContentSize"/>, is visible and should be drawn. The coordinates taken by <see cref="Move"/> and <see cref="AddRune"/> are relative to <see cref="Viewport"/>, thus if <c>ViewPort.Location.Y</c> is <c>5</c> the 5th row of the content should be drawn using <c>MoveTo (x, 5)</c>.

/// <summary>
/// Draws the view's content, including Subviews.
/// </summary>
/// <remarks>
/// <para>
/// The <paramref name="viewport"/> parameter is provided as a convenience; it has the same values as the
/// <see cref="Viewport"/> property.
/// </para>
/// <para>
/// The <see cref="Viewport"/> Location and Size indicate what part of the View's virtual content area, defined
/// by <see cref="ContentSize"/>, is visible and should be drawn. The coordinates taken by <see cref="Move"/> and
/// <see cref="AddRune"/> are relative to <see cref="Viewport"/>, thus if <c>ViewPort.Location.Y</c> is <c>5</c>
/// the 5th row of the content should be drawn using <c>MoveTo (x, 5)</c>.
/// </para>
/// <para>
/// If <see cref="ContentSize"/> is larger than <c>ViewPort.Size</c> drawing code should use <see cref="Viewport"/>
/// to constrain drawing for better performance.
/// </para>
/// <para>
/// The <see cref="Clip"/> may define smaller area than <see cref="Viewport"/>; complex drawing code can be more
/// efficient by using <see cref="Clip"/> to constrain drawing for better performance.
/// </para>
/// <para>
/// Overrides should loop through the subviews and call <see cref="Draw"/>.
/// </para>
/// </remarks>
/// <param name="viewport">
/// The rectangle describing the currently visible viewport into the <see cref="View"/>; has the same value as
/// <see cref="Viewport"/>.
/// </param>
public virtual void OnDrawContent (Rectangle viewport)

Otherwise, it won't redraw as expected considering the Alignment, VerticalAlignment, etc...

I don't understand this point. What information would be missing in how I've defined things above that would prevent alignment from working?

  1. Something that describes the size of the content (virtual content) (a Size)

ContentSize is the right property to use.

Good. That's my current thinking too.

  1. Something that describes where the top-left of the visible area is within the content (a Point).

If you are meaning the visible area of the content ia always Point.Empty.

One idea is to combine 1) and 3) into a single Rectangle. This could be named Bounds, ContentArea, or ViewPort.

On my PR I use the GetVisibleContentArea method for that, but it can be a read-only property too, as VisibleContentArea. We need to decide if the ContentArea is all the Rectangle with negative locations and incremented size, or the visible area used for clipping the region where the contents can be re-draw.

Another idea is to have separate properties for 1) and 3) (a Point and a Size).
The size of the content needs to be a Size and could be named ContentSize, VirtualContentSize, etc...

My PR use ContentSize for Size and ContentOffset for Point. Note that the ContentSize is a fixed value until the user want to change it. What it will change the Width and Height of the Rectangle is the ContentOffset. e.g. A view has a Width=10, Height=10, ContentSize.Width=20, ContentSize.Height=20, ContentOffset.X=-4, ContentOffset.Y=-6. Considering the ContentArea as the real and not the visible region it will return ContentArea(X=-4, Y=-6, Width=16, Height=14). Note that Adornments will never use the ContentOffset neither the ContentSize, obviously.

I think we're aligned here. The only difference is the "offset from the top-left (0,0) coordinate of the content to where the visiible area is" in my mind is Viewport.Location and the offset is provided in positive values. In the OnDrawContent API docs I linked to above "if ViewPort.Location.Y is 5 the 5th row of the content should be drawn using MoveTo (x, 5)."

In this PR, I decided to run an experiment of what it would be like to keep 1) and 3) a single Rectangle property.

  1. should be a Rectangle that is combined with the ContentOffset for Location and the Frame.Size, Adornment.Thickness and the ContentOffset for the Size.

I researched other UI frameworks and saw the term Viewport was used and that its definition in those frameworks seemed to fit what we're looking for perfectly.

The name is irrelevant and must be you to decide because you are the mentor of this project.

I wanted to like ContentArea, but I struggle with because the word "Area" means SIZE, not SIZE & LOCATION (at least to me).

Any area is something with a size, no matter is rectangle, square, etc..., but with a location too. But decide by your head and whatever you decide is fine to me, only hopping you understood all my explanations so far.

So, this PR is an experiment. Nothing's decided and renaming Viewport to ContentArea or something else will not be hard.
Next step for me here is to add public Size ContentSize and get scrolling working with just the keyboard.

Leverage what I did so far because it will no diverge so much.

I'm trying to do that!

Note, there IS another part of all this that needs to be figured out: What do we do about Clip. As you know, I am a fan of us making clipping far more sophisticated....an irregular region;

Well that's a real challenger, may be passing an array of rectangle

I just looked at #2606 again and am now confused. I SWEAR I implemented a really cool structure there that represnted an irregular region composed of rectangles called Region. I must have not pushed a commit ... which means whatever brilliant work I did (ha!) is now lost. I had modeled it after the old Windows User API https://learn.microsoft.com/en-us/windows/win32/gdi/regions.

Anyway, such a complex API may not be needed... a rectangle is probably fine. But if we're serious about great performance over constrained connections like SSH, it may be worth it.

Clearly, clipping only applies to Viewport (aka Bounds). IMO, by default, when View.OnDrawContent is called, the Clip should be set to Viewport and attempts do draw outside of the Viewport should not do anything.

That's also my opinion.

I have continued to ignore this because the one disadvantage of Clip is it hides poorly written drawing code and code that does things like Clear too frequently. But that will come later...

Sometimes the use of the Clear too frequently is because the View is forcing only using the Normal or Focus colors. If a view want to use a custom color other than them, have to override the View.OnDrawContent to do that.

Yep. Another reason we need to revisit the Color API (see #457).

@tig
Copy link
Collaborator Author

tig commented Mar 18, 2024

Oops. Just noticed a typo:

I think we're aligned here. The only difference is the "offset from the top-left (0,0) coordinate of the content to where the visiible area is" in my mind is Viewport.Location and the offset is provided in positive values. In the OnDrawContent API docs I linked to above "if ViewPort.Location.Y is 5 the 5th row of the content should be drawn using MoveTo (x, 5)."

This should read "...is 5 the 6th row of the content...".

@BDisp
Copy link
Collaborator

BDisp commented Mar 18, 2024

No, Window Forms does NOT use Bounds for the same thing. In Windows Forms Bounds is equivalent to our Frame:

Understood.

This is partially why I have long not liked the name.

Choose one that you like it.

FWIW, I started this PR because you told me you wanted me to, here:

#3295 (comment)

I'm approaching this with as fresh a perspective as I can, asking myself the most fundamental questions and trying to ignore legacy thinking. I find that when I do that, it helps my brain get clarity.

I confirm, but I also expect that you target more or less the same result as I achieve. Remember that I implemented the scroll bars on various views that are working great and I hope you don't break any of them. Otherwise this doesn't make sense. I expect you do it better than I did but with more features and improvements and not the other way.

I'm in the middle of thinking this through right now (by actually changing the code and API docs to see the impact). If you look at the current code in this branch you'll see this API doc paragraph:

The <see cref="Viewport"/> Location and Size indicate what part of the View's virtual content area, defined by <see cref="ContentSize"/>, is visible and should be drawn. The coordinates taken by <see cref="Move"/> and <see cref="AddRune"/> are relative to <see cref="Viewport"/>, thus if <c>ViewPort.Location.Y</c> is <c>5</c> the 5th row of the content should be drawn using <c>MoveTo (x, 5)</c>.

I doubt that will work well with positive offset. I think that is the contrary of the scrolling behavior. With positive offsets it look like you're pushing to the right and the bottom. But surely you know what you are doing.

I don't understand this point. What information would be missing in how I've defined things above that would prevent alignment from working?

Here I only was to note that for drawing we send the real content area, the one that use offset, for redraw contents to the view and not the visible area that is used to clip a region. I didn't mention that there is some problem with alignments at all.

Good. That's my current thinking too.

Great.

I think we're aligned here. The only difference is the "offset from the top-left (0,0) coordinate of the content to where the visiible area is" in my mind is Viewport.Location and the offset is provided in positive values. In the OnDrawContent API docs I linked to above "if ViewPort.Location.Y is 5 the 5th row of the content should be drawn using MoveTo (x, 5)."

This is make me confusing. If we are at (0,0) then we are at the start of the content, right?
If the ContentSize is bigger than the View.Width/Height we want to see the contents beyond the right and beyond the bottom, right?
For that we need to push to left and top, right?
Well tell me what values you have for them? Positive or negative?
For me they are negative and is with that way they are working well on my PR.

I'm trying to do that!

I know and you'll do it well.

I just looked at #2606 again and am now confused. I SWEAR I implemented a really cool structure there that represnted an irregular region composed of rectangles called Region. I must have not pushed a commit ... which means whatever brilliant work I did (ha!) is now lost. I had modeled it after the old Windows User API https://learn.microsoft.com/en-us/windows/win32/gdi/regions.

Anyway, such a complex API may not be needed... a rectangle is probably fine. But if we're serious about great performance over constrained connections like SSH, it may be worth it.

That is very interesting and I'm curious to see how they work with irregular areas.

Yep. Another reason we need to revisit the Color API (see #457).

For sure. It may be more configurable to use a view preference than force only the Normal and Focus, beside I understand the reason for that.

@BDisp
Copy link
Collaborator

BDisp commented Mar 18, 2024

Oops. Just noticed a typo:

I think we're aligned here. The only difference is the "offset from the top-left (0,0) coordinate of the content to where the visiible area is" in my mind is Viewport.Location and the offset is provided in positive values. In the OnDrawContent API docs I linked to above "if ViewPort.Location.Y is 5 the 5th row of the content should be drawn using MoveTo (x, 5)."

This should read "...is 5 the 6th row of the content...".

I understand because it start from 0. This is an example about why it should be negative. You are using as positive but in the reality it should be -5 because the first visible row will be the 0th. See how it work: -5, -4, -3, -2, -1, 0. The 0th will be in the reality the 6th row but ofseted.

@tig tig mentioned this pull request Mar 18, 2024
12 tasks
@tig
Copy link
Collaborator Author

tig commented Mar 20, 2024

I doubt that will work well with positive offset. I think that is the contrary of the scrolling behavior. With positive offsets it look like you're pushing to the right and the bottom. But surely you know what you are doing.

I think it will, but I want to make sure I understand your point. This mental model makes sense to me:

  • The View's content area has its origin (0, 0) at the top-left.
  • The View's content area is defined by ContentSize (a Size).
  • The Viewport is a rectangular "portal" into the View's content area with a location and a size.
  • Viewport.Location is the location where the top-left corner of the viewport resides within the content area.
  • Viewport.Size is the size of the viewport (the number of columns and rows visible to the user).
  • Ergo, Viewport.Location will always be >= 0, 0.

Let's compare this to ListView (assume no scrollbars):

  • ListView's content area has it's origin at (0, 0) at the top-left.
  • ListView's content area size is defined by the number of items in Source
  • If ListView.SelectedItem = 0 then the 1st row of it's content (ListView.Source[0]) is selected.
  • If the user uses the mouse wheel to scroll the ListView down one, the first visible item will now be ListView.Source[1].
  • Thus, the location of the first visible item is 1, not -1.

I struggle to articulate this so simply if Viewport.Location has negative values. How would you articulate it?

@tig
Copy link
Collaborator Author

tig commented Mar 20, 2024

Here I only was to note that for drawing we send the real content area, the one that use offset, for redraw contents to the view and not the visible area that is used to clip a region. I didn't mention that there is some problem with alignments at all.

Yes, you did:

That is what the GetVisibleContentArea method does to using by the ClipToBounds method and on others views, that use scrolling. Note that for the TextFormatter?.Draw we need to pass the real location and size, which the location is negative or not and the size is incremented by the negative location or not. Otherwise, it won't redraw as expected considering the Alignment, VerticalAlignment, etc...

I am trying to understand what you meant by "Otherwise, it won't redraw as expected considering the Alignment, VerticalAlignment, etc..."

@BDisp
Copy link
Collaborator

BDisp commented Mar 20, 2024

I think it will, but I want to make sure I understand your point. This mental model makes sense to me:

  • The View's content area has its origin (0, 0) at the top-left.

Correct.

  • The View's content area is defined by ContentSize (a Size).

No. It's defined by the view bounds size incremented by the negative location of the ContentOffset.

  • The Viewport is a rectangular "portal" into the View's content area with a location and a size.

If you meaning Viewport as the visible content area is always (0,0). If you meaning Viewport as the virtual area then the location is the negative values from the ContentOffset and the size is the bounds size incremented by the negative location.
Remember that ContentSize is always the total size of columns and rows, which mean that not all the content on the right and bottom sides are not yet visible.

  • Viewport.Location is the location where the top-left corner of the viewport resides within the content area.

Same answer as above.

  • Viewport.Size is the size of the viewport (the number of columns and rows visible to the user).

Same answer as above.

  • Ergo, Viewport.Location will always be >= 0, 0.

Same answer as above. If it is only the visible area used by the clipping area the location is always (0,0), otherwise is the negative values of the ContentOffset.

Let's compare this to ListView (assume no scrollbars):

You can't compare with ListView which already implemented the left and the top offset and in this case I use a UseContentOffset as false. I only grab their values to assign to the ContentOffset as negative values. If scrollbars are enabled then it use the values from the ContentOffset to set the Position property and vice versa.

  • ListView's content area has it's origin at (0, 0) at the top-left.
  • ListView's content area size is defined by the number of items in Source
  • If ListView.SelectedItem = 0 then the 1st row of it's content (ListView.Source[0]) is selected.
  • If the user uses the mouse wheel to scroll the ListView down one, the first visible item will now be ListView.Source[1].
  • Thus, the location of the first visible item is 1, not -1.

I didn't answer all of them because I'm not change the behavior how they offset left and top, like TextView, TableView, TreeView, etc... and I advise you not to change it. If you use the same code as they use you will struggle with the re-draw and with my solution I don't have to worry about that. The TextFormatter will draw correctly. Any derived View class can use the ContentOffset and the ContentSize properties to only redraw the contents from the positive values up to the available visible area.

I'm only using in views that the user set the UseContentOffset as true, with or without enable scollbars.

I struggle to articulate this so simply if Viewport.Location has negative values. How would you articulate it?

I think you haven't seen my PR properly yet. Otherwise you will understand how it works.

@BDisp
Copy link
Collaborator

BDisp commented Apr 15, 2024

I almost dare to say that the cause of the flickering is not related to the View drawing but to the way the Windows API processes its output to the screen. In WindowsDriver, WriteConsoleOutput and WriteConsole. In NetDriver the Console.Write. If there were problems with View drawing, it would also cause the same symptom in CursesDriver. Unless it is the value of the color attribute that is not being passed correctly to the output, since CursesDriver does not use ASCII escape sequences for color.

@BDisp
Copy link
Collaborator

BDisp commented Apr 15, 2024

Notice the detail in which when I press the mouse on the Border it causes a small blue flicker on the right side.

The cause of the shimmering blue color is due to the GetHighlightColor method returning an ARGB value in the Border_Highlight method.

@BDisp
Copy link
Collaborator

BDisp commented Apr 15, 2024

The other situations are caused by the TextFormatter?.Draw call in the OnDrawContent method of the Adornment class. It is now time for Driver.Clip to be changed to a Rectangle array.

@tig
Copy link
Collaborator Author

tig commented Apr 15, 2024

The other situations are caused by the TextFormatter?.Draw call in the OnDrawContent method of the Adornment class. It is now time for Driver.Clip to be changed to a Rectangle array.

I had reproduced the flicker-bar-thing in cases where there are no Views with (+) Thickness.

Not saying you're wrong, just that it's unlikely the only cause.

@tig
Copy link
Collaborator Author

tig commented Apr 15, 2024

Hewre

The other situations are caused by the TextFormatter?.Draw call in the OnDrawContent method of the Adornment class. It is now time for Driver.Clip to be changed to a Rectangle array.

I had reproduced the flicker-bar-thing in cases where there are no Views with (+) Thickness.

Not saying you're wrong, just that it's unlikely the only cause.

Here's Scrolling with no Adornments. Note OnDrawContent returns immediately if Thickness is Empty.

I set a breakpoint in both Adornment.OnDrawContent and Border.OnDrawcontent and they don't get hit.

JZKMHf3 1

@tig
Copy link
Collaborator Author

tig commented Apr 15, 2024

I also set progress.Visible = false in Scrolling.cs and it doesn't stop the flicker.

@tig
Copy link
Collaborator Author

tig commented Apr 15, 2024

Thought maybe it was Driver.FillRect so I re-implemented to access Contents directly (which is a good idea regardless as it'll be faster). Didn't fix anything.

   /// <summary>Fills the specified rectangle with the specified rune, using <see cref="CurrentAttribute"/></summary>
   /// <param name="rect">The Screen-relative rectangle.</param>
   /// <param name="rune">The Rune used to fill the rectangle</param>
   public void FillRect (Rectangle rect, Rune rune = default)
   {
       rect = Rectangle.Intersect (rect, Clip);
       lock (Contents)
       {
           for (int r = rect.Y; r < rect.Y + rect.Height; r++)
           {
               for (int c = rect.X; c < rect.X + rect.Width; c++)
               {
                   Contents [r, c] = new Cell
                   {
                       Rune = (Rune)' ', Attribute = CurrentAttribute, IsDirty = true
                   };
                   _dirtyLines [r] = true;
               }
           }
       }
   }

@tig
Copy link
Collaborator Author

tig commented Apr 15, 2024

This is only happening with WindowsDriver and NetDriver (Windows and WSL). With CursesDriver the flickering doesn't exist.

FWIW, it happens on WindowsDriver with both Force16colors set and not set. The code paths for cursor handling are very different for the two, which indicates it is NOT a cursor problem (but doesn't rule it out).

@BDisp
Copy link
Collaborator

BDisp commented Apr 15, 2024

FWIW, it happens on WindowsDriver with both Force16colors set and not set. The code paths for cursor handling are very different for the two, which indicates it is NOT a cursor problem (but doesn't rule it out).

I had already said above that in WindowsDriver it happens with both WriteConsoleOutput and WriteConsole, therefore with Force16colors or not. I think we should just assign the View class the responsibility of all writing operations to the console. I think there must be some conflicts with some views that are using the Driver to access Move, AddStr, AddRune. It must be the View class that obtains the mapping with the screen.

@BDisp
Copy link
Collaborator

BDisp commented Apr 15, 2024

Why does the CheckBox blink when clicking on the Border? Should not.

WindowsTerminal_zAWQtgAdmR.mp4

@tig
Copy link
Collaborator Author

tig commented Apr 15, 2024

Why does the CheckBox blink when clicking on the Border? Should not.

Because SetHightlight changes ColorScheme which calls SetNeedsDisplay() which is overkill. SetHighlight should be smarter about how it caues the Border to highlight, but I didn't think it all the way through.

That Window shouldn't be movable anyway.

@tig
Copy link
Collaborator Author

tig commented Apr 15, 2024

I removed the ability for the border to highlight. Centered flickers randomly by just clicking the border repeatedly.

Mfqlvi0 1

@BDisp
Copy link
Collaborator

BDisp commented Apr 15, 2024

That Window shouldn't be movable anyway.

Even if it was movable, this shouldn't happen. But if it is the Border's ColorScheme that is changed, it should not affect the CheckBox's ColorScheme.

@BDisp
Copy link
Collaborator

BDisp commented Apr 15, 2024

The horizontal ruler doesn't show anymore on the Scrolling scenario.

@tig
Copy link
Collaborator Author

tig commented Apr 15, 2024

That Window shouldn't be movable anyway.

Even if it was movable, this shouldn't happen. But if it is the Border's ColorScheme that is changed, it should not affect the CheckBox's ColorScheme.

The SetNeedsDisplay is causing the entire View to be Cleared and redrawn.

Regardless, it's not CheckBox that's causing the flicker. You are seeing the weird flickering thing. For you it just happens to be over Checkbox. for me it's over Centered.

@tig
Copy link
Collaborator Author

tig commented Apr 15, 2024

The horizontal ruler doesn't show anymore on the Scrolling scenario.

Sorry about that. I was debugging and hid some stuff. Fixed.

@tig
Copy link
Collaborator Author

tig commented Apr 15, 2024

I'm done trying to debug this flicker thing for now. So frustrating!

@BDisp
Copy link
Collaborator

BDisp commented Apr 15, 2024

The SetNeedsDisplay is causing the entire View to be Cleared and redrawn.

Regardless, it's not CheckBox that's causing the flicker. You are seeing the weird flickering thing. For you it just happens to be over Checkbox. for me it's over Centered.

You're right about that. By coincidence, for me, it appears on the line where the CheckBox is. I'm going to try changing the ConsoleDriver's Clip property so it can check multiple areas instead of one.

@tig
Copy link
Collaborator Author

tig commented Apr 15, 2024

The SetNeedsDisplay is causing the entire View to be Cleared and redrawn.
Regardless, it's not CheckBox that's causing the flicker. You are seeing the weird flickering thing. For you it just happens to be over Checkbox. for me it's over Centered.

You're right about that. By coincidence, for me, it appears on the line where the CheckBox is. I'm going to try changing the ConsoleDriver's Clip property so it can check multiple areas instead of one.

Let's move this discussion here:

#3408

@tig tig merged commit 56922b4 into gui-cs:v2_develop Apr 16, 2024
1 check passed
@tig
Copy link
Collaborator Author

tig commented Apr 16, 2024

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.

3 participants