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

Batch RTL runs to ensure proper draw order #7190

Merged
merged 5 commits into from
Aug 7, 2020
Merged

Batch RTL runs to ensure proper draw order #7190

merged 5 commits into from
Aug 7, 2020

Conversation

schorrm
Copy link
Contributor

@schorrm schorrm commented Aug 5, 2020

Consecutive RTL GlyphRuns are drawn from the last to the first.

References

#538, #7149 , all those issues asking for RTL closed as dupes.

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Manual test.
  • I've discussed this with core contributors already. See description below.

Detailed Description of the Pull Request / Additional comments

As @miniksa suggested in a comment on #7149 -- handle the thingy on the render side.
If we have GlyphRuns abcdEFGh, where EFG are RTL, we draw them now in order abcdGFEh.
This has ransom-noting, because I didn't touch the font scaling at all. This should fix the majority of RTL issues, except it doesn't fix issues with colors, because those get split in the TextBuffer phase in the renderer I think, so they show up separately by the GlyphRun phase.

Validation Steps Performed

image

@miniksa
Copy link
Member

miniksa commented Aug 5, 2020

except it doesn't fix issues with colors, because those get split in the TextBuffer phase in the renderer I think, so they show up separately by the GlyphRun phase.

That's completely true. And I've got a mental backlog item to make it so every line in the DxRenderer is collected up completely (including all the colors) into a single CustomTextLayout prior to drawing the whole line at once. If I prioritized that, it would fix it, I believe, in combination with this PR.

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 love it. Thank you.

@miniksa miniksa added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Needs-Second It's a PR that needs another sign-off labels Aug 5, 2020
@ghost ghost requested review from zadjii-msft, carlos-zamora, DHowett and leonMSFT August 5, 2020 20:27
@miniksa miniksa changed the title Comprehensive fix for RTL GlyphRun behavior Batch RTL runs to ensure proper draw order Aug 5, 2020
@miniksa
Copy link
Member

miniksa commented Aug 5, 2020

Updated title and reduced description body to read more easily in git log.

Copy link
Member

@DHowett DHowett 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 blocking this just to run out the discussion on whether we should add a config flag to reintroduce the other behavior for users who primarily use guest-controlled RTL. Technically this looks fine :)

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 6, 2020
@schorrm
Copy link
Contributor Author

schorrm commented Aug 6, 2020

The prior behavior is just capricious. Adding would be a new flag (I think in effect, split every Hebrew char into its own GlyphRun and avoid this reorder phase). But this isn't breaking any sort of prior behavior that worked.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 6, 2020
@ghost ghost requested a review from PankajBhojwani August 6, 2020 08:32
@schorrm
Copy link
Contributor Author

schorrm commented Aug 6, 2020

I think that flag could just zero out bidiLevel before we start drawing. It's not like that is the current behavior though, so nothing is really being lost by this PR.

@schorrm
Copy link
Contributor Author

schorrm commented Aug 6, 2020

@miniksa I made a small change to the commit message (from last to first) since I realized that my earlier one was too confusing.

@schorrm schorrm requested a review from DHowett August 6, 2020 15:45
@miniksa
Copy link
Member

miniksa commented Aug 6, 2020

@DHowett for this one specifically, batching the RTL runs together is behavior that the default IDWriteTextLayout would have been doing for us if I didn't roll the CustomTextLayout. Having them be drawn as a unit so the origins are offset is correct. We wouldn't flag this specific PR on and off.

We would do, as @schorrm says, something like zeroing/ignoring all bidiLevel flags for a guest-controlled RTL scenario.

@DHowett
Copy link
Member

DHowett commented Aug 6, 2020

Okay. I don't want to ship 1.3 without a flag that gives the users an escape hatch (config flag) to force LTR when they're using a guest application that tries to handle RTL on its own (by assuming LTR.) It doesn't have to be in this PR, but if it regresses vim -H without the ability to opt into a behavior that works with vim -H by the time we ship, we'll revert it and put it back on the table for 1.4. 😄

@DHowett
Copy link
Member

DHowett commented Aug 6, 2020

Does this replace #7149, require it or augment it?

@schorrm
Copy link
Contributor Author

schorrm commented Aug 6, 2020

