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

When using page as scrollback_pager, if the last character on a line has ANSI formatting, the formatting continues onto following lines #1924

Closed
s1341 opened this issue Aug 25, 2019 · 26 comments

Comments

@s1341
Copy link
Contributor

s1341 commented Aug 25, 2019

When using page (https://github.com/I60R/page) as the scollback_pager, I encounter the following issue:

If the last character on the line has ANSI styling/colors/background, when switching into scrollback_pager mode, the styling continues onto the following lines, until another explicit ANSI styling is encountered.

For example, see the following screenshot (python):
2019-08-25-121312_269x61_scrot

When switching to scrollback_pager mode, we get the following:
2019-08-25-121333_270x52_scrot

Note that the prompt on the line following the word 'start' is in red. It should not be!

I think that this issue is a result of the fact that we retrieve history/scrollback with ANSI formatting on a line by line basis, and the code at https://github.com/kovidgoyal/kitty/blob/master/kitty/line.c#L284 checks whether a new formatting (SGR) is required to be written to the stream. But that check only works within the line, not between consecutive lines. I'm not sure how to fix this... If you give me some guidance I can try for a PR to fix.

@kovidgoyal
Copy link
Owner

Simplest way would be to have line_as_ansi take a pointer to a prev cell, which can be NULL. Have the various call sites to that function do the appropriate thing.

s1341 added a commit to s1341/kitty that referenced this issue Aug 25, 2019
s1341 added a commit to s1341/kitty that referenced this issue Aug 25, 2019
@s1341
Copy link
Contributor Author

s1341 commented Aug 26, 2019

This is still not working as expected. See the below screenshot:

2019-08-26-094114_676x450_scrot

I can't figure out why sometimes the next line is still highlighted (see grouping 3), or why we have the remainder of the line highlighted (as in grouping 4 and 5). Can you point me in the right direction for a fix?

@kovidgoyal
Copy link
Owner

Use less and you should be fine.

@s1341
Copy link
Contributor Author

s1341 commented Aug 26, 2019

Yeah. But I'd like to properly resolve the bug... Do you have suggestions as to how to debug/fix?

@kovidgoyal
Copy link
Owner

There is no bug in kitty as far as I can see. You can confirm that by
writing a simple script that dumps stdin to a temp file and then passes
it to the pager. Set that script as your pager. Then trigger the problem
and inspect the temp file using a hex editor and see if the output is as
expected. Or simply cat the temp file in a new terminal and see how it
displays.

@s1341
Copy link
Contributor Author

s1341 commented Aug 26, 2019

Ok. I believe I just confirmed that it IS a bug in kitty. Here's what I did:

As you suggested, I created a simple script:

import sys
import select

f = open("/tmp/txt", "wb")
while True:
    if select.select([sys.__stdin__.buffer], [], [], 0) == ([sys.__stdin__.buffer], [], []):
        text = sys.__stdin__.buffer.read()
        sys.__stdout__.buffer.write(text)
        sys.__stdout__.buffer.flush()
        f.write(text)
        f.flush()

I then set that script to be my scrollback_pager.

The result (/tmp/txt) of running some python to create green background lines is this:
2019-08-26-132251_650x945_scrot

You can clearly see that the 'world' line in each cluster has a green background, even though it shouldn't according to the ANSI string printed.

The above is without my patch for this PR. With it, I get:
2019-08-26-132621_650x805_scrot

You can see that the 6th cluster is still exhibiting the problem.

What do you think?

@kovidgoyal
Copy link
Owner

Your patch has already been merged, I meant there is no longer a bug,
after that patch. You seem to be still having a bug in one random case,
I dont really see how that's possible, but recreate the bug, and
use --dump-bytes to create a dump of the input used to create the bug
and attach that and I will have a look.

@s1341
Copy link
Contributor Author

s1341 commented Aug 26, 2019

Attached the dump-bytes output.
bytes.log

@kovidgoyal
Copy link
Owner

Those bytes already show the issue you mention when catted. I need the
bytes from before you run the pager. i.e. create the input which would
have caused then issue when the pager is run and post that, without
actually running the pager.

@s1341
Copy link
Contributor Author

s1341 commented Aug 26, 2019

Oh sorry. Here you go.
bytes.log

@kovidgoyal
Copy link
Owner

bc222af

@s1341
Copy link
Contributor Author

s1341 commented Aug 28, 2019

That appears to have fixed it. Thanks a lot!

@s1341
Copy link
Contributor Author

s1341 commented Sep 1, 2019

Ok. Still experiencing one artifact. The following:

2019-09-01-134219_751x585_scrot

Renders like this:
2019-09-01-134235_741x526_scrot

I've attached the dump-bytes output for this kind of issue.
dump2.log

@kovidgoyal
Copy link
Owner

that's a bug in the pager. I cannot replicate using the following
steps:

  1. Run: kitty -o scrollback_pager=/t/filter sh -c "cat /t/dump2.log; read"
  2. Run the pager, which writes the data is received to /tmp/txt
  3. Run: kitty sh -c "cat /tmp/txt; read"

Output does not show artifact

@s1341
Copy link
Contributor Author

s1341 commented Sep 1, 2019

Ok. I'll open a bug for this on the pager github. Thanks for your help running this down.

@s1341
Copy link
Contributor Author

s1341 commented Sep 1, 2019

Ok. So I discussed with some of the people over in #neovim on freenode. They helped clarify some things.

It seems that if I do something similar to what you did above, with the following 'filter':

#!/usr/bin/env python
import sys
import select

f = open("/tmp/txt", "wb")
while True:
    if select.select([sys.__stdin__.buffer], [], [], 0) == ([sys.__stdin__.buffer], [], []):
        text = sys.__stdin__.buffer.read()
        sys.__stdout__.buffer.write(text)
        sys.__stdout__.buffer.flush()
        f.write(text)
        f.flush()

It seems that the output I get is NOT exactly what is expected. Examining the output with cat -e shows the following for one of the groupings:

^[[38:5:28mIn [^[[39m15^[[38:5:28m]: ^[[38:5:83mprint^[[39m(u"\u001b[42mhello\u001b[0m\nh\u001b[42mello\u001b[0m,world\nhi \u001b[42mthe\u001b[0mre")                                $
^[[42mhello$
^[[49mh^[[42mello^[[49m,world$
hi ^[[42mthe^[[49mre$
$

Note that kitty appears to be outputing SGR 49 instead of a 'clear formatting' (SGR 0). Perhaps this is the source of my issue?

<LeoNerd> No, 49 is background reset, 0 is overall reset; but the only thing in effect is background, so either is fine
14:46:44 ⇐ aeonzh1 quit (~aeonzh@2a01:4b00:82bb:4700:d929:7666:f62f:d433) Ping timeout: 252 seconds
14:46:47 <LeoNerd> The issue is the ordering - reset vs. linefeed
14:47:09 <LeoNerd> IF you reset *first* then the pen is back in default state when the linefeed happens, so it doesn't fill the background of the new line in this different colour
14:47:31 <LeoNerd> But if you linefeed while the background colour is still set, it will fill the cells of the newly-created line because of that scroll with that colour

Or perhaps the above is the issue?

@kovidgoyal
Copy link
Owner

49m means reset to default background color, https://en.wikipedia.org/wiki/ANSI_escape_code#SGR_parameters

the pager is presumably misinterpreting it. 0m means reset all
formatting.

@s1341
Copy link
Contributor Author

s1341 commented Sep 1, 2019

Yeah I understood that after I posted. Is it perhaps the ordering of the newline and the SGR 49, as suggested by LeoNerd.

@kovidgoyal
Copy link
Owner

A newline most definitely should not fill a line with the current background color. That is done only by terminals which support bce and indicate so in the terminfo files. kitty does not support bce and says so in its terminfo. The pager needs to respect that. https://invisible-island.net/ncurses/ncurses.faq.html#bce_mismatches

@kovidgoyal
Copy link
Owner

And #160 (comment)

@s1341
Copy link
Contributor Author

s1341 commented Sep 1, 2019

hrm. was a fix upstreamed in neovim for this issue?

@kovidgoyal
Copy link
Owner

Neovim improved its heuristics to detect bce support a long time ago. As of 3.0 I think.

@s1341
Copy link
Contributor Author

s1341 commented Sep 1, 2019

I'm running git master neovim, so I'm not sure why I'm still dealing with this issue... Maybe I need to update my libvte...

@kovidgoyal
Copy link
Owner

It's probable that this has nothing to do with nvim. The pager presumably reads and interprets ANSI contrl sequences and converts them into formatting commands for the nvim engine. The code to do thatis likely assuming bce without consulting terminfo.

@s1341
Copy link
Contributor Author

s1341 commented Sep 1, 2019

The pager I’m using, page, is just neovim’s built in :terminal. The ANSI parsing is done by libvterm. I updated the libvterm I used to build neovim and I have not observed the issue since. I’ll verify tomorrow.

@s1341
Copy link
Contributor Author

s1341 commented Sep 3, 2019

FYI neovim/neovim#10922

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

No branches or pull requests

2 participants