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

Hyperlink detection: something wrong with handling wide characters. #8121

Closed
DHowett opened this issue Nov 1, 2020 · 1 comment · Fixed by #8124
Closed

Hyperlink detection: something wrong with handling wide characters. #8121

DHowett opened this issue Nov 1, 2020 · 1 comment · Fixed by #8124
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@DHowett
Copy link
Member

DHowett commented Nov 1, 2020

I guess there is something wrong with handling wide characters.
image

Originally posted by @comzyh in #7691 (comment)
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 1, 2020
comzyh added a commit to comzyh/terminal that referenced this issue Nov 1, 2020
@comzyh
Accumulate the width of glyphs to calculate the coordinate of matched…
2529832 
… words. fix microsoft#8121
@comzyh comzyh mentioned this issue Nov 1, 2020
Merged
6 tasks
@ghost ghost added the In-PR This issue has a related PR label Nov 1, 2020
@zadjii-msft zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Nov 2, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 2, 2020
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 2, 2020
@ghost ghost added In-PR This issue has a related PR and removed In-PR This issue has a related PR labels Nov 2, 2020
@ghost ghost closed this as completed in #8124 Nov 3, 2020
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Nov 3, 2020
ghost pushed a commit that referenced this issue Nov 3, 2020
@comzyh
Consider the GlyphWidth when calculate the postion of matched word in…
c2db1e9 
… URL detecting (#8124)

Fix #8121
![image](https://user-images.githubusercontent.com/1068203/97811235-2081ca80-1cb4-11eb-82bd-1ddaf15c757c.png)


<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request

When calculating the position of the matched pattern, consider the width of the characters.

However, if there are some wide glyphs in the detected hyperlink(not possible for now, for the existing regex will not match wide-character?). The repeated character in the tooltip is not fixed by this PR.

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [ ] Closes #8121
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

When calculating the coordinate of the match in #7691, it simply uses the `prefix.size()` as the total prefix width on the screen.

This PR fixes that behavior.

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Manually Verified
@ghost
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #8124, which has now been successfully released as Windows Terminal Preview v1.5.3142.0.:tada:

Handy links:

@zadjii-msft zadjii-msft mentioned this issue Jan 6, 2021
Closed
@comzyh comzyh mentioned this issue Feb 27, 2024
Merged
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Consider the GlyphWidth when calculate the postion of matched word in URL detecting comzyh/terminal
2 participants
@DHowett @zadjii-msft