So I would say it actually doesn't regress vim -H, barring an incredibly bizarre font fallback accident to make it work.
Re #7149: in terms of fixing RTL this replaces it. However, 7149 may be worthwhile to adapt into a ransom note fix on its own.

@schorrm
Copy link
Contributor Author

schorrm commented Aug 6, 2020

We didn't support ConHost behavior. If you had a janky enough font setup it might split enough GlyphRuns to make it kind of look like that sometimes. Sometimes.

@schorrm
Copy link
Contributor Author

schorrm commented Aug 6, 2020

If you can tell me how I can get a flag from the JSON, I'm happy to add this behavior, but I think it makes sense to open that as a separate PR.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Just gotta get the builds passing now 😄

@github-actions
Copy link

github-actions bot commented Aug 7, 2020

New misspellings found, please review:

  • abcd
  • EFG
  • EFGh
  • GFEh
To accept these changes, run the following commands
perl -e '
my @expect_files=qw('".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('"atg BKMK colo consoleaccessibility Dst emptybox FFF Filledbox fullcolor hyperlink IDefault minmax NRCS phsl popclip remoting rerendered ROWSTOSCROLL SCS shobjidl stdarg stddef stdlib terminalnuget Tofill unfocuses xab xb xbc xca xce xdd xffff "');
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/af1ff90dc512c83c902762b02f284c1c61603b4a.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"abcd dst EFG EFGh EMPTYBOX GFEh nrcs Remoting Scs Shobjidl "');
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/af1ff90dc512c83c902762b02f284c1c61603b4a.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.

@schorrm
Copy link
Contributor Author

schorrm commented Aug 7, 2020

Okay, take a look at the last. That's not on me, that's a runtime error in the CI I think.

@schorrm
Copy link
Contributor Author

schorrm commented Aug 7, 2020

You can see it passed x86 which was the one it was failing, and then it failed x64 because of memory starvation while compiling something unrelated.

@github-actions
Copy link

github-actions bot commented Aug 7, 2020

New misspellings found, please review:

  • abcd
  • EFG
  • EFGh
  • GFEh
To accept these changes, run the following commands
perl -e '
my @expect_files=qw('".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('"atg BKMK colo consoleaccessibility Dst emptybox FFF Filledbox fullcolor hyperlink IDefault minmax NRCS phsl popclip remoting rerendered ROWSTOSCROLL SCS shobjidl stdarg stddef stdlib terminalnuget Tofill unfocuses xab xb xbc xca xce xdd xffff "');
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/af1ff90dc512c83c902762b02f284c1c61603b4a.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"abcd dst EFG EFGh EMPTYBOX GFEh nrcs Remoting Scs Shobjidl "');
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/af1ff90dc512c83c902762b02f284c1c61603b4a.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.

@schorrm
Copy link
Contributor Author

schorrm commented Aug 7, 2020

That's nice. It recovered the memory issues. Do I have to do the spellcheck thingy?

@DHowett
Copy link
Member

DHowett commented Aug 7, 2020

I’d like that :)

@schorrm
Copy link
Contributor Author

schorrm commented Aug 7, 2020

I did the spelling thingy.

@zadjii-msft
Copy link
Member

(I'm consciously leaving this for @DHowett to hit the merge button, since he was the last one with concerns about it)

@DHowett DHowett merged commit 60b44c8 into microsoft:master Aug 7, 2020
@DHowett
Copy link
Member

DHowett commented Aug 7, 2020

Thanks @schorrm!

DHowett pushed a commit that referenced this pull request Aug 11, 2020
Consecutive RTL GlyphRuns are drawn from the last to the first.

References
#538, #7149, all those issues asking for RTL closed as dupes.

As @miniksa suggested in a comment on #7149 -- handle the thingy on the
render side.

If we have GlyphRuns abcdEFGh, where EFG are RTL, we draw them now in
order abcdGFEh.

This has ransom-noting, because I didn't touch the font scaling at all.
This should fix the majority of RTL issues, except it *doesn't* fix
issues with colors, because those get split in the TextBuffer phase in
the renderer I think, so they show up separately by the GlyphRun phase.

(cherry picked from commit 60b44c8)
@ghost
Copy link

ghost commented Aug 13, 2020

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants