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

Fix "Border.ForgroundColor" typo #2515

Closed
wants to merge 1 commit into from
Closed

Conversation

Nutzzz
Copy link
Contributor

@Nutzzz Nutzzz commented Apr 7, 2023

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

@Nutzzz Nutzzz requested review from migueldeicaza and tig as code owners April 7, 2023 22:37
@Nutzzz
Copy link
Contributor Author

Nutzzz commented Apr 7, 2023

You know, I kind of feel like this should be using an Attribute instead of separate fields for Foreground and Background.

@BDisp
Copy link
Collaborator

BDisp commented Apr 7, 2023

You know, I kind of feel like this should be using an Attribute instead of separate fields for Foreground and Background.

No, because theses colors is normally used alone to provide the same color for foreground and background, by calling SetAttribute(new Attribute(Foreground, Foreground) or SetAttribute(new Attribute(Background, Background). But also can be used with combinations along others attributes, SetAttribute(new Attribute(Foreground, otherAttr.Foreground) or SetAttribute(new Attribute( otherAttr.Background, Background).

@tznind
Copy link
Collaborator

tznind commented Apr 8, 2023

To provide some more context on why we typically use Color instead of Attribute.

  • Color is an enum so very easy to serialize / read from configs etc
  • Creating an Attribute requires a running initialzied ConsoleDriver. You can see if you go to the Attribute constructor it calls Application.Driver.GetColors.
  • Attribute is a struct which means it is copied by value not by reference. This means you can't pass it into another method and modify it etc.

Hopefully that clarifies things? Thanks for this fix :)

@tig
Copy link
Collaborator

tig commented Apr 8, 2023

The current v1 Border class will be deleted soon, so I'm not going to accept this PR.

What I'd like to ask @Nutzzz instead is:

Given the new Frame design in v2 (see https://github.com/gui-cs/Terminal.Gui/blob/v2_develop/docfx/v2specs/View.md), how what would you like the API for customizing the border on a View to look like? What is your scenario for having the border be a distinct color?

My current idea is to keep it really simple for most use cases:

var view = new View () { BorderStyle = BorderStyle.Single };

The implementation:

/// <summary>
/// Gets or sets whether the view has a one row/col thick border.
/// </summary>
/// <remarks>
/// <para>
/// This is a helper for manipulating the view's <see cref="BorderFrame"/>. Setting this property to any value other than
/// <see cref="BorderStyle.None"/> is equivalent to setting <see cref="BorderFrame"/>'s <see cref="Frame.Thickness"/> 
/// to `1` and <see cref="BorderStyle"/> to the value. 
/// </para>
/// <para>
/// Setting this property to <see cref="BorderStyle.None"/> is equivalent to setting <see cref="BorderFrame"/>'s <see cref="Frame.Thickness"/> 
/// to `0` and <see cref="BorderStyle"/> to <see cref="BorderStyle.None"/>. 
/// </para>
/// <para>
/// For more advanced customization of the view's border, manipulate see <see cref="BorderFrame"/> directly.
/// </para>
/// </remarks>
public BorderStyle BorderStyle {
	get {
		return BorderFrame?.BorderStyle ?? BorderStyle.None;
	}
	set {
		if (BorderFrame == null) {
			throw new InvalidOperationException ("BorderFrame is null; this is likely a bug.");
		}
		if (value != BorderStyle.None) {
			BorderFrame.Thickness = new Thickness (1);
		} else {
			BorderFrame.Thickness = new Thickness (0);
		}
		BorderFrame.BorderStyle = value;
		LayoutFrames ();
	}
}

BorderFrame is a Frame and thus derived from View. Therefore changing it's foreground and background colors is no different than any other view. To change the border colors of any View:

var view = new View () { BorderStyle = BorderStyle.Single };
view.BorderFrame.ColorScheme = Colors.ColorScheme ["Error"];

// or, if custom colors are wanted (not recommended):

view.BorderFrame.ColorScheme = new ColorScheme() { 
					Normal = new Terminal.Gui.Attribute(Color.Red, Color.White),
					HotNormal = new Terminal.Gui.Attribute (Color.Magenta, Color.White),
					Disabled = new Terminal.Gui.Attribute (Color.Gray, Color.White),
					Focus = new Terminal.Gui.Attribute (Color.Blue, Color.White),
					HotFocus = new Terminal.Gui.Attribute (Color.BrightBlue, Color.White),	
				};

What do you think?

@Nutzzz
Copy link
Contributor Author

Nutzzz commented Apr 8, 2023

That looks fine...

Would applying custom colors/styles to the superview's BorderFrames be inherited by child BorderFrames? Is there an easy way to change the color of the border title independent of the frame?

@tznind
Copy link
Collaborator

tznind commented Apr 8, 2023

When we have true color support it would be awesome to support Gradient borders.

No need to overcomplicate things now. But would be good to have Border support inheritence so we can have GradentBorder or TwoColorBorder (grey on Left/Bottom and white on Right/Top) etc.

image

@tig
Copy link
Collaborator

tig commented Apr 9, 2023

That looks fine...

Would applying custom colors/styles to the superview's BorderFrames be inherited by child BorderFrames? Is there an easy way to change the color of the border title independent of the frame?

Can you explain the use case for each of these?

@tig
Copy link
Collaborator

tig commented Apr 9, 2023

When we have true color support it would be awesome to support Gradient borders.

No need to overcomplicate things now. But would be good to have Border support inheritence so we can have GradentBorder or TwoColorBorder (grey on Left/Bottom and white on Right/Top) etc.

image

My vision is BorderFrame (also Margin and Padding) is really just a View that that has a clip region rectangle on the Inside.

When v2 is done, the actual rendering of the border and title will be done by subviews of BorderFrame. E.g. 4 LineView and Label. The LineViews (not v1's, but a new class based on LineCanvas) render to a LineCanvas that's shared by all other peers...hense the magic of auto-line joins.

So, yes.

@Nutzzz
Copy link
Contributor Author

Nutzzz commented Apr 9, 2023

Would applying custom colors/styles to the superview's BorderFrames be inherited by child BorderFrames? Is there an easy way to change the color of the border title independent of the frame?

Can you explain the use case for each of these?

Sure. Inheritance here would be to easily maintain a consistent look-and-feel. It's certainly easy enough to create the style once and apply every time you create a child, but ColorSchemes are inherited, and it's the same idea there. I also have the planned title styling in mind here. It is slightly more complicated for BorderFrames since we don't want to show a border by default for every kind of view. Just a thought: Perhaps it would be better to expand ColorSchemes and/or make a new class, say ElementStyle or something, to set defaults for the various UI elements of children. #2284 #2362

Changing the color of the title so it's not the same as the border lines could be a stylistic choice, or a way to draw attention to a change or an error, or to which window/pane/group etc. is focused.

@tig tig closed this Apr 10, 2023
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.

4 participants