Backend support (in flush) for newline : false

[hoodmane]

The current behavior of term.echo(str, {newline : false}) is pretty buggy. For example: term.error(str, {newline : false}) doesn't produce red text, it interferes with term.get_prompt which will show the partially echoed line of text and term.set_prompt which won't work correctly at all, it seems to be incompatible with a bunch of the other options (I don't expect finalize to work). The ENTER handler it provides also causes weird effect.

The problem is that the text echoed with newline : false is handled in a hacked-on extension. In order to get the behavior to be consistent and to make newline : false interact correctly with other formatting, the feature should be implemented in the core rendering engine and not as an add-on.

This PR moves the implementation of the newline option into the main rendering engine. The main idea is to output the partial line as normal but whenever the output ends in a partial line of text, update the style for the command line to move it up and the first line of it over to position the input field to the right of the final partial output field.

[coveralls]

Coverage decreased (-0.07%) to 82.658% when pulling ff142af on hoodmane:echo-newline-2 into a52aeae on jcubic:devel.

[jcubic]

Thanks for the PR, I will need to take a look little big closer how this work. And how it will behave with line wrapping, probably monkey patch in echo_newline.js also didn't handled that properly. The coverage dropped, maybe it would be good idea to also write unit tests for flush, that is missing coverage.

[jcubic]

It will not work in IE, but I think that this not a big deal. This only need to be documented. that echo newline don't work with IE.

[hoodmane]

line wrapping, probably monkey patch in echo_newline.js also didn't handled that properly

Surprisingly, the old method handles line wrap just fine. Apparently it's okay to set really long prompts:

Line wrap is handled fine with this approach too:

[hoodmane]

The coverage dropped, maybe it would be good idea to also write unit tests for flush, that is missing coverage.

Actually flush isn't missing coverage: most of the lines are covered 1000+ times and the only part missing coverage is the catch clause. Looking at the coverage report, every line that I added is covered a large number of times -- I was adjusting very hot functions. I don't really understand why the coverage went down:

[jcubic]

Few more issues I've found.

• Removing background from prompt is not good idea, because it need to work on it's own outside of terminal.
  This is better approach:

.terminal .partial {
    position: relative;
    z-index: 400;
}
.terminal div.partial > div {
    width: -moz-fit-content !important;
    width: -webkit-fit-content  !important;
    width: fit-content  !important;
}

While you modify CSS please modify -src.css file and always put .terminal or .cmd prefix before any selectors.

• Please update the JS file and replace mixed double and single quotes with just single quotes. I should write about this in contribution guide I didn't use ESLint rules because sometimes is better ot use double quotes (e.g. when you write error message and put single quote inside like don't word).
• If you can please use snake case for variables (e.g. lastRow should be last_row).

I don't have any other issues, good job, thanks for the PR

PS: I didn't added the code into the library itself because it needed to be monkey patch, but I didn't know about content-fit value.

[hoodmane]

Removing background from prompt is not good idea... better approach:
z-index: 400

That is better!

I didn't know about content-fit value.

I'm really confused about why this is necessary. I don't understand the behavior of the measured height and width of elements very well at all, I always just get things to work by trial and error. In particular, do you know why width='100%' changes the height of the empty lines? I understand that you are putting a before:: with a zero with space, but why is width='100%' needed? Weirder still, it doesn't work if it's 99% or 101% and it only works as an inline style. Also, why isn't the div sized to fit the content by default?!
/rant

[hoodmane]

Wait:

.terminal .partial {
    position: relative;
    z-index: 400;
}
.terminal div.partial > div {
    width: fit-content  !important;
}

Does this screw up the height of empty lines? Also, does !important override inline styles? I don't think this will work.

[hoodmane]

Oh I get it, you only apply the fit-content to the .partial div and I don't think the .partial one needs width : 100%. Only thing is to double check that term.echo("\n",{newline : false}) works but that should be okay.

[hoodmane]

Yeah this is broken when newline : false is used with a trailing newline. But I can just detect that, remove the trailing newline, and set newline to true.

[hoodmane]

@jcubic Okay I think I addressed your comments?

I tried to ensure that exactly one output div appears with each 'data-index' value. Currently consecutive calls to echo with newline : false get batched together with one call with newline : true (but also if newline : false and the string ends in \n then we remove the trailing \n and set newline : true). This makes the behavior of the update function a bit unpredictable in how many lines of text it will replace, but term.update was already a bit odd.

[jcubic]

I'm thinking about all those inline styles you're adding, there is issue to make jQuery Terminal work with strict CSP maily inline styles. I'm planing on removing the inline styles at all. I'm wondering if it will be possible to change your code into <style></style> even with current code it will be difficult, adding more inline style don't make things simpler.

