One thing that I couldn't figure out how to do is:
If you enter a ton of text to fill the whole screen, the msgarea gets so small and then the program crashes...

Awesome, thanks for the patch!

Great work overall. Before looking at the code in detail I tried a few things. It mostly looks good. Some comments below.

• If the text field grows vertically and then I delete things, the screen layout is not completely restored. In the screenshot below I typed something long and then deleted, and there's on line gap between the text field and in message area now.

• This feature should be optional, enabled with a line in the config.yml file. I think something like text_field_wrap: true would work. If it's not available or set to false then the feature is not enabled.

• On small screens we should fall back to the current behavior, which is more compact (always takes 1 row). When to fall back exactly? A few random ideas (feel free to suggest more):

  • If MsgArea is smaller than a fixed amount (e.g. 5 lines)
  • If MsgArea is smaller than the input field

Btw, have you seen the TUI tests? It'd be good to add some test about these, especially for resizing and rendering on small screens.

Thanks again!

Thanks for the feedback! I will try to implement these suggestions when I get some more time.

@osa1 Hey, I'm making some good progress, but I'm finding it difficult to add a configuration parameter...It doesn't look like there's a nice way to configure a setting on the TextField without passing down the config all the way from main.rs...Any ideas?

Also, I'm pretty overwhelmed with the test framework...I never really worked in testing a TUI before 😩

I wasn't able to reproduce the problem you're seeing and mentioned above in your screenshot. You can see from the video below that I don't have that issue on my terminal emulator...I may need to try some other terminals.
Here's what I have at the moment:

https://streamable.com/lxpt34

Hey @trevarj,

I'm finding it difficult to add a configuration parameter...It doesn't look like there's a nice way to configure a setting on the TextField without passing down the config all the way from main.rs...Any ideas?

TUI has its own config format and parser, see libtiny_tui/src/config.rs. You need to parse the config field there (just update the Config struct in that file with a new bool field) and use it in reload_config in libtiny_tui/src/tui.rs. It should be too hard. No need to make any changes in main.rs.

Also, I'm pretty overwhelmed with the test framework...I never really worked in testing a TUI before

TUI test library is really simple, have you seen some of the tests in libtiny_tui/src/tests/mod.rs? For example, this test in that file

#[test]
fn init_screen() {
    let mut tui = TUI::new_test(20, 4);
    tui.draw();

    #[rustfmt::skip]
    let screen =
        "|Any mentions to you |
         |will be listed here.|
         |                    |
         |mentions            |";
    expect_screen(screen, &tui, 20, 4);
}

creates a TUI and then compares the screen with the textual representation (screen) of it.

To update the TUI after it's initialized you can either send key press events using the handle_input_event method or add a method for adding stuff to the text field, and then use that.

I wasn't able to reproduce the problem you're seeing and mentioned above in your screenshot

Here's how to produce it:

• Make your screen small so that you won't have to enter long text.
• Move to a tab where the message area is full of messages. (there shouldn't be any empty lines in the incoming messages)
• Enter some long text so that the input field grows to multiple rows.
• Now delete the text. You should end up with some empty space above the input field, as shown in my screenshot above.

Let me know if you have any more questions!

Which terminal do you use? Below is me following the steps 😟
https://imgur.com/a/BaxT92o

Here's a demo:

I installed on linux and ran some testing...looks ok for me. Maybe you didn't pull the most recent changes?

There's currently one inconsistency in how we split long lines in messaging area vs. in text field. In messaging area we only split lines at word boundaries (at whitespaces). That works really well I think and renders nicely. I wonder if we could do the same for the text field.

I did notice then when I first started playing with the tui to make my change. It seemed a little difficult for me to extract out, especially with how the autocomplete stuff is right now...Maybe some day I can implement it though.

Could you investigate this?

Other than this bug I can't really see any problems. Great work!

Finished up all other suggestions. Just need to debug this bug and I will commit to the PR again!

Sorry for the delay, I've been busy with work and life. I'll try to take another look today.

@trevarj in my "extensive" testing of this PR I found a bug in rendering of ExitDialogue (unrelated to this patch), which I just fixed on master. Unfortunately it caused a merge conflict in this PR. Would you be able to fix the conflict? If not let me know and I'm going to fix it myself.

The change is simple: I removed draw_input_field method of MessagingUI and very slightly refactored (though indentation changed quite a bit) the draw method. We now don't show the nick when rendering ExitDialogue so it now takes full width.

Sorry again for the trouble..

@trevarj in my "extensive" testing of this PR I found a bug in rendering of ExitDialogue (unrelated to this patch), which I just fixed on master. Unfortunately it caused a merge conflict in this PR. Would you be able to fix the conflict? If not let me know and I'm going to fix it myself.

The change is simple: I removed draw_input_field method of MessagingUI and very slightly refactored (though indentation changed quite a bit) the draw method. We now don't show the nick when rendering ExitDialogue so it now takes full width.

Sorry again for the trouble..

No problem, I will take care of it.

Thanks!

Unfortunately because you did a merge rather than rebase I can't squash these commits now. Anyways I'll be rebasing in my staging branch.

Sorry! 😞 Do you want me to revert?

No need! Already rebased in textfield-overflow branch.

Sigh.. I keep finding bugs:

I'll give instructions for reproducing in a bit. Let's continue development and bug fixing in a branch that I can commit to. @trevarj would you like to give me commit access to your repo so that I can update your branch?

Should I close this PR since the changes are already in your repo?

We can close this and create a new PR from my branch to continue discussion. Or just continue here. I don't have a preference.

Let's continue from this branch as both of us have push access here. I'll have to give you push access to the main repo (to all branches, as far as I can see) if we want to collaborate on this patch using a branch on my repo.

Let's continue from this branch as both of us have push access here. I'll have to give you push access to the main repo (to all branches, as far as I can see) if we want to collaborate on this patch using a branch on my repo.

Ok, sounds good.

but I found it really difficult to draw the cursor in the correct place

Hmm, I'm afraid I don't see why this is difficult. Here's what I have in mind: when rendering the text field we already now the cursor position. Render it char-by-char while counting current char index (use enumerate()). If the current char index is the same as cursor index then render cursor. Otherwise render as usual.

I'll take a look at the code and see what the problem is.

also pre-determine the number of lines the TextField will have in MessageUI::draw()

I think this should be calculated when we call TextField::resize(). This is what we do for Lines in the messaging area, and the logic should be more-or-less the same, I think.

I've put up a PR over on my fork (here) with a working solution to the input field wrapping on words. It's working nicely as far as I can tell...even works with autocompletion.
I'm currently looking at the indentation for messages in the MsgArea to see if there's an easy solution. I need to study MsgArea and Line more to see if I can use the code I wrote for TextField.

@osa1 have you gotten a chance to play around with this latest build? I'm interested in your thoughts after using it for a bit.

I just installed the latest version. Let me use it for a bit, I'll update.

I've been using this for the last couple of days. Here's a screenshot of this patch in action: (text field is empty as that's the same as before, only the changes in the messaging area are visible in this SS)

The main issue is that this does opposite of what I suggested above (sorry if my wording was not clear enough), namely, it aligns everything, rather than not aligning user input. I think it doesn't look good even though it's consistent (both input field and messaging area are formatted the same way), but more importantly tiny is supposed to compact, and for that I'd much rather skip the alignment and start consecutive lines at column 0.

Only bug I could find is also visible in this SS, if you look at the line with the long link you'll see it's in a new line for no reason.

I'm busy with work and other stuff these days so I couldn't review this line by line, but here's a suggestion (feel free to ignore if you think it wouldn't work or would be too much work): implement something like Line that support these operations:

• O(n)-time rendering: it should be possible to simply scan it left-to-right and add one char at a time (or move on to the next line). For this you'll need to maintain indices of whitespace or maybe split the input into segments, as we do in Line.
• Should support adding/removing chars (for user input). It's fine if this does vector insertion in the middle or something like that, but adding a new character to the end (the most common operation) should be O(1) (this should be easily possible, I think).
• Should support pasting. It's fine if we create the entire line (with segments/whitespace indices etc.) from scratch when this happens.
• Unless I'm missing something, this type should not need a resize() method, similar to how we don't need resize() for Line.
• It should support starting at the given column when rendering the first line. This will be used to render nick before the input field. E.g. if the nick string is "osa1: " we'll do something like input_field.draw(tui, x_pos, y_pos, 5) where 5 indicates that in the first line it'll skip the first 5 columns (so the first character will be at x_pos + 5). However in the next line we should be rendering starting from pos_x, not pox_x + 5!

This code should live in its own module.

Once we have that it'll be trivial to replace the current TextField with this one: we'll simply replace the field in MessagingUI, and all the mess and important stuff to carefully implement and review will be in a single module.

Btw, I just realized that it's not possible to implement this feature without calling resize in some cases on user input (something that I wanted to avoid so far). The reason is this: suppose the text field is at its full width and I add one more character. Now the text field will grow one more line, and for that messaging area needs to shrink by one line, so we'll have to resize it. I think this is fine as there's no way around this.

One edge case here is what happens if I keep adding more characters and messaging area becomes too small. I think we should handle this by falling back to the scroll mode.

The actual resizing logic (when terminal size changes) should be the one I describe above.

I forgot to mention, thanks for your persistence and sorry that I've not been more responsive. One problem is I do code reviews at work, so that's not something I've been super keen on doing in my free time. But I'm actually keen on merging this! Hopefully we'll get there.

One problem is I do code reviews at work, so that's not something I've been super keen on doing in my free time.

It's not a problem. Thanks for guiding me through this. I will admit that I am a bit weary of doing a full re-factor because I'm worried I don't have a good idea of what you have in mind. It is a good Rust learning experience for me though, so I will try to attempt something.

Hi @osa1 ,
I'm working on this refactor over here: https://github.com/trevarj/tiny/tree/text-wrap-refactor
Right now it has the basic functionality done. Autocompletion is not working yet, and I haven't thoroughly tested everything.

Do you think you could take a look at it when you have time? I don't want to do too much and go off track of what you are envisioning.

High level overview:

• Migrated text_field.rs into its own module - input_area
  • mod.rs - contains struct TextField and all related functions
  • input_line.rs - contains a new struct InputLine which will replace TextField's buffer (similar to Line in msg_area
    • To track whitespaces/splits I decided to use a BTreeSet to make it slightly easier for inserting/removing, something that msg_area::Line doesn't need to do. I'd like to know your thoughts on the performance of that vs a Vec.
• Moved nickname to inside TextField to have it draw the nick instead of in MessagingUI

Thank you.

Hey @trevarj,

Just took a look -- the code looks great, thanks! It's easy to review the changes in messaging.rs as important bits are all in their own file now.

I found some bugs while testing but you said you haven't thoroughly tested everything so not reporting those now.

Moved nickname to inside TextField to have it draw the nick instead of in MessagingUI

This makes sense to me 👍

Regarding BTreeSet vs. Vec: I think nothing's going to beat Vec iteration performance, but looking at the code, it doesn't seem too hard to replace the BTreeSet with Vec if needed later. So use whatever's most convenient for you, we can replace it later if needed, using some benchmarks to guide the changes.

I think we could cache calculate_lines output. Invalidate it on resize() and when input changes, in all other cases it could return the previous calculated number.

It'd great if you could rebase your branch on master (not a merge!), that makes reviewing so much easier and it'll also make merging easier when the time comes.

Overall looks great! Thanks again!

New conversation here: #200