Fixes 2343 - LineCanvas not respecting X and Y of clip bounds

[tznind]

Fixes #2343 - GenerateImage no longer assumes clip area starts at 0,0

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

[tig]

How come you didn't include the unit test I provided in my Issue?

I have use case where the I want to draw outside of View.Bounds and I need to be able to pass Draw a bounds that is larger than View.Bounds.

[tznind]

Changing this to draft while I have time to think about this more deeply View.Bounds property is only ever 0,0 since it's setter is:

set => Frame = new Rect (frame.Location, value.Size);

And it's getter explicitly sets the point to 0:

get => new Rect (Point.Empty, Frame.Size);

235114e is definetly needed to fix GenerateImage but I need to dig more into the Bounds test and understand what is being translated and where.

My feeling is that with Draw(View view, Rect bounds) should actually be Draw(View view, Rect source) or Draw(View view, Rect source, Point startDrawingAt) . The Rect it gets is where in the coordinate space of the LineCanvas you want to source the runes from. Where you then draw them within the View is a seperate question and API should be flexible there.

[tig]

View.bounds also defines the nominal clip region too.

Thinking about it more i don't really require this to be about view.bounds. What I need is an api that takes a rect and renders the canvas to it. I actually modified LineCanvas.Draw at one point in my adventures to treat rect as Screen relative if the view param was null.

But I backed that out because I could figure how to make it work (now I know this was because of THIS issue). Lol.

[tznind]

Ok. Draw signature is now this:

/// <summary>
/// Draws all the lines that lie within the <paramref name="sourceRect"/> onto
/// the <paramref name="view"/> client area at given <paramref name="drawOffset"/>.
///  This method should be called from
/// <see cref="View.Redraw(Rect)"/>.
/// </summary>
/// <param name="view"></param>
/// <param name="sourceRect">The area of the canvas to draw.</param>
/// <param name="drawOffset">The point within the client area of 
/// <paramref name="view"/> to draw at.</param>
public void Draw (View view, Rect sourceRect, Point? drawOffset = null)
{
 ...
}

I've updated your test to TestLineCanvas_LeaveMargin_Top1_Left1. But I want to make sure I understand the requirements so am happy to add other tests if it doesn't represent your requirement.

The new draw method implementation changes the following:

• You can now draw anywhere in the view client area (specified by offset). The test gives a draw offset of 1.
• The source rect is the part of the canvas you want drawn and now correctly supports x and y of the sourceRect

This method uses View.AddRune for rendering so you cannot draw outside of the bounds of the View.

What I need is an api that takes a rect and renders the canvas to it.

We could add a DrawToScreen method which does allow for this kind of over-spilling. It's signature would be something like:

public void DrawToScreen(Rect sourceRect, Point? screenDrawOffset = null)

[tig]

Ok. Draw signature is now this:

/// <summary>
/// Draws all the lines that lie within the <paramref name="sourceRect"/> onto
/// the <paramref name="view"/> client area at given <paramref name="drawOffset"/>.
///  This method should be called from
/// <see cref="View.Redraw(Rect)"/>.
/// </summary>
/// <param name="view"></param>
/// <param name="sourceRect">The area of the canvas to draw.</param>
/// <param name="drawOffset">The point within the client area of 
/// <paramref name="view"/> to draw at.</param>
public void Draw (View view, Rect sourceRect, Point? drawOffset = null)
{
 ...
}

I've updated your test to TestLineCanvas_LeaveMargin_Top1_Left1. But I want to make sure I understand the requirements so am happy to add other tests if it doesn't represent your requirement.

The new draw method implementation changes the following:

• You can now draw anywhere in the view client area (specified by offset). The test gives a draw offset of 1.
• The source rect is the part of the canvas you want drawn and now correctly supports x and y of the sourceRect

This method uses View.AddRune for rendering so you cannot draw outside of the bounds of the View.

What I need is an api that takes a rect and renders the canvas to it.

We could add a DrawToScreen method which does allow for this kind of over-spilling. It's signature would be something like:

public void DrawToScreen(Rect sourceRect, Point? screenDrawOffset = null)

Let me ask this question:

• What use-case drives LineCanvas having ANY dependency on View? Why can't it always just operate on screen-relative coordinates and have callers do whatever View->Screen mapping required before using it?

The ONLY place you reference View in the current design is within Draw, right?

I've been thinking a ton about the current View class-hierarchy architecture and one of the biggest drivers of the (what I think is unneeded) complexity is a lack of clarity between: Screen coords (Frame), content-area coords (Bound), and the clip rect for Draw (which, sadly, is named 'bounds' everywhere).

(Oh, and, BTW, referring to Frame as "screen" coords is also broken: Someday (v2?) we will support non-full-screen apps. In this case Frame needs to be "Application.Top.Frame relative", not screen relative).

I think a primitive drawing API like LiineCanvas that really has no need to know about Views should be independent of that hierarchy. I'm sorry I didn't notice this when I did the original CR.

[tznind]

Fair points. This is definitely worth exploring to get a good API. LineCanvas is indeed a primitive self contained class. Originally the 'output' method of the class was only the public method public Rune? [,] GenerateImage (Rect inArea). This method is still there and can be used (and this PR fixes its starting offset).

I added Draw method as a helper for rendering to a View client area. Mostly because working out how to iterate over a 2D array is annoying especially for new programmers. Maybe a more user friendly return type would be Dictionary<Point,Rune>?

I can delete the Draw helper method to remove the dependency on View and we can just deal with the GenerateImage method directly instead?

[tznind]

Oh, and, BTW, referring to Frame as "screen" coords is also broken:

Do you mean you want less references to "screen" (e.g. ScreenToView, ViewToScreen)...

Frame is not the same as screen. Views can be nested and the Frame property is just the location within the immediate parent's client area. So they are 3 separate things (client area, frame and screen).

I think there is still value in using the 'screen' terminology for what you get when you use Driver.Move(..) directly.

[tznind]

Ok have switched to Dictionary as return type of GenerateImage and removed Draw completely.

So should be super easy to use in any context regardless of whether it is client area or screen coordinates or writing out to a file etc etc.

Since we have not done any releases this should just be a breaking change for the split container PR (which will be easy to fix).