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

Adds LineCanvas #2281

Merged
merged 14 commits into from
Feb 5, 2023
Merged

Adds LineCanvas #2281

merged 14 commits into from
Feb 5, 2023

Conversation

tznind
Copy link
Collaborator

@tznind tznind commented Jan 7, 2023

drawing

Fixes #_____ - Adds new class for drawing connected lines in arbitrary configurations

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

BDisp commented Jan 7, 2023

@tznind how about to do the same feature for double lines and for rounded corners, single and double lines?

@tznind
Copy link
Collaborator Author

tznind commented Jan 7, 2023

@tznind how about to do the same feature for double lines and for rounded corners, single and double lines?

Should be pretty easy to add a setting for rounded corners on the class.

It already supports double lines and mixing and matching single and double lines.

@BDisp
Copy link
Collaborator

BDisp commented Jan 7, 2023

Should be pretty easy to add a setting for rounded corners on the class.

It already supports double lines and mixing and matching single and double lines.

Great. The Border already use it, maybe you can use the same glyph's.

@tig
Copy link
Collaborator

tig commented Jan 8, 2023

💘

@tig
Copy link
Collaborator

tig commented Jan 8, 2023

Why StraightLineCanvas? Why not just LineCanvas? I can't imagine (probably because I'm dum) anything but straight lines in a TUI.

@tznind
Copy link
Collaborator Author

tznind commented Jan 8, 2023

Why _Straight_LineCanvas? Why not just LineCanvas? I can't imagine (probably because I'm dum) anything but straight lines in a TUI.

Hmn its not great is it...

I was trying to articulate that it only supports Horizontal or Vertical lines (i.e. not diagonal). By contrast the PathAnnotation in the GraphView supports diagonal lines (although it just uses dots).

I think you are right though and we can probably just go with LineCanvas and in the AddLine xmldoc we can emphasise that its horizontal/vertical rather than PointA to PointB. And who knows maybe down the line we do add support for other line types e.g.

image

And who knows maybe down the line we do add support for other line types e.g. with braille:

image

@tznind tznind changed the title Adds Straight Line Canvas Adds LineCanvas Jan 8, 2023
@tznind tznind marked this pull request as ready for review January 8, 2023 11:53
@tznind
Copy link
Collaborator Author

tznind commented Jan 8, 2023

I've marked this ready.

There is room for improvement later on e.g. when making a T that goes from a double line down to a single line you can use  ╨ but the number of combinations gets big fast and it would need quite a bit of work to implement them all.

We can always go down that route later.

@BDisp
Copy link
Collaborator

BDisp commented Jan 8, 2023

We can always go down that route later.

You are right. I think for now you can force to all borders being single or double (vertical and horizontal). When you have time then you will have a lot of work 😃.
That why I didn't implementing rounded corner for double because it probably need to use others codes for double vertical and horizontal lines.

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.

Is this puppy ready?

UICatalog/Scenarios/Drawing.cs Outdated Show resolved Hide resolved
Terminal.Gui/Core/Graphs/LineCanvas.cs Outdated Show resolved Hide resolved
tznind and others added 2 commits January 18, 2023 17:20
Rename Drawing Scenario
Make API simpler by directly referencing Application.Driver
@tznind tznind requested a review from tig January 21, 2023 21:37
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.

This is awesome.

One question @tznind - have you considered how ConsoleDriver.DrawFrame would be re-factored to use this?

@tig tig merged commit 6a47427 into gui-cs:develop Feb 5, 2023
@tznind tznind deleted the line-drawer branch February 5, 2023 22:47
@tznind
Copy link
Collaborator Author

tznind commented Feb 5, 2023

This is awesome.

One question @tznind - have you considered how ConsoleDriver.DrawFrame would be re-factored to use this?

Thanks for review and merge.

I'll take a look into refactoring frame drawing, should cut down the code quite a bit but will have to be careful not to make mistakes and add good code coverage.

Currently would be something like the below. But could add helper methods to LineCanvas to make it less verbose.

var canvas = new LineCanvas();
canvas.AddLine(new Point(region.X + paddingLeft,fleft),fwidth,Orientation.Horizontal,borderStyle);
canvas.AddLine(new Point(ftop,fleft),fheight,Orientation.Vertical,borderStyle);
// TODO: Add lower left lines up and left

var bmp = canvas.GenerateImage(region);
for(int y=0;y< bmp.GetLength(0) ;y++)
for(int x=0;x< bmp.GetLength(1) ;x++)
{
	var rune = bmp[y,x];
	if(rune.HasValue)
	{
		AddRuneAt (region.X + x, region.Y + y, rune.Value);
	}
}

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