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 configuration for min width of line-numbers gutter #4724

Merged
merged 22 commits into from
Jan 21, 2023

Conversation

dgkf
Copy link
Contributor

@dgkf dgkf commented Nov 12, 2022

Supercedes #3953

Now that the line-number gutter can be resized (#3469), this exposes options for the currently hard-coded minimum number of characters used in the line-numbers gutter.

Architectural Changes

  • GutterConfig & GutterLineNumbersConfig config structs added, allowing for the configuration style suggested in #1188.
  • View now accepts a gutters parameter as a GutterConfig object instead of Vec<GutterType>
  • Editor::refresh_config() now also refreshes Views to respond to configuration update events

Design feedback needed

  • Should editor.gutters provided as an array of strings be deprecated in favor of editor.gutters.layout?

@dgkf dgkf marked this pull request as draft November 12, 2022 18:44
helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-view/src/view.rs Outdated Show resolved Hide resolved
@the-mikedavis
Copy link
Member

I wonder if this is necessary after #3469? You would have to be editing a really massive file for the gutter to be noticeably large

@archseer
Copy link
Member

Agree, we can offer a configurable minimum but no maximum.

@dgkf
Copy link
Contributor Author

dgkf commented Nov 14, 2022

@the-mikedavis - I can only speak for myself, but I really try to cut down on the unused space in the gutters. In vim I set numberwidth=1 and signcolumn='number', which usually means I usually get away with only 3 columns for both diagnostics and line numbers. Especially when working on a small display, saving a collective 6-10 columns can make the difference with a vertical split or two.

I don't feel as strongly about a maximum, but it seemed like an easy related addition. I would use it if it was available.

@archseer
Copy link
Member

I think a min of 2 was deliberate in the previous PR:

The line number gutter will have a minimum width of 2 characters. This is so that configurations using line-number: "relative" aren't adversely affected by this change. In the future, the min/max width could perhaps be configurable and this hard-coded minimum could be removed.

@archseer
Copy link
Member

In most cases the line number column is going to be under 5-6 columns (10_000 - 100_000 lines), and any larger than that would be a pain to load fully into memory anyway.

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Nov 14, 2022
@dgkf
Copy link
Contributor Author

dgkf commented Nov 14, 2022

The line number gutter will have a minimum width of 2 characters. This is so that configurations using line-number: "relative" aren't adversely affected

I suggested this, and in retrospect it wouldn't matter. If a document has <10 lines, then the maximum relative offset is also <10, so you don't need two characters devoted to relative numbering.

Anyways, if this whole PR feels unnecessary to you - that's fine too. Feel free to close the request. Either way I'm grateful for the opportunity to get my fingers into some rust development with a clean codebase and will look for the next task to chip away at.

helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
@dgkf dgkf marked this pull request as ready for review November 25, 2022 01:50
@dgkf
Copy link
Contributor Author

dgkf commented Nov 26, 2022

Agree, we can offer a configurable minimum but no maximum.

@archseer @the-mikedavis I've altered the implementation so that only minimum linenumber gutter width is configurable. This is ready for review.

@dgkf dgkf changed the title Adding settings for min and max line-numbers gutter width Adding settings for min line-numbers gutter width Dec 4, 2022
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.

This looks like a good way to configure gutters to me 👍. We will probably end up using this in the future to allow configuration of the characters used for additions/modifications/deletions in the diff gutter element and maybe the separator character (as with the statusline, although I can't imagine anything other than space looking good)

helix-view/src/gutter.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
book/src/configuration.md Outdated Show resolved Hide resolved
@dgkf dgkf requested review from the-mikedavis and pascalkuthe and removed request for the-mikedavis and pascalkuthe December 16, 2022 23:09
@dgkf
Copy link
Contributor Author

dgkf commented Dec 16, 2022

@the-mikedavis @pascalkuthe - Thank you both for reviewing and for the suggestions on how to better handle the different styles of configuration. I've incorporated your feedback. This PR is now ready for another pass of review.


#### `[editor.gutters.spacer]` Section

Currently unused
Copy link
Contributor

Choose a reason for hiding this comment

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

What about [editor.gutters.breakpoints]? I’m mentioning this because of what I’m doing in #5371 (I might eventually rebase on your work).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now the sub-sections are styled after the names that can be provided to editor.gutters. There's no real reason it must be like this, but it think there's some discoverability benefit to being consistent here.

Following that style, both breakpoints and diagnostics would be configured in editors.gutters.diagnostics. I think it's best to follow this precedent, but I'd say this is a call for some of the more core contributors like @the-mikedavis


Looking into the future a bit further, I think the underlying issue here is that diagnostics and breakpoints are a bit oddly coupled - likely a solution from before gutters were configurable. I think there's a case for decoupling diagnostics and breakpoints and trying to tackle that use case more generically. I think it would be interesting to be able to layer gutters as part of the layout. Perhaps something like:

[editor.gutters]
layout = [ [ "diagnostics", "breakpoints" ], "line-numbers" ]

Which would provide the current behavior. I think that needs more discussion before jumping to something like that, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m reimplementing #5371 rebased atop your PR then.

helix-view/src/editor.rs Outdated Show resolved Hide resolved
@dgkf
Copy link
Contributor Author

dgkf commented Jan 10, 2023

@the-mikedavis, @archseer - Any chance I can get your eyes on this PR for review? It's beginning to be used by downstream work, so I'm feeling obliged to push it forward.

@hadronized
Copy link
Contributor

Agreed, it would be nice to be able to merge it. Rebasing my PR (#5371) requires rebasing that one so I can’t really be up-to-date with master besides locally rebasing this one. Is there any missing points?

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.

Looking good overall, just some minor comments

helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-view/src/gutter.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Show resolved Hide resolved
helix-view/src/gutter.rs Outdated Show resolved Hide resolved
runtime/queries/r/locals.scm Outdated Show resolved Hide resolved
@dgkf
Copy link
Contributor Author

dgkf commented Jan 16, 2023

Thank you for the review today @the-mikedavis - I applied all but one change that I wasn't quite clear on.

@hadronized
Copy link
Contributor

Small nit question: have you rebased on master? If so, even though your PR is not merged yet, I’ll rebase #5371 on yours to follow your changes.

@dgkf
Copy link
Contributor Author

dgkf commented Jan 18, 2023

@phaazon - I rebased onto master so hopefully it's easier on you now if you want to use this as the basis for your changes.

@the-mikedavis the-mikedavis linked an issue Jan 21, 2023 that may be closed by this pull request
@the-mikedavis the-mikedavis changed the title Adding settings for min line-numbers gutter width Add configuration for min width of line-numbers gutter Jan 21, 2023
@the-mikedavis the-mikedavis merged commit 2b58ff4 into helix-editor:master Jan 21, 2023
@dgkf dgkf deleted the 2583-pt2-ln-min-max branch January 21, 2023 20:40
kirawi pushed a commit to kirawi/helix that referenced this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config-reload doesn't apply changes in editor.gutters settings
6 participants