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

fix: improve terminal marker placements on windows #209136

Merged
merged 20 commits into from
Apr 19, 2024

Conversation

cpendery
Copy link
Member

@cpendery cpendery commented Mar 29, 2024

This PR attempts to approve terminal marker placements on Windows in 2 ways.

  1. If the Windows prompt adjustment fails, reattempt to identify the command start when a user provides input. This helps with multiline prompts like used in Git Bash
  2. Adjust the prompt start and end markers for all Windows prompts, not just single line prompts
    note: while this will result in the starting line of a multiline prompt getting captured in the output, it improves the sticky scroll behavior significantly.

Sticky Scroll Pre Fix

stickyScrollBad.mp4

Sticky Scroll Post Fix

stickyScrollFix.mp4

Input Prompt Adjustment Example 1

markerInputCorrection2.mp4

Input Prompt Adjustment Example 2

markerInputCorrection.mp4

Related to #143769 & #209003

Signed-off-by: Chapman Pendery <cpendery@microsoft.com>
@cpendery cpendery force-pushed the fix/improve-marker-placements branch from f47baf3 to 83b639d Compare March 29, 2024 19:35
@cpendery cpendery marked this pull request as ready for review March 29, 2024 19:43
@Tyriar Tyriar added this to the April 2024 milestone Apr 1, 2024
cpendery and others added 5 commits April 16, 2024 18:56
Signed-off-by: Chapman Pendery <cpendery@microsoft.com>
Signed-off-by: Chapman Pendery <cpendery@microsoft.com>
Signed-off-by: Chapman Pendery <cpendery@microsoft.com>
Signed-off-by: Chapman Pendery <cpendery@microsoft.com>
@cpendery
Copy link
Member Author

cpendery commented Apr 17, 2024

I fixed the stray markers, avoid adjusting the prompt start if the text is wrapped, and 1-2 small issues I found testing. Here are some new demo videos

No Adjust When Wrapped

fixed_bashNoAdjustOnWrap.mp4

Multi-line Git Bash Adjust

fixed_bashInputAdjust.mp4

Multi-line Pwsh Adjust

fixed_pwshMutliInputAdjust.mp4

Single-line Pwsh Adjust

fixed_pwshInputAdjust.mp4

Signed-off-by: Chapman Pendery <cpendery@microsoft.com>
@Tyriar
Copy link
Member

Tyriar commented Apr 17, 2024

Just tried with Windows/pwsh 7/starship and the multi-line prompt is still not coming through in sticky scroll:

image

It's working for git bash though 🤔

@cpendery
Copy link
Member Author

Just tried with Windows/pwsh 7/starship and the multi-line prompt is still not coming through in sticky scroll:

image

It's working for git bash though 🤔

I'll setup starship and see if I can replicate this.

@cpendery cpendery marked this pull request as draft April 17, 2024 17:07
@cpendery cpendery force-pushed the fix/improve-marker-placements branch from 7ebd75c to 33064ec Compare April 17, 2024 17:15
cpendery and others added 4 commits April 17, 2024 10:16
@cpendery
Copy link
Member Author

I think I fixed it, there were small bugs. It's working for git bash (starship, default multiline, singleline) & pwsh (starship, oh-my-posh [json theme], & default)

  1. bash prompts were getting a height of 1 too small since it doesn't have trailing newlines like a powershell prompt
  2. I used an offset of the prompt height, but it should be promptHeight -1 since the current line is part of the prompt
  3. The bounds were wrong for cloning a marker below line 0 of the terminal, the offset should be - cursorY instead of 0

@cpendery cpendery marked this pull request as ready for review April 17, 2024 18:00
@cpendery cpendery requested a review from Tyriar April 17, 2024 18:01
@Tyriar

This comment was marked as outdated.

@Tyriar

This comment was marked as outdated.

@Tyriar
Copy link
Member

Tyriar commented Apr 19, 2024

Oh I was testing an old version 🤦

Tyriar
Tyriar previously approved these changes Apr 19, 2024
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@Tyriar Tyriar enabled auto-merge April 19, 2024 21:04
@Tyriar Tyriar merged commit 717c9df into microsoft:main Apr 19, 2024
6 checks passed
Tyriar added a commit that referenced this pull request Apr 20, 2024
…lacements"

This reverts commit 717c9df, reversing
changes made to 6294089.
Tyriar added a commit that referenced this pull request Apr 21, 2024
Revert "Merge pull request #209136 from cpendery/fix/improve-marker-p…
sergioengineer pushed a commit to sergioengineer/VimSCode that referenced this pull request Apr 23, 2024
…placements

fix: improve terminal marker placements on windows
sergioengineer pushed a commit to sergioengineer/VimSCode that referenced this pull request Apr 23, 2024
…-marker-placements"

This reverts commit 717c9df, reversing
changes made to 6294089.
sergioengineer pushed a commit to sergioengineer/VimSCode that referenced this pull request Apr 23, 2024
Revert "Merge pull request microsoft#209136 from cpendery/fix/improve-marker-p…
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants