Skip to content

fix: improve terminal marker placements on windows#209136

Merged
Tyriar merged 20 commits intomicrosoft:mainfrom
cpendery:fix/improve-marker-placements
Apr 19, 2024
Merged

fix: improve terminal marker placements on windows#209136
Tyriar merged 20 commits intomicrosoft:mainfrom
cpendery:fix/improve-marker-placements

Conversation

@cpendery
Copy link
Member

@cpendery cpendery commented Mar 29, 2024 •
edited
Loading

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

@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
Tyriar
Tyriar requested changes Apr 1, 2024
View reviewed changes
@cpendery
Copy link
Member Author

cpendery commented Apr 17, 2024 •
edited
Loading

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

@cpendery
style: drop unused import
33064ec 
Signed-off-by: Chapman Pendery <[email protected]>
@Tyriar
Copy link
Member

Tyriar commented Apr 17, 2024 •
edited
Loading

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

cpendery 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 🤔

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
Copy link
Member Author

cpendery commented Apr 17, 2024

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
@cpendery cpendery mentioned this pull request Apr 19, 2024
Merged
@Tyriar

This comment was marked as outdated.

Sign in to view
@Tyriar

This comment was marked as outdated.

Sign in to view
@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
View reviewed changes
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
Tyriar added a commit that referenced this pull request Apr 20, 2024
@Tyriar
Revert "Merge pull request #209136 from cpendery/fix/improve-marker-p…
9782bbc 
…lacements"

This reverts commit 717c9df, reversing
changes made to 6294089.
Tyriar added a commit that referenced this pull request Apr 21, 2024
@Tyriar
Merge pull request #210831 from microsoft/tyriar/revert/si
8c172fc 
Revert "Merge pull request #209136 from cpendery/fix/improve-marker-p…
@Tyriar Tyriar mentioned this pull request Apr 22, 2024
Closed
sergioengineer pushed a commit to sergioengineer/VimSCode that referenced this pull request Apr 23, 2024
@Tyriar @sergioengineer
Merge pull request microsoft#209136 from cpendery/fix/improve-marker-…
341ae24 
…placements

fix: improve terminal marker placements on windows
sergioengineer pushed a commit to sergioengineer/VimSCode that referenced this pull request Apr 23, 2024
@Tyriar @sergioengineer
Revert "Merge pull request microsoft#209136 from cpendery/fix/improve…
a86b392 
…-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
@Tyriar @sergioengineer
Merge pull request microsoft#210831 from microsoft/tyriar/revert/si
db4c30d 
Revert "Merge pull request microsoft#209136 from cpendery/fix/improve-marker-p…
@Tyriar Tyriar mentioned this pull request Apr 24, 2024
Closed
@cpendery cpendery mentioned this pull request Apr 25, 2024
Merged
6 tasks
@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.

Reviewers

@Tyriar Tyriar Tyriar approved these changes

@lramos15 lramos15 lramos15 approved these changes

Assignees

@Tyriar Tyriar

Labels

None yet

Projects

None yet

Milestone

April 2024

Development

Successfully merging this pull request may close these issues.

3 participants

@cpendery @Tyriar @lramos15