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

feat: Use custom character to draw editor rulers #9256

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dylangiles
Copy link

This pull request adds functionality requested in #5190.

The user may specify a character that is used when rendering rulers, instead of colouring a line in the editor. The default behaviour is retained - if the user does not specify the ruler character config, the original behaviour is used. This functionality will be useful for users that want to, for example, use a box-drawing character to draw the ruler.

Please feel free to make suggestions and comments on my pull request, as it is my first time contributing to Helix. Additionally, I would like to thank the Helix project for making an awesome piece of software that has helped with my daily tasks.

Example config:

[editor]
ruler = [70]
ruler-char = '│'

Screenshot:

Screen Shot 2024-01-06 at 12 12 08 pm

@pascalkuthe
Copy link
Member

hmm I am not sure about this one. This will draw over any rendered text (so if you write to the ruler the text there is hidden). A similar issues already occurs with rulers hiding cursors.

IMO the right solution is to use the new infrastructure in #6417 and render the rulers before the text and the ruler highlights after the text highlights but before the selection highlights

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I'm also unsure of this but for a different reason. This will introduce some issues with the theming for rulers, sort of opposite of the problem we have now in #5721. Themes should define rulers with background colors rather than foreground colors. If you configure a ruler-char with this change though you want the ruler's foreground to be themed rather than the background though. So themes will either look wrong if you have ruler-char set or if you don't.

@@ -273,6 +273,11 @@ pub struct Config {
pub terminal: Option<TerminalConfig>,
/// Column numbers at which to draw the rulers. Defaults to `[]`, meaning no rulers.
pub rulers: Vec<u16>,

/// If set, use this character to draw the editor's rulers.
#[serde(rename = "ruler-char")]
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary because the struct is tagged with #[serde(rename_all = "kebab-case")]

@@ -149,7 +149,6 @@ pub struct LanguageConfiguration {
/// global setting.
#[serde(default, skip_serializing, deserialize_with = "deserialize_auto_pairs")]
pub auto_pairs: Option<AutoPairs>,

Copy link
Member

Choose a reason for hiding this comment

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

This should be reset

Copy link
Member

Choose a reason for hiding this comment

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

No need to reformat the tables, let's leave them as-is. Reformatting them inflates the diff and may introduce conflicts with other PRs that touch these docs

@codingjerk
Copy link

Can I continue working on this PR? I want to use character rulers myself a lot.

@the-mikedavis, what are the problems right now?

  1. Styling issues
  • Reformatted table @ book/src/configuration.md
  • Unnecessary removal of newline @ helix-core/src/syntax.rs
  • Unnecessary rename annotation @ helix-view/src/editor.rs
  1. Problem with themes: background vs foreground color
  2. Drawing ruler under the text

Anything else?

@codingjerk
Copy link

I played with code a bit and I have some points here, except styling related, since they are trivial:

  1. For themes, I think it will be better to introduce another theme in addition to ui.virtual.ruler, like ui.virtual.ruler.char or ui.virtual.ruler-char when editor.ruler-char is set.
  2. To draw ruler under text I simply move render_rulers call before render_document. This change will be required if we will use DecorationManager for rulers in future anyway. Cause render_document takes ownership on it.

For now it looks great on my setup, I would like to send PR.

image

I'm not completely happy with rendering code and introducing two separate theme keys tho. So I would appreciate any help.

@codingjerk
Copy link

I created a PR #11798 and I suggest moving further work there

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