Skip to content

Add workaround for Midnight Commander's issue with prompt extraction #9540

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

Merged
merged 1 commit into from
Feb 11, 2023

Conversation

faho
Copy link
Member

@faho faho commented Feb 4, 2023

Description

When we draw the prompt, we move the cursor to the actual position we think it is by issuing a carriage return (via move(0,0)), and then going forward until we hit the spot.

This helps when the terminal and fish disagree on the width of the prompt, because we are now definitely in the correct place, so we can only overwrite a bit of the prompt (if it renders longer than we expected) or leave space after the prompt. Both of these are benign in comparison to staircase effects we would otherwise get.

Unfortunately, midnight commander ("mc") tries to extract the last line of the prompt, and does so in a way that is overly naive - it resets everything to 0 when it sees a \r, and doesn't account for cursor movement. In effect it's playing a terminal, but not committing to the bit.

Since this has been an open request in mc for quite a while, we hack around it, by checking the $MC_SID environment variable.

If we see it, we skip the clearing. We end up most likely doing relative movement from where we think we are, and in most cases it should be fine.

TODOs:

  • [N/A] Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

When we draw the prompt, we move the cursor to the actual
position *we* think it is by issuing a carriage return (via
`move(0,0)`), and then going forward until we hit the spot.

This helps when the terminal and fish disagree on the width of the
prompt, because we are now definitely in the correct place, so we can
only overwrite a bit of the prompt (if it renders longer than we
expected) or leave space after the prompt. Both of these are benign in
comparison to staircase effects we would otherwise get.

Unfortunately, midnight commander ("mc") tries to extract the last
line of the prompt, and does so in a way that is overly naive - it
resets everything to 0 when it sees a `\r`, and doesn't account for
cursor movement. In effect it's playing a terminal, but not committing
to the bit.

Since this has been an open request in mc for quite a while, we hack
around it, by checking the $MC_SID environment variable.

If we see it, we skip the clearing. We end up most likely doing
relative movement from where we think we are, and in most cases it
should be *fine*.
@faho faho added this to the fish 3.6.1 milestone Feb 4, 2023
@ridiculousfish ridiculousfish self-requested a review February 10, 2023 03:38
@ridiculousfish
Copy link
Member

Good fix. Long term I'd like to remove the move(0, 0) in more cases (for example when the input is all ASCII) since it does add to the cursor movement traffic.

@faho faho merged commit b1b2294 into fish-shell:master Feb 11, 2023
@faho faho deleted the workaround-mc branch February 11, 2023 13:18
@mqudsi
Copy link
Contributor

mqudsi commented May 1, 2023

The move(0, 0) adds noticeable delay on conhost (Windows terminal).

@faho
Copy link
Member Author

faho commented May 1, 2023

Unfortunately, the move(0, 0) helps quite a bit - #8011.

So I would love if this could just be fixed in conhost, which has a billion dollar company behind it.

Alternatively, I'm not sure if the move(0, 0) is the actual problem or the move after - is it possible we do cell-by-cell movement here and can that be changed?

@mqudsi
Copy link
Contributor

mqudsi commented May 1, 2023

#8011 is only if right prompt exists (and it doesn't by default) so I think it's fine it ignore.

Alternatively, I'm not sure if the move(0, 0) is the actual problem or the move after

It could be the move after, I'm not sure. All I know is that I can see the cursor moving to the start of the line when doing heavy terminal output so it's a non-negligible amount of time.

@faho
Copy link
Member Author

faho commented May 1, 2023

only if right prompt exists (and it doesn't by default) so I think it's fine it ignore.

No, it's not. It started as that, then I noticed that it suddenly worked better with a right prompt, so I added it to the path without as well.

What happens is that, without the move to the beginning, we depend on our width calculation for the prompt (including left!) being correct - and "correct" in the sense that it's the same result the terminal reached. If it's not, we can be off in all manner of ways.

So, by moving to the beginning, we remove that and then go explicitly to the cell we want to draw the commandline in. So if we're off in the prompt calculation, we can both overwrite the prompt or leave a gap after the prompt, but we can never overflow the line because of errors there. That was a big issue that could render fish almost unusable, and was easily triggered with ambiguous-width codepoints. We've had it reported multiple times, and zero times since #8011.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants