Conversation
| export interface IInputHandler { | ||
| parse(data: string): void; | ||
| print(data: string, start: number, end: number): void; | ||
| setWcwidthOptions(opts: {ambiguous?: 0 | 1 | 2, custom?: {[key: number]: 0 | 1 | 2}}): void; |
There was a problem hiding this comment.
Is the idea to expose this as a setting? What other terminals have you checked that also do this?
There was a problem hiding this comment.
Well the ambiguous setting is comparable to the setting in iterm2, although they only offer to set full width on or off (here 2 or 1, we still support the current "default" by omitting this setting). It might be useful to expose this so people can hot patch the width of ambiguous chars if they encounter issues. I think VTE also supports this by an env variable. There is even a test file, that will show the difference (just open the attachment 00example.txt here mintty/mintty#88 (comment) in an editor in xterm.js with ambiguous unset, set to 1 and set to 2). I also gonna add some tests to show/test the difference.
The custom setting might be helpful internally, if the systems wcwidth does not agree with our implementation - we could simply overload it with the system setting to avoid cursor and line ending problems. I would not expose this the user since it might break more than it will help. Still we could offer an addon or something similar to do the nasty low level stuff.
Local terminals are normally not affected by this since they can use the systems wcwidth (and will have automatically the same settings). We cant do that in a browser component since we have no access to the C land. In VSCode (and similar "local" terminals) this could be achieved by some additional node package with access the system wcwidth (some C++ binding), in the demo it gets more tricky to load those data, maybe by some addon with a server addition.
See also my comment here #1467 (comment) which kinda addresses this problem.
There was a problem hiding this comment.
While it might be useful to allow someone to pass a custom dictionary, in the case where we want to match with the system's wcwidth, it might be better if we can instead pass a function, because that matches the API POSIX gives us.
So, I'd recommend that the option be supplied as custom: (key: number) => 0 | 1 | 2 | undefined, where undefined causes fall-through behavior. If someone wants to use an object, then they can implement it as a function stub (e.g. (key) => {123: 2}[key]).
|
Closing this for now since it needs some more thinking about the right way to deal with the unicode mess behind, see #1467 (comment) for further details. |
This PR addresses 1. and 2. of issue #1467.
No tests yet.
@Tyriar I made as little as possible changes to the existing wcwidth implementation. To still be able to use the closured lookup table I simply extended the old factory like design. Therefore it looks a bit messy now, I can refactor it to a more readable flat design if needed.
Edit:
I was not sure where to put the set options method and added it to
InputHandlerfor now.Edit2:
The options method understands 2 settings:
{charCode: width}. This setting overrides any other width rule. Not applicable for ASCII chars due a performance optimization (should never be an issue).The factory method understands 2 more settings
nulandcontrol. I did not expose them since it is not a good idea to change their behavior.