-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
feat(editor): add support for trailing whitespace rendering #7215
base: master
Are you sure you want to change the base?
Conversation
4533212
to
a4c0fff
Compare
6d48eea
to
4ee444a
Compare
Adds a new render configuration value `Trailing`, which can be used to selectively enable trailing whitespace of certain whitespace characters.
helix-term/src/ui/document.rs
Outdated
self.viewport.x + offset as u16, | ||
self.viewport.y + position.row as u16, | ||
&trailing_whitespace[char_to_byte_idx(&trailing_whitespace, begin_at)..], | ||
style, |
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.
This will messup diagnostic and selection. The style can frequently differ, you should only be updating the text while leaving cell style unchanged
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.
How can I render something without being forced to provide a style?
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.
I'm still not sure how to apply this feedback. I've changed the code to always apply whitespace_style
though. I think this is basically the last pending thing to address.
@pascalkuthe Would it be possible to the next round of review on this? 🙏 It's a feature I have been running a fork for and it would be great to see it included in the main editor, I'm sure @alevinval feels similarly :) |
When testing this I noticed that with many themes the trailing whitespace indicators are not particularly visible, which makes sense since they share a style with the "all" whitespace render feature. However, it's counterproductive if the goal is to have a very clear visible indicator of trailing whitespace. Is it worth considering a separate style class for this? |
5d30aa2
to
ce08a77
Compare
Helix is handling narrow non-breaking spaces, update trailing whitespace tracker to take them into account
Hi @kmicklas, any pointers on where I should look in the codebase to land a custom style? So far I've pushed to keep this PR up-to-date with the latest master changes, which now also deal with narrow non-breaking spaces. |
@kmicklas I realize now that adding the style appears to be quite trivial. I've just pushed 970122f, and with the following update on the config of my current theme: diff --git a/runtime/themes/onelight.toml b/runtime/themes/onelight.toml
index e395716d..2f089060 100644
--- a/runtime/themes/onelight.toml
+++ b/runtime/themes/onelight.toml
@@ -138,6 +138,7 @@
"ui.virtual.ruler" = { bg = "grey-200" }
"ui.virtual.wrap" = { fg = "grey-500" }
"ui.virtual.whitespace" = { fg = "grey-400" }
+"ui.virtual.trailing_whitespace" = { fg = "grey-300", bg = "red" }
"ui.virtual.indent-guide" = { fg = "grey-500" }
"ui.virtual.inlay-hint" = { fg = "grey-500" }
"ui.virtual.inlay-hint.parameter" = { fg = "grey-500", modifiers = ["italic"] } I presume I don't have to update any theme here, and that theme owners can choose how to best style it. |
Hi, just putting here that I'd love this feature -- I write a lot of code for my job that I can't run a formatter on (because we don't have consistent formatting, or because it's some in-house language), and having this display immediately without needing to scour through the whole thing with all the whitespace symbols on or using git diff would be great. |
I've been using this, and I greatly appreciate the feature. I did notice an issue (though I am not meaning to hold up a potential merge): If a line is truncated at the edge of the editor window, any spaces that happen to be at the end are flagged as trailing. This disappears once you scroll to the right. |
Can this feature be rebased on current HEAD instead of using merge tag? |
This works good, will this be merged on main? |
Long time no code, I've updated this branch to latest |
|
Hey, inside trailing_whitespace.rs
For grapheme_tab_width should be char_to_byte_idx(&palette.tab, palette.tab.len()). The default 1 value renders incorrectly when the character for tab is also space. Indeed there are some other bugs but I'm happy with current changes. |
Works well for me, can you also add the new value into the docs https://github.com/helix-editor/helix/blob/master/book/src/editor.md#editorwhitespace-section |
If there is some whitespace and your cursor is on the whitespace, it will become fully highlighted So naturally I would expect that if I pressed So I think it makes sense if the entire whitespace didn't get highlighted, OR the highlighted region was treated as a selection and would delete the whitespace |
@gamma-delta For your use case, it might be even better to configure a custom formatter that only trims trailing whitespace. Less annoying visually, less manual work. A |
Context
Adds a new render configuration value
Trailing
, which can be used to selectively enable trailing whitespace of certain whitespace characters as requested in #2719