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

helix-tui: Skip line-endings in Buffer::set_string #10962

Closed
wants to merge 1 commit into from

Conversation

the-mikedavis
Copy link
Member

The update of unicode-width to 0.1.13 changes the width of line endings to 1 (from 0), so we need to detect line-endings when skipping graphemes in Buffer::set_string_truncated_at_end.

Connects #10950

The update of unicode-width to 0.1.13 changes the width of line endings
to 1 (from 0), so we need to detect line-endings when skipping graphemes
in `Buffer::set_string_truncated_at_end`.
@RoloEdits
Copy link
Contributor

RoloEdits commented Jun 14, 2024

Tried this fix, but I still had the same issues. This branch is what I build my custom build with. I cherrypicked the commit from this pr right on top.

This can be built with a simple command via https://github.com/casey/just

just

Which just expands to this, if you'd like:

RUSTFLAGS='-C target-cpu=native' cargo build --profile opt --bin hx

These are my rust-analyzer settings. (Most of it is still hold overs and random testing 😆)

[language-server.rust-analyzer]
config.check.command = "clippy"
config.check.extraArgs = ["--", "-W", "clippy::pedantic", "-W", "clippy::nursery", "-W", "clippy::cargo"]
cargo.buildScripts.rebuildOnSave = true
completion.fullFunctionSignatures.enable = true
cargo.features = "all"
hover.actions.references.enable = true
hover.show.structFields = 7
hover.show.traitAssocItems = 7
hover.memoryLayout.enable = true
hover.memoryLayout.alignment = "both"
hover.memoryLayout.niches = true
hover.memoryLayout.offset = "both"
hover.memoryLayout.size = "both"
highlightRelated.breakPoints.enable = true
highlightRelated.closureCaptures.enable = true
highlightRelated.exitPoints.enable = true
highlightRelated.references.enable = true
highlightRelated.yieldPoints.enable = true
inlayHints.bindingModeHints.enable = true
inlayHints.chainingHints.enable = true
inlayHints.closingBrace.enable = true
inlayHints.closureCaptureHints.enable = true
inlayHints.closureReturnTypeHints.enable = "always"
inlayHints.discriminantHints.enable  = "always"
inlayHints.implicitDrops.enable = true
inlayHints.parameterHints.enable = true
inlayHints.lifetimeElisionHints.enable = "always"
inlayHints.lifetimeElisionHints.useParameterNames = true
inlayHints.rangeExclusiveHints.enable = true
inlayHints.expressionAdjustmentHints.enable = "always"
typing.autoClosingAngleBrackets.enable = true
semanticHighlighting.doc.comment.inject.enable = true
typing.continueCommentsOnNewline = true

@RoloEdits
Copy link
Contributor

RoloEdits commented Jun 14, 2024

It could also be that #6417 has to make fixes for the new changes as well, this has just been the most apparent way to show the issue at hand. Hate to make you go on a wild goose chase like this, kind of unfortunate that this wasn't considered a breaking change by unicode-width. There could be many areas where this could unknowingly crop up.

@the-mikedavis
Copy link
Member Author

For inline diagnostics I believe it fixes it to copy this same width == 0 || str_is_line_ending(s) to the other if width == 0 checks in Buffer. But I found other breakages as well, for example opening a PDF causes many more artifacts than on master. Let's revert the unicode-width update for now, the update doesn't seem to benefit us in any way as far as I can tell anyways

@the-mikedavis the-mikedavis deleted the unicode-width-line-ending-fixes branch June 14, 2024 19:36
@RoloEdits
Copy link
Contributor

Sounds good.

@the-mikedavis
Copy link
Member Author

Actually I think unicode-width's intepretation of this as non-breaking is correct since it's a bug-fix. It's just unfortunate that tui-rs (the crate that historically became helix-tui and ratatui) assumes line-endings are zero-width. I'm not well read on the unicode width vs. terminal symbol-width issue though so maybe @pascalkuthe has a more informed opinion here

@pascalkuthe
Copy link
Member

Newlines are treated as zero width by all terminal emulators. For us they are width one because we actually show them but that is very helix specific.

Our width definition must align with the terminal emulator to avoid bugs (or we would need to start adding all kinds of special casing to the rendering).

I have my own implementation of unicode width in a repo. That crate also allows customizing which unicode version to use (also differs between emulators... fun). I didn't think it was worth bringing it in. I wrote.it originally because I thought I could solve the issues for multicodepoint graphemes but there is not standard for that and emulators can't agree on anything so it doesn't solve that problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants