fix: return consistent border sizes when BorderStyle is used.

[qualidafial]

Return non-zero border sizes when border style is set, and no border sides have been specifically turned on or off.

Fixes: #281

[qualidafial]

@meowgorithm ping. I've pushed the changes you requested. Would you mind taking another look?

[qualidafial]

Ping?

[bashbunni]

I think this should probably either short-circuit with s.shouldAutoEnableBorder or continue to have false as the second arg here. I don't think this function call is necessary unless I'm missing something 🤔

e.g.

if !s.shouldAutoEnableBorder() || !s.getAsBool(borderTopKey, false)

Although, again not sure this check saves us in efficiency

[bashbunni]

If it's to account for the border value set in BorderStyle, wouldn't that be handled already at line 306?

[qualidafial]

The double negative !a || !b style is confusing (and I think it should actually be && not ||). However I think if we invert the if condition it will be easier to follow:

if s.shouldAutoEnableBorder() || s.getAsBool(borderTopKey, false) {
	return s.getBorderStyle().GetTopSize()
}
return 0

It breaks the Go idiom of putting the happy path return at the bottom, but I think the improvement in clarity is worth it.

[qualidafial]

If it's to account for the border value set in BorderStyle, wouldn't that be handled already at line 306?

That would ignore the show/hide border flags set on the style, e.g. under borderTopKey, which would be a regression I believe. At the very least it would change the behavior of many existing programs which users could rightfully consider a regression.

[bashbunni]

Also related: (and might eliminate the need for multiple checks of BorderStyle)
https://github.com/charmbracelet/lipgloss/pull/380/files
#353

I tried adding a custom border in the test introduced here and it's not fixed by this PR (doesn't work on master either). I do still think this is a step in the right direction though

Here's the failing test case in case you're curious:

 		{
			name:  "Custom BorderStyle",
			style: NewStyle().BorderStyle(Border{Left: "      new"}),
			wantX: Width("      new"),
			wantY: 0,
		},

[qualidafial]

Also related: (and might eliminate the need for multiple checks of BorderStyle) https://github.com/charmbracelet/lipgloss/pull/380/files #353

I tried adding a custom border in the test introduced here and it's not fixed by this PR (doesn't work on master either). I do still think this is a step in the right direction though

Here's the failing test case in case you're curious:

 		{
			name:  "Custom BorderStyle",
			style: NewStyle().BorderStyle(Border{Left: "      new"}),
			wantX: Width("      new"),
			wantY: 0,
		},

Based on my read of Style.applyBorder, it looks like successive runes in a left or right border are meant to be rendered vertically. e.g. rendering the border Border{Left:"|:",Right:"<>"} around the string "foo\nbar\nbaz" would look like:

|foo<
:bar>
|baz<

So I think wantX: 1, is the appropriate expectation from the test case above.

[qualidafial]

@bashbunni @meowgorithm I've pushed additional changes based on your comments.

[meowgorithm]

Thanks for your patience and effort with this one, @qualidafial. It occurred to me (and @bashbunni): is this basically the same issue as #411? If so we can probably go with this one, but bring in the getters on my branch.

[qualidafial]

I like the implicitBorders method name better, but yes it looks like both PRs are addressing the same issue. Feel free to pull over my unit tests to your PR if you like.