Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vertical scroll #378

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Vertical scroll #378

wants to merge 3 commits into from

Conversation

tailhook
Copy link
Contributor

@tailhook tailhook commented May 7, 2020

This PR implements vertical scroll in case input doesn't fit the screen.

This is based on top of #372 (so I'll rebase this PR when that one is ready).

This PR also includes d84c034 which unifies row/column measuring on unix and windows (because we use escape codes for colorful text on windows, and tab stops also work on windows, so it's basically the same)

The last commit also unifies what previously was "wrap_at_eol" thing. As it's not harmful to insert newlines after last column on unix terminals too. And it looks like it makes code and testing simpler.

This PR is tested on linux and windows.

@gwenn
Copy link
Collaborator

gwenn commented May 10, 2020

  • I don't have access to a windows 7 (<10) desktop anymore.
    If you do, could you please confirm that \t are (still) correctly handled.
  • And I need to figure out why wrap_at_eol was removed.

@tailhook
Copy link
Contributor Author

tailhook commented May 12, 2020

* I don't have access to a windows 7 (<10) desktop anymore.
  If you do, could you please confirm that `\t` are (still) correctly handled.

I don't have too. Since windows 7 end of life has been reached on 14 January 2020, should we still be concerned about it? (and since the issue here is relatively minor).

There are threads on tab size that predate windows 10, so it probably works anyway.

* And I need to figure out why `wrap_at_eol` was removed.

We are always in wrap_at_eol mode now. It works fine on unixes too.

@gwenn
Copy link
Collaborator

gwenn commented May 12, 2020

We are always in wrap_at_eol mode now. It works fine on unixes too.

I mean wrap_at_eol was introduced because of a bug on Windows 10 and is ugly.
So wrap_at_eol is useless on Unix and may be removed once the bug is fixed...

@tailhook
Copy link
Contributor Author

I mean wrap_at_eol was introduced because of a bug on Windows 10 and is ugly.
So wrap_at_eol is useless on Unix and may be removed once the bug is fixed...

  1. I don't think we can get rid of support of old versions of windows soon enough
  2. It's not that easy to move that logic into tty/windows part without duplicating too much code. I can move it to some kind of if cfg(windows) if you wish, but it still complicates the logic and tests (I've started from that).

@tailhook
Copy link
Contributor Author

We are always in wrap_at_eol mode now. It works fine on unixes too.

I mean wrap_at_eol was introduced because of a bug on Windows 10 and is ugly.
So wrap_at_eol is useless on Unix and may be removed once the bug is fixed...

Also note: #396

In the case terminal size is uninitialized, when we force inserting newlines we will get nicely formatted 80 chars column (assuming real terminal is >= 80). Otherwise the output will be a little bit messy.

@gwenn
Copy link
Collaborator

gwenn commented May 26, 2020

We are always in wrap_at_eol mode now. It works fine on unixes too.

I mean wrap_at_eol was introduced because of a bug on Windows 10 and is ugly.
So wrap_at_eol is useless on Unix and may be removed once the bug is fixed...

Also note: #396

In the case terminal size is uninitialized, when we force inserting newlines we will get nicely formatted 80 chars column (assuming real terminal is >= 80). Otherwise the output will be a little bit messy.

But we will wrap uselessly, no ?

@tailhook
Copy link
Contributor Author

We are always in wrap_at_eol mode now. It works fine on unixes too.

I mean wrap_at_eol was introduced because of a bug on Windows 10 and is ugly.
So wrap_at_eol is useless on Unix and may be removed once the bug is fixed...

Also note: #396
In the case terminal size is uninitialized, when we force inserting newlines we will get nicely formatted 80 chars column (assuming real terminal is >= 80). Otherwise the output will be a little bit messy.

But we will wrap uselessly, no ?

No, in case we wrap, we'll get something like:

------------------------------ the physical terminal ends here --v
--- our imaginary 80 columns --v
prompt> xxxxxxxxxxxxxxxxxxxxxxxx                                 |
yyyyyyyyyyyyyyyy                                                 |
------ cursor --^

In case we don't wrap, rustyline will think the cursor is there on the second line, while the actual picture is this:

------------------------------ the physical terminal ends here --v
--- our imaginary 80 columns --v
prompt> xxxxxxxxxxxxxxxxxxxxxxxxyyyyyyyyyyyyyyyy                 |
-- but physically cursor is on the first line --^

So on every refresh rustyline would clean two lines and so input will gradually move towards the top of the screen.


By the way, this also help in testing. As I've recently discovered, you can excute stty columns 80 and see how your application behaves on 80 columns. This also works better with manually wrapping at eol.

@gwenn
Copy link
Collaborator

gwenn commented May 29, 2020

Maybe we should remember that the size cannot be trusted and wrap manually only in this case ?

@tailhook
Copy link
Contributor Author

tailhook commented Jun 1, 2020

Maybe we should remember that the size cannot be trusted and wrap manually only in this case ?

Yes. But this sounds like another branch (e.g. if stament) of code that has to be accounted for and tested. It works just as well when we always wrap. What downsides of this approach do you see? Code is simpler with always wrapping. I don't think putting a single byte per line on the wire has any noticeable performance differences even over a remote connection.

@gwenn
Copy link
Collaborator

gwenn commented Jun 1, 2020

Code is simpler with always wrapping.

Code is simpler if we don't perform wrapping when terminal already handles it.

tailhook added 2 commits July 9, 2020 15:18
This is a reimplementation of kkawakam#338 which was reverted.

When `Highligher::has_continuation_prompt` returns true, rustyline
enables a special mode that passes the original prompt for every line to
the highlighter, and highlighter is free to change color or characters
of the prompt as long as it's length is the same (usual contract for the
highlighter)

Note:
1. Wrapped lines are not prefixed by prompt (this may be fixed in future
   releases either by prepending them, or by collapsing and scrolling
   lines wider than terminal).
2. Unlike kkawakam#338 this PR doesn't change the meaning of `cursor` and `end`
   fields of the `Layout` structure.
Previously the differences where:
1. Escape codes were skipped on windows
2. Tab stops weren't accounted on windows

Actually windows supports both tab stops and escape codes (windows >= 10) so
there is no good reason to keep the code different.
@tailhook
Copy link
Contributor Author

Note to myself: have to retest it on windows after rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants