2.4b1 prints escape characters before and after every process #3499

Closed
zanchey opened this Issue Oct 27, 2016 · 59 comments

Projects

None yet

7 participants

@zanchey
Member
zanchey commented Oct 27, 2016

By private email, a tester reports:

Today I upgraded my fish shell to version 2.4b1.1 (using default apt-get upgrade). Apparently new version contains some unusual bug, every fish command is preceded by string [1 q and followed by [1 q, see attached screenshot.

Downgrading to 2.3.1 fixes my problem.
I encounter this problem regardless of my custom config used (I moved ~/.config/fish/config.fish to other directory, restarted shell, problem was still there).

screenshot from 2016-10-20 13_52_29

@faho
Member
faho commented Oct 27, 2016

That's from __fish_cursor_xterm - echo -en "\e[$shape q" with $shape = 2 (block).

$TERM is set to xterm(-256color), but the terminal doesn't seem to support this. Which is it?

@faho faho added this to the fish 2.4.0 milestone Oct 27, 2016
@ridiculousfish
Member

I can repro on CentOS5. Bisecting...

@faho
Member
faho commented Oct 27, 2016

@ridiculousfish: No need to bisect. It's gonna show the commit where I turned the vi-cursor on by default.

The question is how to detect this particular term and that it will bork this escape.

@ridiculousfish
Member

Bisects to cac3b23 as faho predicted

@ridiculousfish
Member

From my investigation, the cursor shape sequence is known as DECSCUSR and per Stéphane Chazelas "is supported since patch 282". We can detect this via $XTERM_VERSION.

$XTERM_VERSION has a value like XTerm(282) which is sort of annoying to check, but I think this is the right approach if we want this on by default. I verified in my repro case, the variable was XTerm(215).

@faho faho added a commit that closed this issue Oct 27, 2016
@faho faho Disable vi-cursor on xterm < 282
Fixes #3499.

@zanchey: This is for integration-2.4.0.
7ea2dc4
@faho faho closed this in 7ea2dc4 Oct 27, 2016
@faho
Member
faho commented Oct 27, 2016

This should be fixed by the commit I just pushed. I verified it does not fail in konsole (since that does not have $XTERM_VERSION) or xterm 327. It does with "XTerm(280)" and invalid values for $XTERM_VERSION.

@faho faho removed the needs-more-info label Oct 27, 2016
@ridiculousfish
Member

Unfortunately this isn't fixed on CentOS5 at least. The issue there is that $TERM is xterm, but $XTERM_VERSION is unset.

@floam
Member
floam commented Oct 27, 2016

Any chance they are betraying indication of support for this via terminfo somehow, in any of the extensions? infocmp -x?

@faho
Member
faho commented Oct 27, 2016

The issue there is that $TERM is xterm, but $XTERM_VERSION is unset.

$*§%$@! That means using $TERM to detect it is broken. We cannot do this unless we expect people on ancient OSs to manually disable it, which doesn't seem friendly.

@floam
Member
floam commented Oct 27, 2016

Possibly you could still detect it by trying the escape, and see if it moved the cursor... if it did erase that and never try it again.

@ridiculousfish
Member

@floam from brief testing it looks like infocmp -x has an entry Ss= if and only if DECSCUSR is set.

@faho what's the issue with only enabling it for $TERM=xterm and when $XTERM_VERSION is set (and sufficient version)?

@faho
Member
faho commented Oct 27, 2016

This really is an issue for ancient xterm versions - from the changelog

Patch #202 - 2005/5/2 - XFree86 4.5.99.3
[...]
add environment variables $XTERM_SHELL and $XTERM_VERSION (request by Zdenek Sekera).

Yes. 2005. XFree86.

The issue there is that $TERM is xterm

@ridiculousfish: Is it actually just "xterm"?

If so, we might be able to get away with blacklisting $TERM = xterm, and reading $XTERM_VERSION for "xterm-256color".

Btw "DECSCUSR" was added in 252, in 2009, not 282 in 2012 (though it was extended then).

@faho
Member
faho commented Oct 27, 2016 edited

what's the issue with only enabling it for $TERM=xterm and when $XTERM_VERSION is set (and sufficient version)?

The issue is that everyone and their mum claims to be "xterm" (or "xterm-256color"), including iTerm, but I don't believe they set $XTERM_VERSION.

Edit: Gnome-terminal at least can accept this sequence, sets $TERM to xterm-256color but does not set $XTERM_VERSION.

@floam
Member
floam commented Oct 27, 2016 edited

I think iTerm should be setting xterm-256color, at least newer ones. It can also set other TERMs though. Ss seems ideal to use but of course plenty of terminfo.src's are out of date and won't have it. Detecting terminal capabilities reliably is a total shit show unfortunately.

@ridiculousfish
Member

@ridiculousfish: Is it actually just "xterm"?

Yes, exactly that.

I like the idea of basing the decision off of environment variables. It is less precise than reading from terminfo (i.e. infocmp) but much simpler.

@floam
Member
floam commented Oct 27, 2016 edited

As long as nothing sets xterm-256color and is unable to handle this, it seems to make sense to leave xterm out, and rely on $XTERM_VERSION, else tput Ss ... for it there. (Or just leave it out of the party entirely.)

@faho
Member
faho commented Oct 27, 2016

As long as nothing sets xterm-256color and is unable to handle this, it seems to make sense to leave xterm out, and rely on tput Ss ... for it there.

First of all, we might use the tput thing to determine if the xterm terminfo is new enough, which would correspond to an installed xterm being new enough. But we should not use it for the sequence itself. Both konsole and iTerm default to xterm-256color but use a different sequence (and choke on the xterm one).

Secondly, here's what we could get with environment variables:

  • For xterms from version 202 (from 2005) to 281, read $XTERM_VERSION and refuse to change cursor
  • For xterms from 252/282 (from 2009/2012), read $XTERM_VERSION and change cursor
  • For terms pretending to be xterm-256color, change cursor
  • For terms pretending to be xterm, refuse
  • For xterms < 202 that use xterm, refuse
  • For xterms < 202 that use xterm-256color, try to change cursor and end up with spurious stuff on screen

The question here is if that last case exists. Is there anything but CentOS/RHEL that is still supported and uses xterm < 202? Do they set TERM = xterm-256color?

I can't come up with something that is supported as long, and I think if they are sooooo stale that they use 11+ year old xterm, they are probably also conservative enough to distrust color support (I recall 256 color support was a big release notes item for Fedora 19 or so).

@ridiculousfish
Member

Looks like xterm-256color is from patch #111 which predates the cursor shape stuff. So the bad case seems possible in principle, though in practice it's hard to say.

@faho
Member
faho commented Oct 27, 2016

So the bad case seems possible in principle, though in practice it's hard to say.

Okay, I think we need tput Ss in some form. We can't use the sequence, but we should be able to use it for detection. The question is how much we want to rely on it.

@faho faho self-assigned this Oct 27, 2016
@faho
Member
faho commented Oct 27, 2016

So, here's what I came up with:

    # Check hard if we are in a supporting terminal.
    #
    # Issues here are term-in-a-terms (emacs ansi-term does not support this, tmux does),
    # that we can only figure out if we are in konsole/iterm/vte via exported variables,
    # and ancient xterm versions.
    #
    # tmux defaults to $TERM = screen, but can do this if it is in a supporting terminal.
    # Unfortunately, we can only detect this via the exported variables, so we miss some cases.
    #
    # We will also miss some cases of terminal-stacking,
    # e.g. tmux started in suckless' st (no support) started in konsole.
    # But since tmux in konsole seems rather common and that case so uncommon,
    # we will just fail there.
    #
    # We use the `tput` here just to see if terminfo thinks we can change the cursor.
    # We cannot use that sequence directly as it's not the correct one for konsole and iTerm.
    if not tput Ss > /dev/null
        # Whitelist tmux...
        and not begin
            set -q TMUX
            # ...in a supporting term...
            and begin set -q KONSOLE_PROFILE_NAME
                or set -q ITERM_PROFILE
                or test "$VTE_VERSION" -gt 1910
            end
            # .. unless an unsupporting terminal has been started in tmux inside a supporting one
            and begin string match -q "screen*" -- $TERM
                or string match -q "tmux*" -- $TERM
            end
        end
        and not string match -q "konsole*" -- $TERM
        # Blacklist
        or begin
            # vte-based terms set $TERM = xterm*, but only gained support relatively recently.
            set -q VTE_VERSION
            and test "$VTE_VERSION" -le 1910
        end
        or set -q INSIDE_EMACS
        return
    end
@ridiculousfish
Member

Cool. If you push to master or a branch, I can give it a try on CentOS5.

@floam
Member
floam commented Oct 27, 2016 edited

We might want to whitelist Terminal.app.

I notice that there is no Ss with the Apple-provided terminfo but if I install a new ncurses via homebrew and do this:

env TERMINFO=/usr/local/Cellar/ncurses/6.0_2/share/terminfo/ fish

.. the tput Ss sequences do change the cursor. (Among other things like dim and italics. Wish they'd update their xterm-256color entries.)

@faho
Member
faho commented Oct 27, 2016

@floam: It's the same sequence as xterm and the way to detect it is $TERM_PROGRAM = "Apple_Terminal", right? Are there older supported versions of it that truly don't support it?

@floam
Member
floam commented Oct 27, 2016

Yes for the first question. I'm not sure about which OS release added the cursor support. It might be necessary to check TERM_PROGRAM_VERSION. Not quite sure how to figure that out without any old machines I have access to.

@floam
Member
floam commented Oct 27, 2016

I can only really attest that at least with 387 (latest), it works.

@faho faho added a commit to faho/fish-shell that referenced this issue Oct 27, 2016
@faho faho Rework cursor detection
Fixes #3499.
b31bb46
@faho
Member
faho commented Oct 27, 2016

@ridiculousfish: My "cursor" branch has it.

@floam: I'd like to not add that at this point if we don't have a definitive answer. Let's keep this change to a minimum for 2.4.0 at least.

@floam
Member
floam commented Oct 27, 2016

Sure, that'd make more sense for the next release.

@ridiculousfish
Member

@faho not seeing your cursor branch - perhaps you have not yet pushed it?

@floam
Member
floam commented Oct 27, 2016 edited

It's a pretty inconsequential point but probably one could use the sequence from the tput Ss $i itself, by providing it a -T argument with the "real" terminal, or at least one with the escape one wants.

@zanchey
Member
zanchey commented Oct 31, 2016

@ridiculousfish, any luck? This is the only thing blocking 2.4.0.

@ridiculousfish
Member

Sorry for the delay. @faho 's cursor branch indeed fixes the problem on CentOS5.

@zanchey zanchey added a commit that closed this issue Oct 31, 2016
@faho @zanchey faho + zanchey Rework cursor detection
Fixes #3499.
2a5ad19
@zanchey zanchey closed this in 2a5ad19 Oct 31, 2016
@zanchey zanchey added a commit that referenced this issue Oct 31, 2016
@faho @zanchey faho + zanchey Disable vi-cursor on xterm < 282
Fixes #3499.

@zanchey: This is for integration-2.4.0.

(cherry picked from commit 7ea2dc4)
7bcae09
@zanchey zanchey added a commit that referenced this issue Oct 31, 2016
@faho @zanchey faho + zanchey Rework cursor detection
Fixes #3499.

(cherry picked from commit 2a5ad19)
c8fe0e5
@pawelmhm
pawelmhm commented Nov 16, 2016 edited

Hello there,

I'm the guy who reported this issue via email to @zanchey is it meant to be fixed in 2.4.0-2?

Because I updated to 2.4.0.2 today and I see the problem persists, see screenshot

screenshot from 2016-11-16 08 20 57

@floam
Member
floam commented Nov 16, 2016 edited

Hi, no, it wasn't, it was fixed after 2.4.0 was released. You'll need to build fish from git master.

edit: Er, it was partially fixed for 2.4.0. See #3535

@pawelmhm
pawelmhm commented Nov 16, 2016 edited

do you know when there will be next release? I prefer to install from PPA rather than build from source, this way I can keep my system sane. I see now old version is no longer in PPA so I can't even downgrade to old version.

@floam
Member
floam commented Nov 16, 2016 edited

I'm not sure, but entering this will work around the issue for you:

function fish_vi_cursor; end
funcsave fish_vi_cursor
@pawelmhm
pawelmhm commented Nov 16, 2016 edited

indeed that helps, i added this to my config.fish. Thanks @floam

From above discussion I see that it happens only in some terminals. Should I upgrade my terminal too?

@floam
Member
floam commented Nov 16, 2016

It shouldn't be necessary to enter that in your config.fish file, and might slow things down - typing it out and entering it once will suffice because the funcsave command will create a fish_vi_cursor.fish file with a no-op function in ~/.config/fish/functions.

What terminal are you using and what is the output of echo $TERM and echo $VTE_VERSION?

@pawelmhm
> echo $TERM
xterm
> echo $VTE_VERSION
3409
@faho
Member
faho commented Nov 16, 2016

echo $VTE_VERSION
3409

WAT? That version should support this sequence! Which terminal is it, are you using any multiplexers or other kinds of term-in-a-terms?

I'll try to find the documentation as to when this was introduced again.

@faho faho reopened this Nov 16, 2016
@pawelmhm

Which terminal is it, are you using any multiplexers or other kinds of term-in-a-terms?

No, this is just pure and simple xterm without anything fancy. Default terminal on ubuntu 14.04.

@faho
Member
faho commented Nov 16, 2016

just pure and simple xterm without anything fancy

I don't think so - if it were xterm, then $VTE_VERSION shouldn't be defined.

@faho
Member
faho commented Nov 16, 2016

Okay, this is a gtk3 gnome-terminal with vte 0.34.9. From https://github.com/GNOME/vte/blob/master/NEWS, version 0.19.1 has

  • Configurable cursor shape (block, underline, I-beam).

I have absolutely no idea why this shouldn't be able to do the sequence.

I mean it's possible that that entry is about something different, but I cannot find anything else in that file.

@floam
Member
floam commented Nov 16, 2016

Configurable cursor shape (block, underline, I-beam).

Maybe it just means that the user can choose one of those cursors in the app configuration, but it doesn't allow switching on the fly through that sequence?

@faho
Member
faho commented Nov 16, 2016

Maybe it just means that the user can choose one of those cursors in the app configuration, but it doesn't allow switching on the fly through that sequence?

It's possible, but then I just can't find a mention of this sequence in that file at all. I searched for "shape", "DECSCUSR" and just "cursor" and I cannot find anything about it. I think I've searched before for that version (though I believed wrongly that it was rather old).

Anyway, what's the oldest version that we know this works with? Because mine is "4601" (i.e. 0.46.1), and I doubt that's the first version to support it. If I don't get any more info, I can just use that.

@pawelmhm
pawelmhm commented Nov 16, 2016 edited

this is a gtk3 gnome-terminal with vte 0.34.9. From

how did you find this out? My help -> about says GNOME Terminal 3.6.2 does this match your findings?

edit: so I guess I'm using this: https://launchpad.net/ubuntu/+source/gnome-terminal/3.6.2-0ubuntu1

@faho
Member
faho commented Nov 16, 2016

@pawelmhm: I ran Ubuntu 14.04 in a vm and just checked apt show libvte<TAB>. The gnome-terminal version isn't as important, the vte one is.

@faho faho added a commit that closed this issue Nov 16, 2016
@faho faho vi_cursor: Set required VTE version to 4000
It seems the changelog entry for 1910 was misleading.

Fixes #3499.
3e82be4
@faho faho closed this in 3e82be4 Nov 16, 2016
@faho
Member
faho commented Nov 16, 2016

@pawelmhm: I believe this is now fixed by not sending the sequence for VTE-based terms < 0.40.0.

@pawelmhm

thanks will test with fish source later in the day. If I'd like to benefit from this cool new feature I'm missing out with old VTE version what should I do? Upgrade libvte?

@floam
Member
floam commented Nov 17, 2016

You'll need a newer libvte at least, not sure if that means a newer gnome-terminal as well.

You'll also need to get rid of the fish_vi_cursor.fish in your .config/fish/functions, which is forcing it disabled for you now.

@faho
Member
faho commented Nov 17, 2016

@pawelmhm: The easiest solution is to get a different terminal. Xterm should work - as in real proper xterm. Like floam, I'm not sure how interdependent the pieces are for vte and gnome-terminal, so I can't recommend that route.

@floam
Member
floam commented Nov 17, 2016

If there is a newer version of ubuntu you can dist-upgrade to, I'll hazard a guess that the newer GNOME which comes along with a newer gnome-terminal which will probably have a newer VTE, could probably get you something new enough.

@faho
Member
faho commented Nov 17, 2016

Yeah, but of course that'll pull in a whole bunch of other stuff. Though I didn't think of that since I assumed it was the latest LTS. Since 16.04 is out, that'd also be a viable solution.

@faho faho added a commit that referenced this issue Nov 18, 2016
@faho faho vi_cursor: For TERM = xterm require another condition
We cannot just use TERM = xterm and defined Ss sequence, as some old
vte-based terminals are still in the wild that don't support the
sequence and don't have $VTE_VERSION set.

I have tested this on

- konsole - supported and works ($KONSOLE_PROFILE_NAME)
- new xterm - supported and works ($XTERM_VERSION)
- lxterminal-gtk3 - supported and works ($VTE_VERSION)
- new gnome-terminal - supported and works ($VTE_VERSION)
- lxterminal-gtk2 - not supported and deactivated (no $VTE_VERSION)
- tmux in konsole - works
- tmux in lxterminal-gtk2 - deactivated

and for all supported ones with the respective variable erased, to see
that it is deactivated.

Fixes #3499.
acc2353
@faho
Member
faho commented Nov 18, 2016

@pawelmhm: Can you confirm that acc2353 does not try to set the cursor, i.e. does not leak this escape sequence? You should just be able to take fish_vi_cursor.fish from that commit and stick it into ~/.config/fish/functions/.

@aureooms

Works in terminator!

@warptozero

When I have fish_vi_key_bindings insert set on my server, git gives the following error message upon fetching or pushing:

fatal: protocol error: bad line length character: 
�[1 ⏎

function fish_vi_cursor; end has no influence.

Could this be related or should I start a new issue?

@faho
Member
faho commented Jan 6, 2017

@warptozero:

function fish_vi_cursor; end has no influence.

It's not enough to run that interactively - the function defines event handlers that also exist without it. Try function fish_vi_cursor; end; funcsave fish_vi_cursor.

Could this be related or should I start a new issue?

Both. This is a different kind of incompatibility, and we'll need a bunch more info on it. Most of all your OS, your locale, your terminal and what you are using to connect to that server (ssh?). That's best done in a new issue.

@warptozero
warptozero commented Jan 6, 2017 edited

It's not enough to run that interactively

I'm using it in the config file though.

Thanks for the response, I'll start a new issue with the info you requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment