fix: don't print bidi isolates LRI, RLI, FSI, PDI

[adalinesimonian]

Summary of the Pull Request

Fixes #16574. Skips printing LRI, RLI, FSI, and PDI characters to the terminal.

References and Relevant Issues

Does not implement BIDI/RTL; that is out of scope, see #538. This is just a hotfix to stop spamming the console with undesired character printouts. Once BIDI support is implemented, this change will (maybe?) no longer be necessary.

Detailed Description of the Pull Request / Additional comments

See discussion starting at @lhecker's comment: #16574 (comment)

Validation Steps Performed

Built, launched console, verified LRI, RLI, FSI, and PDI characters are not printed.

PR Checklist

• Closes WT doesn't support U+2068 and U+2069 characters #16574
• Tests added/passed

[lhecker]

As discussed in the issue, personally I believe this a good approach for solving this. Assuming that we merge it as-is: Thank you for writing and testing the PR in my stead! ❤️

[adalinesimonian]

As pointed out by @j4james, the previous implementation caused some issues with rendering. The latest commit 5a9b861 moves logic out of the renderer and into the state machine, and adds a test. It also expands coverage to all explicit directional isolates, rather than just FSI and PDI.

[j4james]

Aside from the potential performance impact, this seems like a reasonable solution to me. However, if the core team prefers this to be handled in the renderer, I think your original solution could still be made to work, if instead of dropping the isolates, you replaced them with an invisible character with the same spacing semantics (e.g. a zero-width space).

So where you had if (...) continue; you would instead have something like if (...) emplace_back(L'\u200B'); else emplace_back(ch);

This also feels like a hack, but I thought it was worth mentioning as a possible solution if there are objections to your current approach.

[DHowett]

I apologize for I have only thought about this for roughly eight seconds.

If we strip the characters out before the make it into the buffer, they will not be available if the user copies that section of the text.

Is keeping the isolates intact for the clipboard important?

[lhecker]

Personally, I'd just fix the behavior of zero-width chars in the first column. I effectively prototyped this behavior for Edit's Framebuffer. The only problem is that we need to refactor out the Cluster thing out of the rendering code.

[adalinesimonian]

If we strip the characters out before the make it into the buffer, they will not be available if the user copies that section of the text.

Uh oh...

Is keeping the isolates intact for the clipboard important?

Yes, they direct how the text must be handled.

[adalinesimonian]

Personally, I'd just fix the behavior of zero-width chars in the first column. I effectively prototyped this behavior for Edit's Framebuffer. The only problem is that we need to refactor out the Cluster thing out of the rendering code.

So in other words, skip rendering but using the zero-width char approach @j4james suggested, but then also fix the zero-width char rendering bug? Should all that happen in this PR or in separate PRs?

[lhecker]

That would be separate PRs and fixing the zero-width thing would be my job because it's a bit difficult. 😅

[adalinesimonian]

Alright, in that case, I just moved the logic back into the renderer, but this time I'm rendering zero-width spaces instead so as not to actually end up with less characters rendered than went into the renderer.

I've tested:

• Isolate characters are not rendered (in any visible way), minus of course behaviour due to any bugs related to handling of zero-width characters.
• Using @j4james's suggested manual QA check for behaviour of columns, columns do not shift and act as expected.
• Copying the printed output correctly copies the original isolates in the text.

This does remove the test on the state engine; I'm still not sure if there is a place to add a test for this behaviour and if yes where the best place for it to be. Happy to write a test if there's a good spot for one.

[j4james]

@adalinesimonian Sorry for misdirecting you with the whole statemachine tangent.

@lhecker When you're looking at the zero-width fix, another issue I noticed is that zero-width characters alter the attributes of the cell that they "occupy", even when they aren't actually consuming any space. So if you have a red zero-width space followed by a green asterisk, the asterisk will actually be rendered in red.

echo -e '*\e[31m\U200B\e[32m*\e[m'

[adalinesimonian]

@adalinesimonian Sorry for misdirecting you with the whole statemachine tangent.

No problem, it's just the name of the game with software. Sometimes one ends up going down a tangent or two.

[j4james]

My approval doesn't really count, but this seems like a reasonable solution to me.

[DHowett]

this is excellent. thank you so much -- and thanks Leonard for the quick/easy patch too

[adalinesimonian]

Thanks to everyone for the support with this, glad to have helped. Is all that's left to let CI run and then the PR gets auto-merged? Is there a release schedule for how bug fixes go out? Not familiar with this repo just yet, pardon the questions.

[DHowett]

UGH. I keep forgetting that I need to manually trigger CI for community pull requests. This should have built and merged last night ☹️

In general, bug fixes like this will be cherry-picked to the stable and preview branches and released within a couple weeks. We've been focused on other projects lately, so the speed of those releases has dropped a bit. Sorry about that!

[DHowett]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[adalinesimonian]

Thanks for the info, I appreciate it!