Maybe there is better way then this, it seems that you just move the cmd to put above last line (didn't notice that first time when check the code), I think this is worse then modifying the prompt. Also I don't like exposing hidden __set_prompt_margin function.

This is why I didn't wanted to put that code into the terminal because your solution is also one big hack, like old echo_newline.js. I didn't wanted to put such hacks into the code.

Usually when I want echo without the newline I don't use echo_newline.js I use buffer defined in my own code outside of jQuery Terminal. Usually I don't want to left the code before the cursor like in Bash I want to add implicit newline then.
You can see how this work in my lisp interpreter https://lips.js.org/beta.html
Type

(display "x") (display "y") (newline)

and

(display "x") (display "y")

both work the same because there is implicit newline after the flush (flush in my code not in jQuery Terminal).

I think this is better then broken command line like in bash when someone don't echo newline at the end.
Here is the code for my terminal app https://github.com/jcubic/lips/blob/master/lib/js/terminal.js

[hoodmane]

Does that approach work with sleep? I'm concerned about something like
(display "x") (sleep 10) (display "y")
if you buffer the output then the first x won't be displayed while sleeping. A lot of existing Python programs with interactive code (say using input) rely on the normal behavior of the console in order to work correctly, in particular they rely on partial lines being displayed. input("Enter your name: ") in python does effectively:

print("Enter your name: ", end="")
sys.stdin.readline()

and in order to make code like this behave correctly it's helpful for the terminal to behave similar to a native terminal.

[hoodmane]

I'm wondering if it will be possible to change your code into <style></style> even with current code it will be difficult, adding more inline style don't make things simpler.

I think I can update it to remove the inline styles, except for the width : 100% hack which was in the original code and I have no replacement for it. If this could work without inline styles, would you consider including it? I think this is a huge improvement over hacking the prompt because:

1. it supports normal styling and options (with the prompt hack, newline : false breaks the flush option, the finalize option, and others, term.error("some error", {newline : false}) is not displayed in red).
2. it breaks get_prompt and set_prompt
3. it interacts badly with term.push, term.pop, term.read, term.login... basically everything

[jcubic]

I will think about this, you did lot of great work. I will test how this works with my Scheme REPL in Scheme that give me problems with old echo_newline.js

[jcubic]

Hi, I decided that I will merge your code. I will need to check why the drop of 0.07 in coverage though. It's getting close to 82 after rounding.

I will yet again documentation issue to document that newline: false feature is experimental.

[hoodmane]

Would you like me to try to modify to remove the added inline styles?

[jcubic]

You can try but I don't think it's really possible with current approach. You need dynamic top and left to move the cmd. I was thinking about CSP fix with dynamic <style> tag, but also I will need to check if it will work with strict CSP.

I've found one issue with your solution. If you echo without newline you can't select text of the prompt. The problem is that div with partial class is on top of the cmd.

This is the fix:

.terminal div.partial > div, .terminal div.partial {
    width: 100% !important;
    width: -moz-fit-content !important;
    width: -webkit-fit-content !important;
    width: fit-content !important;
}

[jcubic]

I will also need to check how this will work on mobile phone.

[jcubic]

Another bug is in copy/paste terminal is hijacking the right click over hidden textarea to allow pasting of text. This don't work anymore if you echo without newline.

[hoodmane]

You can try but I don't think it's really possible with current approach

Well if you had a css variable --line-height you could do

.cmd.shift_up {
    top : calc(-var(--line-height));
}

and then add or remove the .shift_up class from the .cmd div. Though this would screw up if the partial line contained an unusually tall unicode character. For the width you could add an extra <span> to the left of the command prompt with:

.cmd .prompt-margin {
    display: inline-block;
    user-select : none;
    visibility : hidden;
    width : fit-content; /* plus vendor-versions */
}

and set the text in the span like promptMarginSpan.innerText = partialLineInnerText. Then instead of measuring the width of the original .partial element, we could measure the .prompt-margin width and then we wouldn't have to add / remove the .partial width.

[hoodmane]

I think there is also a bug if the prompt is empty.

[hoodmane]

Any update on this?

[jcubic]

Sorry didn't have time to test locally how this works. Will merge into experimental branch, since this is not 100% ready.

[jcubic]

Will check how this work and maybe fix some issues, maybe tomorrow will find some time.

[hoodmane]

Thanks!

[jcubic]

It works great, thanks for working on this. I've added minor change to get rid of 0.01px border, I've added zero space width prompt when it's empty.

[jcubic]

About copy/paste it's also broken on devel branch.

[jcubic]

New version 2.23.0 with the feature was just released.

[hoodmane]

Thanks! I'm really glad this worked out.

[jcubic]

Add one fix to your hack to force reflow in Firefox, it was not shoring the cursor in one of my projects, I've set the timeout to 0 but move the showing timeout into the first timeout.

PS: what tools did you use to show the coverage? I've missed that first time when you added that screenshot.