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.

Sign up for GitHub

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

Jump to bottom

Style.GetFrameSize() does not account for border when using only BorderStyle() #281

Open
qualidafial opened this issue Apr 22, 2024 · 2 comments · Fixed by Brugarolas/lipgloss#9 · May be fixed by #282
Open

Style.GetFrameSize() does not account for border when using only BorderStyle() #281

qualidafial opened this issue Apr 22, 2024 · 2 comments · Fixed by Brugarolas/lipgloss#9 · May be fixed by #282
Labels
triage

Comments

@qualidafial
Copy link

Describe the bug

When rendering, a style configured with BorderStyle(border) renders the same as using Border(border) with no sides arguments, thanks to this check in applyBorder():

if border != noBorder && !(topSet || rightSet || bottomSet || leftSet) {
	hasTop = true
	hasRight = true
	hasBottom = true
	hasLeft = true
}

However, the border sizes reported from GetHorizontalBorderSize(), GetVerticalBorderSize(), GetHorizontalFrameSize(), and GetVerticalFrameSize() are not adjusted when using BorderStyle(). I'm new to the Bubbletea ecosystem and have been trying to use these functions to calculate content sizes, and kept getting frames that exceeded the intended size without understanding why.

To Reproduce

package lipgloss

import "testing"

func TestStyle_GetBorderSizes(t *testing.T) {
	tests := []struct {
		name  string
		style Style
		wantX int
		wantY int
	}{
		{
			name:  "Default style",
			style: NewStyle(),
			wantX: 0,
			wantY: 0,
		},
		{
			name:  "Border(NormalBorder())",
			style: NewStyle().Border(NormalBorder()),
			wantX: 2,
			wantY: 2,
		},
		{
			name:  "Border(NormalBorder(), true)",
			style: NewStyle().Border(NormalBorder(), true),
			wantX: 2,
			wantY: 2,
		},
		{
			name:  "Border(NormalBorder(), true, false)",
			style: NewStyle().Border(NormalBorder(), true, false),
			wantX: 0,
			wantY: 2,
		},
		{
			name:  "Border(NormalBorder(), true, true, false)",
			style: NewStyle().Border(NormalBorder(), true, true, false),
			wantX: 2,
			wantY: 1,
		},
		{
			name:  "Border(NormalBorder(), true, true, false, false)",
			style: NewStyle().Border(NormalBorder(), true, true, false, false),
			wantX: 1,
			wantY: 1,
		},
		{
			name:  "BorderTop(true).BorderStyle(NormalBorder())",
			style: NewStyle().BorderTop(true).BorderStyle(NormalBorder()),
			wantX: 0,
			wantY: 1,
		},
		{
			name:  "BorderStyle(NormalBorder())",
			style: NewStyle().BorderStyle(NormalBorder()),
			wantX: 2,
			wantY: 2,
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			gotX := tt.style.GetHorizontalBorderSize()
			if gotX != tt.wantX {
				t.Errorf("Style.GetHorizontalBorderSize() got %d, want %d", gotX, tt.wantX)
			}

			gotY := tt.style.GetVerticalBorderSize()
			if gotY != tt.wantY {
				t.Errorf("Style.GetVerticalBorderSize() got %d, want %d", gotY, tt.wantY)
			}

			gotX = tt.style.GetHorizontalFrameSize()
			if gotX != tt.wantX {
				t.Errorf("Style.GetHorizontalFrameSize() got %d, want %d", gotX, tt.wantX)
			}

			gotY = tt.style.GetVerticalFrameSize()
			if gotY != tt.wantY {
				t.Errorf("Style.GetVerticalFrameSize() got %d, want %d", gotY, tt.wantY)
			}

			gotX, gotY = tt.style.GetFrameSize()
			if gotX != tt.wantX || gotY != tt.wantY {
				t.Errorf("Style.GetFrameSize() got (%d, %d), want (%d, %d)", gotX, gotY, tt.wantX, tt.wantY)
			}
		})
	}
}

The final test fails with these errors:

=== RUN   TestStyle_GetBorderSizes/BorderStyle(NormalBorder())
    borders_test.go:66: Style.GetHorizontalBorderSize() got 0, want 2
    borders_test.go:71: Style.GetVerticalBorderSize() got 0, want 2
    borders_test.go:76: Style.GetHorizontalFrameSize() got 0, want 2
    borders_test.go:81: Style.GetVerticalFrameSize() got 0, want 2
    borders_test.go:86: Style.GetFrameSize() got (0, 0), want (2, 2)
    --- FAIL: TestStyle_GetBorderSizes/BorderStyle(NormalBorder()) (0.00s)

Expected behavior

Given that BorderStyle(border) renders the same as Border(border), I would expect methods that calculate border / frame sizes to return the same sizes as when using Border().

This could be resolved in several different ways:

  • Perform an on-the-fly calculation as is being done in applyBorder, under the same conditions. This is probably the safest option.
  • Have BorderStyle explicitly set all border sides to visible if none of the border side keys have been set yet. This might be risky depending on whether somebody is using inherited styles, and in what order they set their style properties.
  • Invert the default visibility in the methods that calculate border sizes of each border, e.g.:
func (s Style) GetBorderLeftSize() int {
	if !s.getAsBool(borderLeftKey, true) { // <- currently this uses a default of false.
		return 0
	}
	return s.getBorderStyle().GetLeftSize()
}

This last approach could also risk changing behavior for existing applications, if they are relying on explicit calls like BorderLeft(true) to turn on border sizes.
@qualidafial
Copy link
Author

I'm happy to submit a PR for this if the team has a preference on which solution they prefer.

I personally favor computing these on the fly in the Get<Horizontal|Vertical><Border|Frame>Size() methods as it seems to agree with how Render enables the border on the fly.

qualidafial added a commit to qualidafial/lipgloss that referenced this issue Apr 22, 2024
@qualidafial
fix: return consistent border sizes when BorderStyle is used.
86319a6 
Return non-zero border sizes when border style is set, and no
border sides have been specifically turned on or off.

Fixes: charmbracelet#281
@qualidafial qualidafial linked a pull request Apr 22, 2024 that will close this issue
Open
@qualidafial
Copy link
Author

Ping! I've submitted PR #282 as a proposed fix for this. Would love your feedback!

@Brugarolas Brugarolas mentioned this issue May 3, 2024
Merged
qualidafial added a commit to qualidafial/lipgloss that referenced this issue May 7, 2024
@qualidafial
fix: return consistent border sizes when BorderStyle is used.
4a09ade 
Return non-zero border sizes when border style is set, and no
border sides have been specifically turned on or off.

Fixes: charmbracelet#281
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
triage
Projects
None yet
Milestone
No milestone
2 participants
@meowgorithm @qualidafial