Clear to end of each line in left prompt

[soumya92]

Description

Changes the left prompt rendering to include clr_eol for each rendered line. This prevents issues where if a multiline prompt changes between renderings, the previous prompt shows through.

Fixes #6532

TODOs:

• Changes to fish usage are reflected in user documentation/manpages.
• Tests have been added for regressions fixed
• User-visible changes noted in CHANGELOG.rst

[mqudsi]

Thanks for the initiative and the patch.

Writing a character at a time is extremely suboptimal. I'd rather split on new lines and then use s_write_str, even with the extra allocations for each line rather than do it this way. Better yet would be to only do so if the prompt contains a new line in the first place. As I understand it, this fixes two different problems: #6532 which deals with resized terminal windows, and a multiline issue? Or is the multiline issue only in the event of a resized terminal window? If it's the latter, then we can predicate this whole business on having received a SIGWINCH and avoid the slowdown pretty much all the time.

[soumya92]

fish-shell/src/screen.cpp

Line 735 in 45d60f6

if (left_prompt != scr->actual_left_prompt) {

should cover all cases of rendering. AIUI, if we're here, then the prompt is different, which means we could run into the issue that the previous prompt had some lines that were longer and now we need to clear them.

Keeping track of which lines we need to clear is unlikely to be worth the increased complexity, so clearing all lines makes sense. I did not realise that writech would be so expensive. I assumed an internal buffer. I think we can easily take "views" into the string by jumping from one \n to the next, and write those in one call. I will update this PR shortly.

[soumya92]

Okay, updated to write each line at a time. I'm not sure if c_str() causes an allocation, but I think we can live with that even if it does, since this shouldn't be running too frequently (given the if check already in place).

[mqudsi]

Thanks for updating the patch. The .c_str() call wouldn't allocate, but the .substr() call would. With C++14, .substr() could return a string_view (directly?) which doesn't, but calling .c_str() afterwards necessarily would since it must be both contiguous and null-terminated.

[mqudsi]

I would move this to after the last write to minimize flashes of contents/repaints. But first - in what case is it required, given that it would presumably apply to even single-line prompts but we don't have any problems with those now, even without it?

[soumya92]

That's a good idea. I will make that change. It might make the logic slightly more complicated, since currently it goes "clr, line+terminator", which will need to be changed into "terminator+line, clr". Just some indexing stuff to take care of :)

I think the fact that it's being printed on a single line prompt is also just an artefact of that, so I can definitely eliminate it. We don't have a problem for single line prompts because rendering the command also clears to the end of the line.

[soumya92]

Updated, let me know if you have any suggestions to make the logic clearer.

[krobelus]

We can avoid allocation in the common case by using left_prompt.c_str() + prev_nl.

We could also avoid allocations from the substr above, if we temporarily swap out newlines for null bytes.

        while ((next_nl = left_prompt.find('\n', start)) != wcstring::npos) {
	        left_prompt.at(next_nl) = L'\0';
            s_write_str(scr, left_prompt.c_str() + prev_nl);
	        left_prompt.at(next_nl) = L'\n';
	        ...
        }

However, I don't think swapping null bytes is common in the fish code base, so when in doubt I'd leave that part as is. What do others think?

[soumya92]

Done for the common case. Agreed that swapping '\n' with '\0' seems dubious. If we're going that far, we might as well make left_prompt a wcstring_list_t instead of a single wcstring. WDYT?

[soumya92]

I had to change the logic a little bit because I realised that my first attempt would not handle '\n' inside escape sequences properly.

[ridiculousfish]

Merged as 80aaae5, thank you!

[soumya92]

Happy to help! I think there's still room to add the wcstring_list_t change as well, so I'll probably come back to it later.