-
Notifications
You must be signed in to change notification settings - Fork 228
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
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
@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 |
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 |
ef0557f
to
8c0be42
Compare
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.
8c0be42
to
36483f4
Compare
@maaslalani Can you please take another look at this? I have rebased it from |
Thanks @mikelorant, I will take a look! Thanks for the reminder! |
Thanks @mikelorant, tested things out in the example and worked well. Thank you so much for the extensive test suite also, it really makes these PRs easier to merge. Really appreciate the patience and great work ❤️ |
Wahoo. Finally merged! Thank you so much for your efforts in making sure we keep the quality high. |
Upgrade dependencies to latest releases. As part of this upgrade there were two packages worth mentioning. The `bubbles` package was previously using a modified fork that added support for [textarea multiline placeholders][1]. This change has now been merged upstream and there is no longer a need to rely on the custom version. The `coloredcobra` package which provides the colours for the CLI help text was impacted by the upgrade of the `color` package. A [fix][2] has been applied that currently only exists in a forked version. This is necessary until the upstream merges the pull reques to fix this problem. [1]: charmbracelet/bubbles#302 [2]: ivanpirog/coloredcobra#5
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.