Text field refactor - word wrap support #200
Conversation
7073475 to
bca5f4f
Compare
|
Thanks for rebasing your branch! Let me know why you've fixed the bugs and this is ready for review. |
I think I got confused somewhere in the rebase and picked the wrong change. It was a little confusing because I did a few merges from master earlier (mistake). Just want to mention that this change does include a little tweak to indent the message lines and align them with the nickname. It's easily removable. I'm not quite sure I personally like it yet, but maybe it's ok. Also there was a bug you mentioned on the last PR, where someone had posted a long link and it moved it to a new line. I tried this out on master and it happens there. The cause is because we split on the space after the ": " so it moves the whole link to the next line. It doesn't like when the user sends text with no whitespace either. It gets cut off. I solved this in |
|
Great, thanks. Started reading an testing. One bug is in some cases it crashes with this backtrace (you need a debug build): BacktraceNot sure how to best reproduce this, I just made the screen small and edited randomly until it crashed like this. |
|
Ok, thanks, I will take a look. I probably missed a place where we need to invalidate the TexTArea's height on edit/history/autoocomplete |
osa1
left a comment
There was a problem hiding this comment.
Thanks @trevarj , this looks great now! Added a few inline comments. Two high-level comments:
- Could you hide aligning messages in the message area behind a flag? It's fine to keep it but it shouldn't be the default. (Note to self: review that code)
- Could you document when we fall back to wrapping exactly?
I also think we should make this the default and remove text_field_wrap setting. What do you think?
libtiny_tui/src/input_area/mod.rs
Outdated
| /// Get the scroll value based on width of TextArea | ||
| /// If the width is less than the width defined on SCROLL_FALLBACK_WIDTH | ||
| /// we will transfer to scroll mode, otherwise we will continue to wrap text. | ||
| fn get_scroll_for_resize(&mut self) -> Option<i32> { |
There was a problem hiding this comment.
It would be nice to have a way to fall back to scrolling when the TextField height is greater than, say, half of the entire window height, but I don't think it would be a clean implementation. On resize, we invalidate the height and then calculate it later while drawing.
I think this is a decent way for now, unless someone uses a really wide and short window 😆
I removed this for now. I think it's too hacky and not a really nice solution right now. I think #169 can address this.
I agree. Done. |
|
I've been testing this -- so far if I only edit when the cursor is at the end everything works fine, I couldn't find any bugs. But when I edit in the middle things get weird. Here's an example:
Note that some part of the text field is written to the tab line. I can even move my cursor there and edit it:
|
|
@osa1 thanks for testing. I can reproduce this, but only when there's a full line with no whitespace at all. I will have to add some extra checks on |
|
Not really happy with the fix that I had to do to get this working 👀, but it seems ok. Maybe you will have a better idea than I could come up with. We will have to address a similar issue with MsgArea as well - like in that screenshot where someone sent a multi-line link and it didn't render correctly. |
|
Thanks @trevarj , it seems much better. I still find bugs though. One example:
Now if I resize the screen
Note that there's more than enough space in the first line for the next word, which is just one characters, but the character is still on the next line. I wonder if we could simplify the implementation, maybe at the cost of some performance, to make this easier to implement and maintain. I haven't tried to implement this myself, but it seems quite tricky. |
|
Yeah, this is demoralizing. I would imagine that other programs that do this are probably taking a bigger performance hit in order to make things more robust. If you can show me in more detail how to reproduce this one bug, maybe I can fix it. If there are a lot more than just this one than it's probably better to think of another solution... |
Let's take this approach. Thinking about this more, I think it's safe to assume that text field will be edited very rarely (most of the time tiny will be rendering incoming messages and waiting idle). Resizes are also rare, and resizing while text field is not empty should be even more rare. So assuming the common case (when the text field is empty) is fast when resizing and rendering (which should be the case, it's trivial to render and resize an empty text field) I think performance hits here will be acceptable. |
Basically I'm adding some long text, then removing stuff in the middle, and resize every once in a while, and at each step (after every key press basically) I look and check if it looks as expected. |
|
Ok 😅. I think that the refactoring into the separate module is good and should stay. I will start thinking of how to make this simpler. If you come up with any sleek ideas, let me know! |
|
Tried to keep things simple and comment everything so it all makes sense. I'm glad to have removed the BTree stuff, which was nasty looking. |
|
Working on adding a limit of lines. I'd hate to add the magic number "5" maximum lines again. Would rather it be half of the window height. |
|
Yeah I think half of the window height is a good heuristic. If it'll make things simpler we could use a magic number, but even if we did that at some point we'd have to check window height anyway, for example if the magic number is "5" but window height is "6", even though we have enough space we should fall back to scrolling to give space to other widgets. So I think hard-coded magic number will not make this any more simpler. "No more than half of the window height" handles that nicely. If the ratio (1/2) is not great we could tweak it later easily if you could make it a parameter (just define it as a constant in the module). |
| ) { | ||
| let slice: &[char] = &line[scroll as usize..min(line.len(), (scroll + width + pos_x) as usize)]; | ||
| let slice: &[char] = | ||
| &line[scroll as usize..min(line.len(), (scroll + (width - pos_x)) as usize)]; |
There was a problem hiding this comment.
Fixed a bug that I introduced
| height: Option<i32>, | ||
|
|
||
| /// Maximum lines the widget can grow to | ||
| max_lines: i32, |
There was a problem hiding this comment.
Gets calculated on resize(), sent from MessagingUI
| /// Gets the height of the widget | ||
| /// If the height is larger than the allowed max lines turn on scroll | ||
| /// else turn off scroll | ||
| pub(crate) fn get_height(&mut self, width: i32) -> i32 { |
There was a problem hiding this comment.
I think it makes the most sense to add the fallback code here. It abstracts it away from the draw() and resize() of MessagingUI.
libtiny_tui/src/input_area/mod.rs
Outdated
| pub(crate) fn resize(&mut self, width: i32, max_lines: i32) { | ||
| self.width = width; | ||
| self.height = None; | ||
| self.scroll = self.get_scroll_for_resize(); |
There was a problem hiding this comment.
Setting scroll is moved to get_height().
|
Hey @trevarj -- great work, this works really well now! I've been using and testing recently, and haven't found any problems so far. I'll review the code soon. Thanks! |
|
Hey @trevarj -- I took another look at the code, and I've been also using it lately, it looks and works great! If you could squash your commits and rebase I'm going to merge this now. |
Closes osa1#101 Update tests: - Improve error reporting on test failure - Use smaller screen in text_field_wrap tests, document screen size - Combine multiple text_field_wrap tests in single test (not complete) Fix a few clippy warnings tui: Implement methods for testing resizing Allow servers to have aliases. Fixes osa1#186. Remove outdated contributors list from README We don't update it, and Github maintains an up-to-date list for us anyway (in 'contributors' page). Test resizing Fixed test after rebase with master Added word wrapping on text field input Fix for line calculation. Added indentation for message lines Fixed bug with Line::rendered_height not calculating based on timestamp and nick length Implement wrapping in text field Closes osa1#101 Update tests: - Improve error reporting on test failure - Use smaller screen in text_field_wrap tests, document screen size - Combine multiple text_field_wrap tests in single test (not complete) Test resizing Formatting Fixed test after rebase with master Added word wrapping on text field input Fix for line calculation. Added indentation for message lines Fixed bug with Line::rendered_height not calculating based on timestamp and nick length - moved text_field to own module - input_area - started making TextArea work like MsgArea, new InputLine like Line Working more on wrapping...autocomplete still buggy buffer remove fix. Progress on wrapping, added some tests - need more Autocompletion - hacky solution Not resizing msg_area every draw - only on height change Caching textarea height so that we only need to calculate lines on modify() or resize() Fixes after my bad rebase Fixed nickname not showing on tiny window. Fixed PR feedback: - Nickname suffix is now static - removed pub from some functions Fixes for PR: - Removes text_field_wrap config. Defaulting to text wrapping, falling back to scroll when width is too small. - Reverted indenting for message area. Fixed issue with calculate_lines() where it would not correctly calculate when there were multi-line words. Another rework. Exhaustive wrapping algorithm. Removed keeping track of white spaces. Added keeping track of line starts for easy wrapped drawing. No more crazy BTree insert/remove code. Added fallback to scroll after input area is greater than half of the screen height.
69867ac to
dfd080d
Compare
|
@osa1 Hey, sorry for not seeing your message sooner. I rebased from master, squashed, and forced a push. Not sure why the commit message is so long...it should have only been one line. Thanks! |
|
Thanks! |
Closes #101
See #183 for conversation history.