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

[1.15+] Flushing FTCS sequences causes flickering in nushell 0.66+ #13710

Closed
rgwood opened this issue Aug 9, 2022 · 29 comments · Fixed by #14677
Closed

[1.15+] Flushing FTCS sequences causes flickering in nushell 0.66+ #13710

rgwood opened this issue Aug 9, 2022 · 29 comments · Fixed by #14677
Assignees
Labels
Area-VT Virtual Terminal sequence support In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Conpty For console issues specifically related to conpty

Comments

@rgwood
Copy link

rgwood commented Aug 9, 2022

Windows Terminal version

1.15.2003.0

Windows build number

Microsoft Windows [Version 10.0.22000.795]

Other Software

Nushell v0.66+

Steps to reproduce

Hi, I work on a cross-platform shell called Nushell and we've noticed a rendering regression in Windows Terminal Preview v1.15.

Nushell's prompt now flickers on every keystroke. The following recording illustrates the problem (but it's a little worse in real life, the recording didn't capture every flicker):

2022-08-09_11-27-44.mp4

To reproduce this, run any version of Nushell from v0.66 or later and start typing. Nushell can be downloaded from our GitHub releases or installed with Scoop (scoop install nushell) or winget (winget install nushell). nu.exe is the executable for Nushell.

Details / Investigation

The flickering does not occur in Windows Terminal v1.14.1963.0.

We bisected Nushell's code and found that the flickering did not occur prior to this PR which added some scroll mark ANSI escape codes for VS Code. WT v1.15 Preview added experimental support for scroll marks; maybe this is a bug in the experimental scroll mark code?

The flickering occurs even with the scroll mark settings (experimental.autoMarkPrompts and experimental.showMarksOnScrollbar) set to false.

If there's anything else we can do to help with investigation or reproduction, just let me know!

@rgwood rgwood added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Aug 9, 2022
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 9, 2022
@zadjii-msft
Copy link
Member

zadjii-msft commented Aug 9, 2022

