laravel / prompts

Beautiful and user-friendly forms for your command-line PHP applications.
https://laravel.com/docs/prompts
MIT License
533 stars 94 forks source link

Add Component: Multiline Text Input #88

Closed joetannenbaum closed 7 months ago

joetannenbaum commented 1 year ago

This PR introduces a multiline text input called textarea. It's parameters and behavior are largely identical to the text prompt, with the added parameter of $rows to specify how tall the input should be.

https://github.com/laravel/prompts/assets/2702148/ae3a3c49-d576-4471-a152-9ceea9c2b2f1

I really tried to refine this and make it as ready as possible, but I have a couple of "magic numbers" in here that may need more context from @jessarcher to properly implement.

Let me know what you think!

jessarcher commented 1 year ago

Wow! This is wild!

I'll check this out soon. A few initial questions:

joetannenbaum commented 1 year ago

As far as I could tell, I wasn't able to detect Shift+Enter. I really wanted to get away from the Ctrl+D convention as I find that really unnatural, but I couldn't figure out another detectable key combo that wouldn't potentially be typed in the prompt itself. But if you have any ideas I'm all ears.

I initially went with the auto grow scenario (starting with a minimum height of several rows to indicate that it accepted multiline input). Went back and forth on it, I found it to be a little unwieldy when it started getting close to the height of the terminal, so I went with the scrollbar.

Maybe there's a middle ground there, we let it auto grow, but the max height is something like half of the terminal height (or some other calculation)?

jessarcher commented 1 year ago

Hey @joetannenbaum!

I'm sorry for not getting back to you sooner on this one.

I've just merged in the latest changes from main and updated the scrolling trait that changed in #79.

I also fixed a minor issue when trying to move back to the last line when it's empty.

joetannenbaum commented 1 year ago

@jessarcher I removed the two "magic numbers" I had and replaced them with proper solutions.

Let me know what you think about how I altered the addCursor method in TypedValue. I made it so that if you pass in a negative value such for the $maxWidth argument, it avoided truncating the text altogether.

Felt ok to me as the alternatives would have been to either pass an additional boolean argument or to create another method for getting the value with a cursor sans truncation, which felt like too much duplication.

Definitely open to suggestions if you're not a fan of the approach.

I also fixed the "cancelled" state of the textarea as the dim + strikethrough styling was leaking into the box lines itself.

ClaraLeigh commented 12 months ago

How are we progressing on this PR? I was needing a textarea field for a project and would love to start using this

jessarcher commented 12 months ago

Hey @ClaraLeigh - things have been pretty hectic with Pulse and Laracon, but I'll be circling back to new Prompts features soon!

jessarcher commented 10 months ago

Finally revisiting this!

I've found two obscure issues. I'll try to fix them but documenting them here also:

  1. When writing a long line with no spaces and it wraps, the virtual cursor position on the following line is one position to the left. If you continue typing it until it wraps again, the cursor will continue moving to the left for each wrapped line:

    image

  2. When writing enough text that it scrolls, when you get to the end of the next line, a weird color inversion occurs:

    image

    I believe this is caused by the virtual cursor being trimmed before it can restore the inversion. When dumping the raw output via Ray, you can see on the second last line that it outputs a [7m (invert) but only a [27 without the trailing m, which would undo the inversion.

    image

    If you continue typing onto the next line, it gets more messed up, presumably because the inversion is still "active" when it redraws the whole prompt:

    image

These issues may be related as both seem like an "off by one" error.

I also think that the box should expand to the edge of the cursor before it wraps, similar to how the other prompts work. That's an easy change, but it doesn't seem to fix the above issue.

joetannenbaum commented 10 months ago

@jessarcher great catches! Thank you.

I believe I fixed the cursor position for longer words. When the user types longer words, the typedValue falls out of sync with the display, since wordwrap injects a line break mid-word. I added a method that looks for longer words that wrap to calculate an offset that we take into account when passing the cursorPosition into valueWithCursor.

I'll add some tests later to triple verify, but on my end it looks good. I'll take a peek at that weird inversion behavior later as well.

jessarcher commented 10 months ago

Thanks @joetannenbaum,

There's another issue - PHP's wordwrap function isn't multi-byte safe (it will split in the middle of multi-byte characters), and there's no mb_ equivalent. It also uses length, rather than width, so even if it were multi-byte safe, lines containing full-width characters (such as emojis) would be too wide. I've tried creating an appropriate function using a combination of mb_strwidth and mb_strimwidth, but there are a lot of edge cases.

jessarcher commented 10 months ago

I played around creating a multi-byte safe wordwrap function that uses width instead of length and got something that seems to work. I haven't tried putting it all back together with the virtual cursor yet though.

Here's the code as global functions in case it helps:

function mb_wordwrap(string $string, int $width = 75): string
{
    $pieces = array_reduce(
        preg_split('/\b|(\n)/', $string, flags: PREG_SPLIT_DELIM_CAPTURE|PREG_SPLIT_NO_EMPTY),
        fn ($carry, $chunk) => [...$carry, ...mb_str_split_width($chunk, $width)],
        []
    );

    $output = [];
    $line = '';

    foreach ($pieces as $piece) {
        if ($piece === PHP_EOL) {
            $output[] = rtrim($line);
            $line = '';
        } elseif (mb_strwidth(rtrim($line.$piece)) <= $width) {
            $line .= $piece;
        } else {
            $output[] = rtrim($line);
            $line = $piece;
        }
    }

    if ($line !== '') {
        $output[] = rtrim($line);
    }

    return implode(PHP_EOL, $output);
}

function mb_str_split_width(string $string, int $width): array
{
    $chunks = [];

    while (mb_strwidth($chunk = mb_strimwidth($string, $start ??= 0, $width)) !== 0) {
        $chunks[] = $chunk;
        $start += mb_strlen($chunk);
    }

    return $chunks;
}

I've mimicked the behaviour of a real <textarea> where white space at the end of the line doesn't cause a wrap until non-blank character is typed, but I think the rtrim will cause issues navigating with the cursor. The behaviour may need to change to better work with a terminal.

Also, the length can change quite drastically (and could even get shorter due to the trimming), so the only way I can think to place the virtual cursor would be to add a placeholder before wrapping and then replace it afterwards, but the placeholder would need to be the same width to not mess up the calculations. I'm not sure how to handle moving up and down either.

joetannenbaum commented 10 months ago

Ok, interesting. I took a different approach, where I just tried to match the behavior of wordwrap and then make it as multi-byte safe as possible. I'm sure there are edge cases I'm missing but this was my stab at it:

https://github.com/joetannenbaum/multi-byte-wordwrap/blob/main/src/multi-byte-wordwrap.php

Also, I'm wondering: Do we want to cap the width at a more comfortable reading width instead of expanding the full width of the terminal? Something around 70-75 characters? I think expanding to the entire width makes sense for a single input, but for multi-line it may get a little unwieldy? Just a thought.

https://baymard.com/blog/line-length-readability

joetannenbaum commented 10 months ago

Ok, I got this a bit further along. It feels pretty consistent now, and from what I can tell those edge cases have been cleaned up. I went with my mb_wordwrap implementation just for now, if you'd like to use yours I'm totally down, we'll just have to re-work the cursor stuff a bit.

Let me know what you think, I think it's feeling pretty good overall.

joetannenbaum commented 9 months ago

Sorry for the massive delay in working on this, finally got a moment tonight.

I think I figured out the weird bug with the up arrow cursor, there was actually a correlating bug with the down arrow as well. Looks like I may have reversed some logic at this point, it looks good to me now.

I also fixed a bug that I encountered where the cursor wasn't in the scrolling viewport anymore when pasting large chunks of text that spanned multiple lines.

Overall in playing with it tonight I think it's looking pretty good!

driesvints commented 8 months ago

@joetannenbaum tests are failing here. Are you still working on this one?

joetannenbaum commented 8 months ago

Yes, this is still active. I'll take a look at what's failing today and fix 👍

driesvints commented 8 months ago

@joetannenbaum tests still fail here.

joetannenbaum commented 8 months ago

@driesvints I'm so sorry, I swear I got it green yesterday before I marked it as ready for review. Should be good now.

@jessarcher heads up: I altered the Prompt class to allow passing null into cancelUsing so that it could be reset between tests. Running the TextareaPromptTest test in isolation worked, but when I ran all of the tests together the TextPromptTest was setting up a custom cancellation callback that was bleeding into the TextareaPromptTest test.

Let me know if you'd like to handle that another way, happy to adjust.

taylorotwell commented 8 months ago

Just waiting on final thumbs up from @jessarcher here.

joetannenbaum commented 7 months ago

@jessarcher we should be looking better here, let me know if you come across any other oddities.

I ended up moving the mb_wordwrap function to the Truncates trait, felt like it might be right?

I also moved the $rows argument to the last position so that the signature basically matched the signature of text, let me know if that's cool.

jessarcher commented 7 months ago

Looks great @joetannenbaum! I made a few formatting tweaks to the code and also added support for the placeholder to wrap and/or contain newlines.

taylorotwell commented 7 months ago

Thanks!