-
-
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
visible whitespace #1802
visible whitespace #1802
Conversation
I vote for on by default, this makes it fast easier to see when you are on the ending character, which will resolve a lot of confusion on how certain selection operations work. It suits the batteries included approach of helix better, IMO. |
Thanks for continuing #1218. There's a bug I'd encountered when I first made that PR where when the cursor is at the very end of the file (after all characters in the file), the spot the cursor is on is rendered as a space; the bug seems to still be present here. |
Looks like the space comes from here: helix/helix-term/src/ui/editor.rs Lines 343 to 346 in 05161aa
So I just pushed a commit that lets you configure which character that is and defaulted it to the braille blank (U+2800). It's a little brighter than a space, but I don't think it's very noticeable. This approach seems kinda hacky though, what do you think? |
Also |
actually it ended up being cleaner to detect when the highlight affected the trailing cursor and prevent substitution in that case: 45fd800 |
I wouldn't want this on by default but it should be easy to toggle at runtime. I had |
(So two config options: one for character map, one for on/off) |
I was also thinking about a generic toggle mode, so for example |
I added this to the setting command so we can say |
88e6e18
to
954d423
Compare
helix-term/src/ui/editor.rs
Outdated
surface, | ||
theme, | ||
highlights, | ||
&editor.config.whitespace, |
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.
Should we just store this within editor?
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.
Yeah I think there's a method or two in Editor (other than this one) that end up using the config in other ways as well. Seems wise to make that available so we don't need to pass it in all the time.
edit: actually I don't think we can have the editor config or whitespace config be a field on the EditorView struct, this call to render_text_highlights
looks static
helix-term/src/ui/editor.rs
Outdated
let text = doc.text().slice(..); | ||
|
||
let mut spans = Vec::new(); | ||
let mut visual_x = 0u16; | ||
let mut line = 0u16; | ||
let tab_width = doc.tab_width(); | ||
let tab = " ".repeat(tab_width); | ||
let tab = if whitespace.render.tab() == WhitespaceRenderValue::All { | ||
(1..tab_width).fold(whitespace.substitutions.tab.to_string(), |s, _| s + " ") |
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 think we might need to check the tab substitution width, if it's substituted with two-width characters then it will look twice as large. But I doubt anyone would want to do that.
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 it possible to have a multi-column char
? If so, we might be able to check for this when deserializing from config with helix_core::graphemes::grapheme_width
#[serde(untagged, rename_all = "kebab-case")] | ||
pub enum WhitespaceRender { | ||
Basic(WhitespaceRenderValue), | ||
Specific { |
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.
What is this Specific for? Why do we need to differentiate with basic?
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 comes from the original PR (#1218) - it's for this more advanced configuration: #1068 (comment)
I'm not too sure how valuable it is to have it be this configurable though. Maybe it'd be good enough to have a simple boolean that enables visible whitespace? You can also "turn off" visible whitespace for each kind of character by configuring substitutions to use spaces
[editor.whitespace.substitutions]
newline = " " # don't make newline visible
@sudormrfbin IIRC doom emacs have some triggering mechanism as well similar to what you said, maybe can steal from them. |
954d423
to
c9fc549
Compare
c32346b
to
038e6bb
Compare
d3ec589
to
3e2f361
Compare
3e2f361
to
31698c9
Compare
f168b4c
to
e40efd8
Compare
e40efd8
to
73084fb
Compare
Co-authored-by: Michael Davis <[email protected]>
73084fb
to
81f76cb
Compare
81f76cb
to
17e51b9
Compare
Example with this rebased on top of #1796:
Adds some configuration options to
config.toml
:And a new scope for themes
ui.virtual.whitespace
, which should be set to something slightly off-background.This makes way for a future
"editor.whitespace.render" = "selection"
as well, which could enable showing whitespace only in a selection rather than always.TODOs:
⍽
)ui.virtual
connects #1068
closes #1218
closes #688