Switch Unicode Escaping to a VSCode-like system

[zeripath]

This PR rewrites the invisible unicode detection algorithm to more
closely match that of the Monaco editor on the system. It provides a
technique for detecting ambiguous characters and relaxes the detection
of combining marks.

Control characters are in addition detected as invisible in this
implementation whereas they are not on monaco but this is related to
font issues.

Close #19913

Signed-off-by: Andrew Thornton art27@cantab.net

Screenshots

[zeripath]

I've marked this as 1.17 but happy to move this to 1.18

[lunny]

I've marked this as 1.17 but happy to move this to 1.18

Since we are rushing for 1.17, I moved this to v1.18 because I think we need more time to discuss here.

[DarrenPIngram]

May I check, would this resolution stop me getting errors in the editor ( 1.17.0+rc1) when adding a space,

e.g.

text text (2020) [AU]
I get the nasty yellow symbol because it does not like the space between (2020) and (AU] yet it is a space freshly issued from my space bar.

Other spaces, using the same method, are not upsetting things.

[zeripath]

This is is not actually changing monaco, the editor, but instead copying the way it detects ambiguous unicode string into our file viewer.

I think you need to give us a screenshot of your problem though.

[zeripath]

@silverwind are you able to take a look at this - this now matches vscode's behaviour.

[silverwind]

Seems to work well so far. Can't really comment much on the code, but i'm happy to get this on 1.18 rather sooner than later, so it gets more testing until release.

[DarrenPIngram]

I think you need to give us a screenshot of your problem though.
Thank you for the response and my apologies for the delayed response, the alert seemed to get filed with promotions on Google. I found the thread when searching for a possible reason for the issue in my quite new installation.

In a document (default options, nothing "clever" done at least) I can't always reproduce it, in other words you can use spaces sometimes without a problem.

In the example below, I reopened the document, added the second line (line 2) and typed the same text with a regular space (from the space bar). You then got the square box and a hover mentioning unicode. This is displayed in the "ready" page too. I do not recall doing any cut and paste in the original line, and definitely not in the second line that I just added.

Does this help? Are you then able to steer me in a direction? Thanks.

[wxiaoguang]

Not sure whether the <code> line could be improved. update: done in new commits

And one more thing, not sure whether the double-encoded ambiguous/ambiguous.json is done by purpose: update: there are more comments, clear.

Overall LGTM.

[zeripath]

Not sure whether the <code> line could be improved.

I'm just looking at it... I think we can just use a template.

And one more thing, not sure whether the double-encoded ambiguous/ambiguous.json is done by purpose: "{\"_common\":[8232,32,8233... (why double-encoded)

maybe the encoded JSON could be {"_common": [8232,32,8233...

This file is directly taken https://github.com/hediet/vscode-unicode-data/blob/main/out/ambiguous.json

---

Don't merge just yet OK I've done some more extensive subtemplating.

[delvh]

What exactly prevents this PR from being merged?
I see 4 approvals, and no open comments...

[zeripath]

I've bitten the bullet and merged it. There will be some complaints.

[MaeIsBad]

Here come the complaints!
This commit breaks the routers/web/repo/commit.go:Diff route, causing it to return a 500 internal server error, because of locale being undefined in templates/repo/diff/escape_title.tmpl file

[zeripath]

Apologies this is precisely why I was reluctant to write subtemplates in the first place.

[silverwind]

We should really refactor .locale to a global helper locale, it's just a pain in the ass to have this function passed down as "data" when it doesn't have to be. Makes subtemplating a lot harder.

[zeripath]

@silverwind well the problem here is that you can't do that (easily) with go templates because there is no way of setting Funcs on a per execution basis and the Funcs are stored in the templates themselves.

I've put a proposal up to go itself begging them to add methods to allow passing of execution templates (even providing a PR to do it.)

The current situation is absolutely intolerable though.

I don't think there is any better way of describing it than that. What we're being forced to do is incredibly cumbersome and extremely error prone.

If they reject this proposal and persist in suggesting Clone Funcs Execute - which just isn't feasible or performant - or suggest that we have to continue passing contextual rendering functions as data we should reconsider our use of this package.

We could come with a common passing system, e.g. always have common things in common (or root or context), so that locale is always in $.comon.locale in every template (even non subtemplates) but this is still ugly and really quite poor.

If we can't or won't tolerate that then our options then are to either fork the packages, to either add the proposed methods or to properly separate out the template collection, a template, the parser and the executor, (This would allow us to make a potentially lock free executor and template collection - or even leverage code generation to precompile templates to code).

Another option is to find a different template language. (Hopefully one shared on the frontend too.)

We're going to hit this problem more and more though as we try to make our templates easier to read and to handle.

[silverwind]

because there is no way of setting Funcs on a per execution basis and the Funcs are stored in the templates themselves.

You mean helper functions are not really "global" as I thought? At least in original Handlebars (registerHelper) they are, so every subtemplate has access to all of them.

[zeripath]

In some ways they're too global.

They're stored in the templates so if you want to change the func to have a per execution function you have to Clone the template at time of execution then set a new funcmap with the required Funcs and then Execute. This is expensive precisely because Funcs are stored in templates and they have to be concurrency safe.

If you don't do the clone first off you're going to change the templates for everyone using them.

What we need is something like Template.ExecuteFuncMap where you could pass in a map of functions specific for this execution that would override the currently template defined functions but I just don't know if it is going to be accepted.

[IzzySoft]

Can this feature be "switched off" somehow? I get the warning box on several pages in a wiki, but the escape button neither produces the triangles nor marks which characters it finds offending, so I've no idea what (if anything) to clean up. Maybe this issue exists only on Wiki pages: I found one other file in a "normal repo" (i.e. not wiki) which had the warning box, and there escaping worked.

In all the cases I found however, those characters were intended (non-breakable spaces, single quotes and such). So that box is rather confusing or at best annoying there in my cases (I understand it can be quite useful in other cases). So there should be a way to disable it either per install, per project or both.

Btw: I only noticed that box when I upgraded to 1.18.0, it was not there before in case this matters. And yes, for my local instance I certainly could simply disable it via a CSS rule:

div.error.unicode-escape-prompt, div.warning.unicode-escape-prompt { display:none !important; }

But one might wish to toggle it on/off easily from within the browser, so checking remains possible when required/intended.