-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Revamp wcwidth #1470
Closed
Closed
Revamp wcwidth #1470
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
, whereundefined
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]
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bgw Yupp thats better, gonna change it.