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.

Sign up for GitHub

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

Jump to bottom

URL highlight broken when URL ends at the EOL #17065

Closed
bestis opened this issue Apr 16, 2024 · 3 comments · Fixed by #17120
Closed

URL highlight broken when URL ends at the EOL #17065

bestis opened this issue Apr 16, 2024 · 3 comments · Fixed by #17120
Assignees
@lhecker
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal. zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes.
Milestone
Terminal v1.21

Comments

@bestis
Copy link

bestis commented Apr 16, 2024

Windows Terminal version

1.19.10821.0

Windows build number

10.0.22631.3447

Other Software

No response

Steps to reproduce

  • Open Ubuntu terminal
  • Set your terminal to X cols
  • Run these echo's so that the amount of spaces make the middle one end in the EOL
echo 'https://developer.mozilla.org/en-US/docs/Web/CSS/letter-spacing'
echo '                 https://developer.mozilla.org/en-US/docs/Web/CSS/letter-spacing'
echo '                  https://developer.mozilla.org/en-US/docs/Web/CSS/letter-spacing'

  • Open Windows Powershell

  • Run similar echos

  • Open Command Prompt

  • Run similar echos without the quotes.

Expected Behavior

All the links open the right webpage.

Actual Behavior

  • Ubuntu, the middle link looses the last letter.
    image
  • Powershell, the middle link gets extra PS in the end.
    image
  • Command prompt, middle link looses the last letter
    image
@bestis bestis added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 16, 2024
@github-actions GitHub Actions
Copy link

Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one and thumbs upping the other issue to help us prioritize it. Thank you!

Closed similar issues:

Note: You can give me feedback by thumbs upping or thumbs downing this comment.

@bestis
Copy link
Author

bestis commented Apr 16, 2024

My example has no emoji, wide-characters, etc. So doesn't seem related to me.

@lhecker lhecker self-assigned this Apr 17, 2024
@lhecker
Copy link
Member

lhecker commented Apr 17, 2024

This likely occurs because the UTextAdapter.cpp code doesn't insert newlines between rows. This only becomes apparent when there's a newline at the exact end of the row because then there's no separator between the match (URL) and the remaining text.

@lhecker lhecker added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Product-Terminal The new Windows Terminal. Priority-3 A description (P3) labels Apr 17, 2024
@carlos-zamora carlos-zamora added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 17, 2024
@carlos-zamora carlos-zamora added this to the Terminal v1.21 milestone Apr 17, 2024
@lhecker lhecker mentioned this issue Apr 24, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Apr 29, 2024
@lhecker
Make UTextFromTextBuffer newline aware (#17120)
5b8eadb 
This PR achieves two things:
* When encountering rows with newlines (`WasForceWrapped` = `false`)
  we'll now copy the contents out of the row and append a `\n`.
  To make `utext_clone` cheap, it adds a reference counted buffer.
* Text extraction in `Terminal::GetHyperlinkAtBufferPosition`
  was fixed by using a higher level `TextBuffer::GetPlainText`
  instead of iterating through each cell.

Closes #16676
Closes #17065

## Validation Steps Performed
* In pwsh execute the following:
  ``"`e[999C`e[22Dhttps://example.com/foo`nbar"``
* Hovering over the URL only underlines `.../foo` and not `bar` ✅
* The tooltip ends in `.../foo` and not `.../fo` ✅
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Apr 29, 2024
DHowett pushed a commit that referenced this issue May 7, 2024
@lhecker @DHowett
Make UTextFromTextBuffer newline aware (#17120)
2b156d2 
This PR achieves two things:
* When encountering rows with newlines (`WasForceWrapped` = `false`)
  we'll now copy the contents out of the row and append a `\n`.
  To make `utext_clone` cheap, it adds a reference counted buffer.
* Text extraction in `Terminal::GetHyperlinkAtBufferPosition`
  was fixed by using a higher level `TextBuffer::GetPlainText`
  instead of iterating through each cell.

Closes #16676
Closes #17065

## Validation Steps Performed
* In pwsh execute the following:
  ``"`e[999C`e[22Dhttps://example.com/foo`nbar"``
* Hovering over the URL only underlines `.../foo` and not `bar` ✅
* The tooltip ends in `.../foo` and not `.../fo` ✅

(cherry picked from commit 5b8eadb)
Service-Card-Id: 92509615
Service-Version: 1.20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lhecker lhecker

Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal. zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes.
Projects
None yet
Milestone
Terminal v1.21
Development

Successfully merging a pull request may close this issue.

Make UTextFromTextBuffer newline aware microsoft/terminal
3 participants
@bestis @lhecker @carlos-zamora