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

Consider the GlyphWidth when calculate the postion of matched word in URL detecting #8124

Merged
3 commits merged into from
Nov 3, 2020

Conversation

comzyh
Copy link
Contributor

@comzyh comzyh commented Nov 1, 2020

Fix #8121
image

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.

References

PR Checklist

  • Closes Hyperlink detection: something wrong with handling wide characters. #8121
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo 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

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.

Validation Steps Performed

Manually Verified

@github-actions
Copy link

github-actions bot commented Nov 1, 2020

New misspellings found, please review:

  • chr
To accept these changes, run the following commands
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/5757ec679b03a4240130c3c53766c91bbc5cd6a7.txt
.github/actions/spell-check/expect/655f007265b351e140d20b3976792523ad689241.txt
.github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"Bopomofo CParams GENERATEPROJECTPRIFILE hhhh renamer rgus SGRXY xe xlang "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/085507936f96e1912dd868ff6099da17e1b8703f.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"chr Renamer "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/085507936f96e1912dd868ff6099da17e1b8703f.txt is added to your repository...'
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

@comzyh comzyh closed this Nov 2, 2020
@comzyh comzyh deleted the fix_full_width_glyph_in_url_autodetecting branch November 2, 2020 15:16
@comzyh comzyh reopened this Nov 2, 2020
@ghost ghost 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
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I could've sworn that we had a helper for figuring out how wide a given string is in cells, but I suppose this will work...

Actually, I think I'd be more comfortable if @miniksa had a chance to take a look at this and make sure that the character/glyph width math was being done right here.

@zadjii-msft
Copy link
Member

@msftbot make sure @miniksa signs off on this

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 2, 2020
@ghost
Copy link

ghost commented Nov 2, 2020

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

I'm OK with this for now. If it ends up becoming a performance problem, we'll revisit it.

The trick here is that on line 2413, the row.GetCharRow().GetText() call technically was aware already at that point of the width of every cell in the buffer and stripped that information out to give back a string.

But it has to be in a nice string format for std::wsregex_iterator to work.

So while I might be like "it's sort of annoying to re-parse the whole string again for the width of every character when we technically already knew that at some point above", I don't think I'm going to hold up this quick fix for that reason.

I think the increased difficulty of performing regex matching against not-a-string OR the carry of additional width data back out when text is retrieved is likely not worth it at this time.

@miniksa
Copy link
Member

miniksa commented Nov 3, 2020

I could've sworn that we had a helper for figuring out how wide a given string is in cells, but I suppose this will work...

Ehhhh we have like a GetALengthFromW which isn't exactly a cell count. And the act of storing the characters in the buffer is using this width thing to know how wide each character is going into the cells. But we don't strictly have this sort of all-at-once thing except in the DX renderer (which is ready to support NxM, but the text buffer is not.) At least as far as I can recall.

Copy link
Contributor

@PankajBhojwani PankajBhojwani left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@ghost ghost merged commit c2db1e9 into microsoft:main Nov 3, 2020
@ghost
Copy link

ghost commented Nov 11, 2020

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hyperlink detection: something wrong with handling wide characters.
4 participants