Skip to content

fix(filepicker): remove loop iteration appending extra newline#914

Open
bntrtm wants to merge 1 commit intocharmbracelet:mainfrom
bntrtm:fix-fp-terminal-spacing
Open

fix(filepicker): remove loop iteration appending extra newline#914
bntrtm wants to merge 1 commit intocharmbracelet:mainfrom
bntrtm:fix-fp-terminal-spacing

Conversation

@bntrtm
Copy link
Copy Markdown

@bntrtm bntrtm commented Mar 23, 2026

The Issue

The lines in question are implemented for the purposes of filling out empty terminal space. At least, that's my read on it. However, by adding a loop iteration for where the height of the filepicker string equals the height of the terminal itself, we end up adding an extra newline we don't need.

Example

Context first. Assume that the filepicker model's internal height variable is 4, our terminal height set with SetHeight. Keep in mind that lipgloss.Height returns num \n chars + 1 to calculate the cell height. Assume that s, by the time the loop begins, is a_file\nb_file\nc_file.

With condition being i <= m.Height():

# cell height of filepicker.View() output is 5. Unexpected behavior; 5 =/= 4.
a_file\n
b_file\n
c_file\n # i < 4, this newline is added
\n # i == 4, this newline is added
# EMPTY LINE

With condition being i < m.Height():

# cell height of filepicker.View() output is 4, matching expected `filepicker.height`! Expected behavior.
a_file\n
b_file\n
c_file \n # i < 4, this newline is added
# EMPTY LINE

When simply using the filepicker model in and of itself, we don't really see the consequence of the unexpected behavior. But we do begin to see it as soon as we want to nest it within other models, as seen in the case of the gum file model.

If desired, this branch in my fork of gum demonstrates what this fix looks like in v1.0.0 (the same logic applies in this v2 PR; gum simply uses v1 charm versions), alongside a (only somewhat unrelated) gum fix. If you clone it and change the conditional operator under filepicker.View() from < back to <=, you'll find that the command to run gum file unexpectedly remains rendered on the first line of the TUI, without this fix.

Related Issues

I personally have a theory that a lack of clarification on lipgloss.Height's in-line documentation is what led to this conditional bug.

If this fix is merged, I would reason that an identical fix is necessary in v1 for gum's sake. If a maintainer is willing to open a v1 maintenance branch, I can submit an identical PR for that; I have a branch already made for that purpose.

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.

1 participant