(I'm supposed to be on vacation RN so apologies for the brevity in my notes)

Hmmmmmmmmm. I wonder if this isn't playing nicely with the frame flushing. Like, when the prompt gets drawn, what's happening is a

  • Move the cursor to the start of the prompt
  • erase the rest of the line
  • mark here as the start of the prompt (FLUSHES THE FRAME)
  • draw the prompt
  • mark here as the end of the prompt (flushes maybe? maybe not yet)
  • draw the right side of the status

The trick is that we're flushing all buffered state to the Terminal when we see a prompt sequence. The problem is that at the time the mark is re-rendered, the prompt line is empty, causing the flickering.

No clever solutions off the top of the dome. I'll noodle on the drive home.


Edit 2023/01/10: The above set of steps is wrong. Don't use that as a guide for what's happening.

@zadjii-msft zadjii-msft changed the title Regression: flickering when typing in v1.15 Preview [1.15+] Flushing FTCS sequences causes flickering in nushell 0.66+ Aug 11, 2022
@fdncred
Copy link

fdncred commented Aug 29, 2022

@zadjii-msft for what it's worth, we're tracking a related issue with WezTerm. I say they're related because disabling the OSC 133 (and friends) escape sequences seem to correct the problem but, of course, we lose the features that OSC 133 provides. @wez, the author, has a theory here nushell/nushell#5585 (comment).

Quoting here

If I edit the macos capture and remove the OSC 133 A and OSC 133 B sequences, and replay that file on windows, then the output looks correct. Those are the FinalTerm style shell integration escape sequences.

It looks to me as though this is a ConPTY issue where unexpected/unknown OSC sequences confuse it about cursor positioning or some other similar concern that triggers it to rewrite the output stream.

A workaround would be to skip outputting OSC 133 on Windows or if you know that ConPTY is present.

You can see more of the detailed research here nushell/nushell#5585

@zadjii-msft
Copy link
Member

Ah, that's probably another related issue. Before a few months ago, conpty didn't recognize those sequences at all, and would emit them immediately, possible before the rest of the "frame" they were emitted with. at the end of the day, it just means that FTCS sequence support on older Windows builds is gonna be wacky.

Now, the Terminal uses an out-of-band version of the ConPTY apis. We do that so we can always use the main version of ConPTY in the Terminal, rather than the OS one (which might be months or years out of date). So the issue in the Terminal version of ConPTY is caused by me trying to fix the issue in the OS version of ConPTY (used by wezterm et. al).

If that all makes sense.

@fdncred
Copy link

fdncred commented Aug 29, 2022

it just means that FTCS sequence support on older Windows builds is gonna be wacky.

It all makes sense except this was on an updated copy of Windows 11. So, I expect old versions to be wacky but hopefully recent Win10 and Win11 will behave better in the near future.

@zadjii-msft
Copy link
Member

an updated copy of Windows 11

I mean, FTCS support landed literally 3 months ago - I'm not sure it's even made its way to any official Windows builds yet 😅

@wez
Copy link

wez commented Aug 29, 2022

FWIW, wezterm bundles and uses openconsole.exe and conpty.dll built from GitHub. I just updated it to the most recent tag this morning and the issue reproduces there too.

wez/wezterm@0d85d05

@zadjii-msft zadjii-msft added Product-Conpty For console issues specifically related to conpty Needs-Discussion Something that requires a team discussion before we can proceed labels Aug 30, 2022
@carlos-zamora carlos-zamora removed the Needs-Discussion Something that requires a team discussion before we can proceed label Oct 4, 2022
@FilipAndersson245
Copy link

Any update on this issue @zadjii-msft?

@zadjii-msft
Copy link
Member

Alas, no. I haven't noodled on this too much since getting back from leave, and I'm only just getting back into the swing of things after the holidays.

I've got this marked up to take a look at ~this month, but I seriously doubt I'll have a good solution that fast.

IIRC we briefly discussed some ideas, that might work okay in this specific scenario, but wouldn't work more broadly for the other sequences that are leveraging the same flushing that we do here (looking at you alt buffer sequences). I can't remember the details though 😕

@zadjii-msft zadjii-msft added the Area-VT Virtual Terminal sequence support label Jan 5, 2023
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 5, 2023
@ghostbutter-games
Copy link

I would also really appreciate a solution for this. Makes using Windows Terminal a pain right now because of all the flickering.

@zadjii-msft
Copy link
Member

zadjii-msft commented Jan 10, 2023

(huge apologies to folks following this thread, these notes are an unintelligible stream of consciousness)

Refer to #13163

My outline of events in #13710 (comment) might be wrong[1]. Look at the video closer - is that it? The left prompt is always visible, even as command and right prompt are flickering. Hmm.

This diff would seem to suggest that the prompt is written as one long string, "FTCS A + prompt + FTCS B", presumably followed by the command, followed by the right prompt. In 1.15, only FTCS A would have been implemented. I don't think that matters

In 1.15 we return false out of the adapter on the FTCS B which means "I didn't understand that, just write that to the pipe NOW". this is also wrong.

ConPty always flushes all FTCS's in 1.15. On "FTCS A + prompt + FTCS B", we do

[
  "FTCS A" -> Flush(),
  prompt + "FTCS B" -> Flush(),
  input command + right prompt -> (regularly scheduled) Paint()
]

as three frames. That it shouldn't. That probably should all be one frame. Can we just FlushNoWrite() from the adapter? Would that be insane? We'd have to be careful to write to the buffer, not write the frame to the pipe, write the sequence, but then also leave the engine in a state where the regularly scheduled paint frame tick would still cause the actual Present to the pipe, but with a quick path for "we had nothing to do but we did have stuff we already did.

I'll keep editing with notes as I go

[1] it might not be, I've re-edited this comment like 10 times as writing it


This is what I get out the debug tap. Now, where are the frames....

  • Frame 1:
␍
␛[K
␛[?25l
␛[57C
␛]133;A␛\

CR, cursor off, Cursor Forward 17 (?!), mark

  • Frame 2:
␛[32m
␛[1m
␍
C:\Users\migrie
␛]133;B␛\
␛[2␣q
␛[m
␛[38;5;14m
␛[1m
〉
␛[36m
␛[22m
z
␛[38;5;5m
␛[1m
␛[17C
01/11/2023␣09:43:35␣AM
␛[24;19H
␛[?25h
␛[m

This is what actually gets written to conhost

�[?25l
�[24;1H
�[J
�[38;5;10m
�[1m
�]133;A�\
�[1;32m
C:\Users\migrie
�]133;B�\
�[38;5;14m
�[1m
〉
�[38;5;5m
�[1m
�7
�[24;36H
01/11/2023 10:40:54 AM
�8
�[0m
�[36m
zmh
�[0m
�7
�8
�[2 q
�[?25h

�[2 q looks out of place....


Okay playing with this more locally, and staring at it, I do see the prompt section flickering as I'd expect.

So, I guess the trick now is that even if we do not immediately flush the frame, a regular frame might come in in the middle of the string. Hmm.

Bad idea

  • if we have buffered output on StartPaint, but there's nothing to do, don't flush.
  • At the end of a VT string, in pty mode, poke the render engine to let it know it's safe to flush.

The console should be locked for that whole write, start to finish. ApiRoutines::WriteConsoleWImpl takes the lock and holds it till it's all processed. So that must mean we're getting a Flush we're not expecting, during the handling of the string. After the 133;A, before the B. Hmm.

@zadjii-msft zadjii-msft self-assigned this Jan 10, 2023
@jxcrw
Copy link

jxcrw commented Jan 10, 2023

I accidentally discovered one configuration in which nushell does not seem to flicker within Windows Terminal:

  • Enabled WSL1 on a Windows 11 22H2 system.
  • Installed ArchWSL as the Linux distribution (btw :) ).
  • Installed (Windows version of) nushell via scoop.
  • Ran nu.exe from within an ArchWSL profile in Windows Terminal.
  • No flicker.

In contrast, all of the following exhibit the flicker in Windows Terminal (on the same Windows 11 22H2 system):

  • Running Linux version of nushell (installed via pacman) from within ArchWSL profile in Windows Terminal.
  • Running Windows version of nushell from within Powershell profile in Windows Terminal.
  • Running Windows version of nushell from within cmd profile in Windows Terminal.

Absolutely no idea what it is about the former configuration that differs from the latter three in terms of flicker-proneness, but posting just in case it may be a clue for anyone who has a better bead on this.

@zadjii-msft
Copy link
Member

@jxcrw that makes sense. The nu.exe in WSL invokes another ConPTY, one that shipped with the OS which farther out of date, and that version would end up probably either

  • eating all the FTCS sequences
  • passing them through immediately to the Terminal, before the rest of the frame starts rendering, so nothing looks out of place (though, shell integration wouldn't work right)

@popcar2
Copy link

popcar2 commented Feb 3, 2023

I would also really appreciate a solution for this. Makes using Windows Terminal a pain right now because of all the flickering.

A fix was mentioned in another thread and I can confirm it works:

  • Type config nu
  • In the config file, find shell_integration and set it to false

It should stop flickering now. I don't know what the downside is for disabling shell integration, everything seems to work totally fine over a month in.

@fdncred
Copy link

fdncred commented Feb 3, 2023

@popcar2 When you disable shell_integration you lose the ability to have automatic OSC 133;A/B/C/D terminal markers (FTCS sequences) and automatic OSC 2 & OSC 7 term/window titles. It may also interfere with OSC 8 clickable links in nushell.

@Yiniau
Copy link

Yiniau commented Feb 18, 2023

Interestingly, there is no such problem in the terminal of vscode

@lost22git
Copy link

any update?

This problem does not occur when nushell runs on alacritty ;-)

@lost22git
Copy link

any update?

This problem does not occur when nushell runs on alacritty ;-)

Alacritty does not seem to support OSC133 ;-)

alacritty/alacritty#5850

DHowett pushed a commit that referenced this issue May 11, 2023
Tl;dr: Conpty would flush a frame whenever it encountered a FTCS mark.
Combine that with the whole-line redrawing that nushell does, and the
Terminal would get the prompt in two frames instead of one, causing a
slight flickering. This fixes that by rendering the frame, but not
flushing to the pipe when we encounter one of these sequences.

Closes #13710 

A complication here: there are some sequences that we passthrough
_immediately_ when we encounter them. For example, `\x1b[ 2q`. we need
to also not flush when we encounter one of these sequences. nushell
emits one of these as a part of the prompt, and that would force the
buffered frame to get written _anyways_, before writing that to the
pipe.
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label May 11, 2023
wez added a commit to wez/wezterm that referenced this issue May 21, 2023
I want to try out the FTCS flush and any other conpty fixes that may
have been made since our last update.

refs: microsoft/terminal#13710

These other issues may experience improved results, but likely not:

refs: #3748
refs: #3624
refs: #2779
refs: #2902
refs: #3531
refs: #3562
@bennett-sh
Copy link

Nushell is still flickering when used within Windows Terminal (both MS store stable/preview + latest GitHub release).

@zadjii-msft
Copy link
Member

What version are you on exactly? I just tried 1.18.1462 here and I don't see any flickering. Perhaps what you're seeing is one of the newer rendering bugs? That might repro regardless of what shell you're using. Could you share your settings.json?

@bennett-sh
Copy link

bennett-sh commented May 28, 2023

nu v0.80.0
windows terminal v1.17.1023 | 1.17.1023
is v1.18.x built from source or where did you get it from?
edit: nvm forgot to check pre-releases and it doesn't flicker anymore

@zadjii-msft
Copy link
Member

Oh good, I'm glad! This was a tricky fix so I'm really glad it did work after all.

Looks like our friendly neighborhood bot is having a tough time this week and forgot to come around with release notes. This fix shipped in Terminal Preview 1.18 earlier this week.

@fdncred
Copy link

fdncred commented May 28, 2023

No flickering issues with nushell and v1.18 when I tested (great job with this!!!), although it wasn't real stable. I also noticed that the right click command selection was selecting the wrong data. I tried the debug tap and think our escapes look good.

@wez
Copy link

wez commented May 28, 2023

if one of you could check this behavior with the current nightly build of wezterm, I'd appreciate it: I updated the bundled conpty pieces to match 0ee2c74 of this repo, so my understanding is that this should be fixed in wezterm now too.

@fdncred
Copy link

fdncred commented May 29, 2023

@wez wezterm nightly, downloaded from here exhibits a similar issue as before but slightly different.
wezterm

@rgwood
Copy link
Author

rgwood commented Feb 24, 2024

Uh oh, the terminal flickering is back!

I can reproduce it in Terminal Preview 1.20.10303.0 and Nu 0.90.1. I cannot reproduce it in Terminal 1.18.10301.0.

It seems to occur with or without AtlasEngine enabled.

@zadjii-msft Would you like me to open a new issue for this?

@lhecker
Copy link
Member

lhecker commented Feb 26, 2024

@rgwood I can't reproduce any flickering when using nushell's default out-of-the-box configuration.

I think it'd be best to open a new issue for this. I don't think it'll be necessary to describe the issue in detail - IMO you can just refer to this issue (#13710) in your description. 😅

Given that I don't have this issue, could you check if it occurs when you use the default configuration? If it doesn't reproduce for you either, it'd be nice if there was some way to figure out which option or plugin in nushell causes the issue. (...since reproducing it locally would help me a lot while fixing it. 🙂)

@rgwood
Copy link
Author

rgwood commented Feb 26, 2024

@lhecker I'm really sorry, I should have clarified that this only occurs with shell_integration: true set in the Nushell config (I forgot that this is not on by default. It enables the escape codes for prompt markers etc).

Easiest way to change this setting is by opening the config file with code $nu.config-path or similar, changing it, and relaunching Nushell.

@lhecker
Copy link
Member

lhecker commented Feb 26, 2024

Thanks! That was super easy to reproduce. I should've realized that given that this issue was also about FTCS. I've opened #16769 so we don't ping the ~dozen people on this issue unnecessarily. I'll try to fix it soon (if possible).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

Successfully merging a pull request may close this issue.