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

Add rulers option #2060

Merged
merged 2 commits into from
Apr 20, 2022
Merged

Add rulers option #2060

merged 2 commits into from
Apr 20, 2022

Conversation

DeviousStoat
Copy link
Contributor

@DeviousStoat DeviousStoat commented Apr 10, 2022

Attempts at a solution for #1897

This adds a rulers option which is a list of column positions at which a color column should be drawn.
I added a ui.virtual.ruler theme value to set the theme of the color column. I am not sure if this is the best solution. I think this would need to be added to every theme.

I initially wanted to add the cursor column to this option as well but I thought it would be better as a different option as this needs to be drawn only on the focused window. And I didn't really find a good way to specify it in the option. I will probably open another PR for this soon if this sounds good to you.

The cool thing using the theme system is that we can set only the foreground color for the colorcolumn and I think it is actually really cool:

foreground

With rulers = [20] and "ui.virtual.ruler" = "red"

@sudormrfbin
Copy link
Member

An alternate name could be "ruler" instead of vim's colorcolumn.

})
.filter(|area| area.left() < inner.right())
})
.for_each(|area| surface.set_style(area, theme.get("ui.colorcolumn")));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can probably default this to red.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already ui.highlight that can be used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd scope this as ui.highlight.ruler so that it can be styled independently but have a fallback

Copy link
Contributor Author

@DeviousStoat DeviousStoat Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so neat! This dot notation fallback system is very powerful.
I allowed myself to add ui.highlight along with the new ui.highlight.ruler in the list of available theme keys in the documentation as I think it was missing.

@@ -42,6 +42,7 @@ hidden = false
| `completion-trigger-len` | The min-length of word under cursor to trigger autocompletion | `2` |
| `auto-info` | Whether to display infoboxes | `true` |
| `true-color` | Set to `true` to override automatic detection of terminal truecolor support in the event of a false negative. | `false` |
| `color-column` | List of column positions at which to display the color columns. | `[]` |
Copy link
Contributor

@pickfire pickfire Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be in editor but in languages.toml, people might want to set a different color column for different themes? For example, rust might have one on 80 and one on 100. Or maybe we can have this on both side?

Copy link
Contributor Author

@DeviousStoat DeviousStoat Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds very useful but I personally really like having a global color column. It would be tedious to have to set it up for each language. So I tried to go with both. The language one having the priority but falling back to the editor one if not set.

@pickfire
Copy link
Contributor

An alternate name could be "ruler" instead of vim's colorcolumn.

Where do you see that being used? I think ruler might be a bit confusing since people might mistake it for line number.

@archseer
Copy link
Member

An alternate name could be "ruler" instead of vim's colorcolumn.

ruler is actually a different thing: https://codeyarns.com/tech/2010-11-28-vim-ruler-and-default-ruler-format.html

@sudormrfbin
Copy link
Member

I picked ruler straight from VSCode (seems like sublime does too), and it sounds more clearer in intent IMO as something used to draw straight lines (vertical lines here). And we do deviate from vim's naming scheme where it makes sense (eg. cursor-shape vs nvim's guicursor).

@the-mikedavis
Copy link
Member

I think ruler is a strange name for that widget in vim. To me ruler implies some straight line you might use for alignment (or measurement I suppose) which fits better with this feature

@archseer
Copy link
Member

archseer commented Apr 13, 2022

Okay I agree, let's rename this to ruler/rulers

@DeviousStoat DeviousStoat force-pushed the color-columns branch 2 times, most recently from 44f5cdc to ecd5f7b Compare April 14, 2022 01:02
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@archseer
Copy link
Member

Ah, looks like there's a small conflict, can you resolve?

Comment on lines 231 to 232
| `ui.highlight` | |
| `ui.highlight.ruler` | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ui.virtual namespace used in #1802 and #1796 might be a better fit here.

Comment on lines 171 to 184
rulers
.iter()
.filter_map(|ruler| {
ruler
.checked_sub(1 + view.offset.col as u16)
.map(|x| {
viewport
.with_height((view.last_line(doc) - view.offset.row + 1) as u16)
.clip_left(x)
.with_width(1)
})
.filter(|area| area.left() < viewport.right())
})
.for_each(|area| surface.set_style(area, theme.get("ui.highlight.ruler")));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rulers
.iter()
.filter_map(|ruler| {
ruler
.checked_sub(1 + view.offset.col as u16)
.map(|x| {
viewport
.with_height((view.last_line(doc) - view.offset.row + 1) as u16)
.clip_left(x)
.with_width(1)
})
.filter(|area| area.left() < viewport.right())
})
.for_each(|area| surface.set_style(area, theme.get("ui.highlight.ruler")));
rulers
.iter()
// View might be horizontally scrolled, convert from absolute distance
// from the 1st column to relative distance from left of viewport
.filter_map(|ruler| ruler.checked_sub(1 + view.offset.col as u16))
.filter(|ruler| ruler < &viewport.width)
.map(|ruler| viewport.clip_left(ruler).with_width(1))
.for_each(|area| surface.set_style(area, theme.get("ui.highlight.ruler")))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also pull out the theme.get call so we're not re-fetching on every iteration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks a lot nicer! There is just one thing I am wondering about. You dropped the with_height. Is that on purpose? Because it draws the ruler on the whole screen instead of stopping at the last line of text

height-ruler

Is that the wanted behaviour?

Isn't it a bit nicer with .map(|ruler| viewport.clip_left(ruler).with_width(1).with_height((view.last_line(doc) - view.offset.row + 1) as u16)?

height-ruler

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally like the version which extends to the whole screen since it feels consistent (not a strong argument for a vertical colored line I know :)

@DeviousStoat DeviousStoat changed the title Add color_column option Add rulers option Apr 17, 2022
@archseer archseer merged commit 5d5b6ba into helix-editor:master Apr 20, 2022
sudormrfbin pushed a commit that referenced this pull request Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants