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
feat(textarea) Add multiline placeholder #302
base: master
Are you sure you want to change the base?
feat(textarea) Add multiline placeholder #302
Conversation
805ce3b
to
c5687de
Compare
3cff057
to
5585074
Compare
@meowgorithm @maaslalani Can I get approval to run the workflow? Thanks. |
Can I get someone from @charmbracelet to review this. Thanks. |
Hey @mikelorant. Thank you so much for this PR. I'm seeing a bug with how this PR is handling line numbers: This is what the textarea should look like: Here is what it looks like using this PR (rebased on main): |
The testing code I'm using can be found here: https://github.com/charmbracelet/bubbletea/tree/master/examples/textarea |
Just to clarify, line numbers only display if there is user input (or cursor) on that line? |
Just did some basic investigation and it seems the aim is that it should look like Vim with line numbers enabled. Which means only lines with content get a number. |
Yep, that's correct! Similar to vim! |
5585074
to
97e8494
Compare
Have fixed the issue you have reported and here is the result using the code you linked: Using a multiline placeholder shows like this: I wish to make it clear that line numbers are only for lines that have a cursor or entered text. This is placeholder text, so only the first line will ever have a line number in this case. As soon as you start typing and adding new lines, none of this code is used. I've had to extensively increase the unit tests because it was far too complex checking all the variations. As part of this process I needed a small helper function to strip ANSI and trailing white spaces to make comparison easier. I would have preferred to use golden files however I think this is a reasonable compromise. Unfortunately no other tests in |
@maaslalani Any chance you could do a review, if this is working correctly I'd like to get it merged otherwise if there are problems, lets me get started on fixing them. |
Hey @mikelorant, thanks again for your work on this one! One issue I'm seeing is the CursorLine (if it has a background color, bleeds over to the line number column) Here's what it should look like: |
The default prompt which is being used here is defined at: Code as follows:
This has me thinking that both versions are wrong. The background colour should start at position 3 for each line.
The Thoughts? Additional notes: Consider the following change:
This would put the changed background colour outside the left border which would be very wrong. |
@maaslalani I'll quickly do the update to this pull request providing the original functionality, but I still believe both implementations have it wrong. Using Vim as the reference, highlighted cursor line goes as far as the beginning of the line number or end of buffer character. It is hard to know what we should do if we put a border around it since we cant do that in Vim. |
Here are the two possibilities in one image: The top version is how it is currently implemented and the bottom version is how I think it should be. However as a lowly minion here to serve the wonderful Charm team 😄 I have pushed up the version that conforms with how it is also displayed when text is entered. Thanks for helping make sure we catch all the little subtle things, very much appreciated @maaslalani. |
97e8494
to
5f819d1
Compare
fc81153
to
a2668fb
Compare
@mikelorant Thanks for the care you're putting into this PR! Yes, I see what you mean by the border. However, when the cursor line is defined with a background color I think it looks more correct (but maybe we will change this later since you are actually right about the correctness technically) Also when a text area has a multiline placeholder I believe the cursor line background should be applied to the entire placeholder. In your screenshots it only applies to the first line. |
In my opinion the second option is correct here as the entire line is highlighted with the cursor line and if the user types that exact placeholder then the same highlighting will occur. |
a2668fb
to
bf2cc79
Compare
@maaslalani Commit updated to highlight all lines of the placeholder. |
bf2cc79
to
699fb3e
Compare
That is, long placeholders should behave as multi-line placeholders. |
Agreed, let me add a test case and fix for this one. |
e7d2b21
to
0c30b26
Compare
@maaslalani This started to get complicated so I had to use Have lots of test cases (include some edge cases) to verify this is working as expected. |
0c30b26
to
8aee984
Compare
Added a few more test cases because I am paranoid of all the edge cases 😄 |
8aee984
to
1e8da29
Compare
@maaslalani Don't believe we have any issues left with this pull request. Would love to get this merged as my own app depends on it. |
We're waiting on muesli/reflow#71 for this one, correct? |
@meowgorithm Not at all. This one is ready to go. This has nothing to do with emojis or grapheme clusters. |
1e8da29
to
54bf36a
Compare
@maaslalani Rebase completed and all unit tests are passing. 🤞 that there are no more issues. |
@meowgorithm @maaslalani Can we please get this reviewed. Want to get this finalised so I can move off using my own fork. |
@mikelorant Will take a look today |
@mikelorant I'm still seeing the bug of long placeholders, let me know if I'm doing something incorrectly. But, if I set the placeholder to: ti.Placeholder = "Once upon a time in a land far far away..." Then the text is broken. However if I add a new line it works as expected: ti.Placeholder = "Once upon a time in a land far far\naway..." I wouldn't mind truncating the placeholder like we currently do if the user has not explicitly opted into the newline, if that easier. |
view: heredoc.Doc(` | ||
> placeholder the first line that is | ||
> longer than the max width |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add a test where the CursorLine
is styled like we see in the bubbletea example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots more tests coming up.
No truncating, will work out what is going on. Just need lots more tests to find all these weird edge cases that keep popping up. Not sure why this feature has been so difficult to implement 😢 Thanks for finding these problems. |
Found the bug and unfortunately it seems like we may have 2 bugs 😢 Firstly, my bug is quite simple, I was using However, this has uncovered something new with all the tests we have added. Looking into the function if m.MaxWidth > 0 {
m.width = clamp(inputWidth, minWidth, m.MaxWidth)
} else {
m.width = max(inputWidth, minWidth)
} If the Hoping my logic here is correct, will keep hacking away and see where this leads. |
Going to add another problem, setting I believe the order we need to follow is: ta := textarea.New()
ta.MaxWidth = 25
ta.SetWidth(20) This might be a case where Definitely need some guidance on how this should work. |
Sorted out all the problems, however this is now dependent on #496. |
Thanks for debugging the |
Add the capability to show a multiline placeholder. Some refactoring was required to improve readability and improve logic. End of line buffer character was only shown when line numbers were displayed which requires some verification whether this is the intended outcome. This change resolves this issue.
54bf36a
to
ef0557f
Compare
Have fixed the conflicts and unit tests now pass. Will run some manual tests to see if we have all the issues finally resolved 😄 |
@maaslalani Actually, everything looks OK from both unit tests and manual tests. Hopefully you see the same results. |
@maaslalani Can we get another review of this one, I am hoping this one passes now that we have the |
Add the capability to show a multiline placeholder. Some refactoring was required to improve readability and improve logic.
End of line buffer character was only shown when line numbers were displayed which requires some verification whether this is the intended outcome. This change resolves this issue.