Consistent statusline spacing

[hamrik]

Statusline elements used to have inconsistent spacing due to each element having to pad itself without knowledge of surrounding elements.

• mode, selections, file-name and position are padded, the rest are not
• file-modification-indicator reserves space for itself, spinner and diagnostics do not

This PR fixes these spacing inconsistencies.

• No elements pad themselves
• No elements reserve empty space
• The statusline pads everything
• The spacer element has been repurposed to join multiple spacers, e.g. to pad the ends of the statusline

The render functions of statusline elements previously used an ad-hoc write callback to blit Spans directly to the Surface of the statusline. This callback has been removed. Now each render function has to return the Spans it wants to display, which the renderer of the statusline collects, intersperses with spaces, then blits to the surface.

[gabydd]

At first look one thing that this changes that we probably shouldn't is it removes padding around the edges in the default config which imo we should keep especially looks bad when color-modes is false. This brings up another question if we are going for consistency do we really want modes size to be dependant on coior-modes? I haven't looked much at the implementation but I think improving consistency is generally good as long as we are not breaking peoples configs.

[David-Else]

@gabydd Regarding color -modes: #7289 (comment)

[gabydd]

Ah right, I only noticed this because I don't use color-modes. Removing the option is probably a good idea in the long run though.

[pascalkuthe]

I am not sure if we need the editor.statusline.spacer config. Why would this be ever set to anything but space?

[hamrik]

Sorry for disappearing, had a busy couple of weeks. Checking the responses right now.

[hamrik]

At first look one thing that this changes that we probably shouldn't is it removes padding around the edges in the default config which imo we should keep especially looks bad when color-modes is false. This brings up another question if we are going for consistency do we really want modes size to be dependant on coior-modes? I haven't looked much at the implementation but I think improving consistency is generally good as long as we are not breaking peoples configs.

I originally didn't have padding in color-modes either, but then the background color cut right off at the end of the mode name, leaving no padding inside the element. It just didn't look right. I agree that this makes modes a bit inconsistent and will have to think of something to address this.

Padding the statusbar edges could be done by changing the default config to include spacer-s on the sides.
Alternatively I could force padding into the statusline, but then color-modes won't look right and the user won't be able to fix it by removing a spacer, so I vote for chaging the default config.
Those who have customized statusline configs shouldn't have a hard time adding a spacer if they don't use color-modes.

[hamrik]

I am not sure if we need the editor.statusline.spacer config. Why would this be ever set to anything but space?

A couple examples:

• Two or more spaces for wider spacing without spamming spacer: file.txt 1:1 [+]
• Powerline-style (colorless) separators: file.txt 〉1:1 〉[+]
  • Altough to make this last one flippable we would have to add separate spacer configs for left, center and right.
  • EDIT: I guess there's separator, but changing spacer width is still a valid reason for keeping it in my opinion.

[pascalkuthe]

Generally I would prefer explicit spacers since it allows different spacing between different elements without adding a ton new config options. I don't think that the spacing inside of elements needs to be configurable. We are shooting for a mininal config file.

[hamrik]

You are free to leave editor.statusline.spacer on the default setting and use spacers. It don't feel it's out of place next to the configurable separator and configurable mode names.

[pascalkuthe]

You are free to leave editor.statusline.spacer on the default setting and use spacers. It don't feel it's out of place next to the configurable separator and configurable mode names.

we are very conservative with regards to adding new config options and this doens' cross the bar for me. The rest of the PR is probably a nice improvement but I don't see us adding this config option

[hamrik]

While I personally disagree, I've removed the option so we can move on. At least that solved the documentation wording problem. We can revisit this in a later PR.

[hamrik]

I believe the only two remaining issues are the spacing of the mode indicator and the spinner. The spinner discussion is happening in the review.

I don't like the idea of hardcoding spacers on the edges, in my opinion modifying the default config to include spacers would be more flexible.

But regardless of how the padding is handled, it would look weird the color modes option turned on.

If we go with the hardcoded route, we could omit adding spacers where the mode indicator is the first/last item and color modes are enabled. But this feels like a hack.

If we go with the modified default config, we could add/omit the spacer depending on whether we enable color modes by default, then expect people to add/remove the spacer manually if they change the color modes option.

On a similar note, right now padding is automatically added to the mode indicator when color-modes are on, but that could also be removed if we expect people to modify the mode names to include spaces when they toggle color modes.

Thoughts?

[hamrik]

I have removed the auto-padding from the mode element, regardless of color-modes.

• This makes the mode element consistent with the other elements.
• If color-modes is off by default, people who turn it on can add padding via editor.statusline.mode.
• If color-modes becomes default, the default values for mode names can also be tweaked to include spaces.

[pascalkuthe]

Yeah I agree with that. I think hardcoded padding is never a good idea and changing the modenames is a pretty good workaround IMO

[pascalkuthe]

This PR is very hard to review right now because you changed the order of some.functions (and in general have included some unnecessary changes) sk the diff is pretty clobbered right now.

Could you undo that so it's easier for us to see what's actually changed

[pascalkuthe]

Also I am not really sure if we really want the implhct spacer between each element. I actually lprefer having no implicit spacer at all and instead including the necessary specaers in the default config. This offers the most flexibility and is the easiet to implement

[hamrik]

I actually lprefer having no implicit spacer at all and instead including the necessary specaers in the default config.

Since some elements only appear occasionally (such as file-modification-indicator, spinner, etc) these would leave two spacers behind whenever they are hidden (assuming the config has explicit spacers between every element). Implementing collapsing spacers would be even more complicated IMO.

Example:

 NOR implicit_spacing.txt 1:1
 NOR implicit_spacing.txt [+] 1:2

vs

 NOR explicit_spacing.txt  1:1
 NOR explicit_spacing.txt [+] 1:2

[hamrik]

This PR is very hard to review right now because you changed the order of some.functions (and in general have included some unnecessary changes) sk the diff is pretty clobbered right now.

I'll clean up the commit history on the weekend.

[hamrik]

Okay, I've removed all additions to Spans, and consolidated the changes into two commits/"phases":

1. Refactor statusline elements to return Spans rather than using a callback. At this point the spacing behavior is identical to the master branch.
2. Removed padding from all statusline elements.

Right now there is no spacing interspersed as Iterator::intersperse() is in nightly for now. While this matches what @pascalkuthe requested I think spaces should be added because manually interspersed spacers don't collapse when an element hides itself, such as file-modification-indicator, leaving two spacers next to each other, which looks inconsistent.

Will address that later.

[hamrik]

Added back automatic interspersing of spaces. Does not pad the sides of the panel, those should be done in config.

Uses a custom interspersing iterator until intersperse() lands in stable Rust. I'm fairly certain that its implementation should be moved elsewhere, not yet sure where would be best.

As for the spinner, I think simply hiding it is the most consistent behavior with other auto-hiding elements like file-modification-indicator. If the jumpyness is annoying it can be moved to a place where it does not push other elements around, such as the inner edge of the left and right sections. Maybe in the future it could get a debouncer so that split-second LSP activity does not trigger it.

[hamrik]

The only thing remaining to tweak in my opinion is the default config. After that I'll reopen the PR.

On the left section:

• If color-modes becomes the default then the default mode strings should be padded with spaces.
• If it does not than an extra spacer needs to be prepended to the default element list.

On the right section:

• A spacer needs to be appended to the default element list.

[hamrik]

Added spacers to the default config to pad the statusline on the edges, with the assumption that color-modes will remain off by default for now.

People who want to enable it and have it look good will have to:

1. Enable color-modes
2. Set custom mode strings that include spaces, e.g. normal = " NOR "
3. Set a custom left section that does not begin with spacer

Debating whether this should be documented.
Also fixed the bug where spacer adds an extra space by accident, which was introduced in the cleanup.

The intersperse implementation still needs a better place, but I think it's OK to reopen the PR by this point.

[hamrik]

I was thinking about color-modes lately. Since the goal is to eventually remove the option altogether, maybe this would be a good time to set it to true by default and incorporate the rest of the tweaks mentioned above into the default config.

It would still look better for people who turn the feature off than it does now when people turn it on without the tweaks.

[hamrik]

Looks like intersperse won't make into the stdlib anytime soon so I introduced the itertools dependency to avoid reinventing the wheel.

If this is a problem I can revert the last commit, before which a hand-rolled solution was used add spacers between statusline elements.

[the-mikedavis]

I would like to avoid itertools if possible - it's a fair amount of code to add for things that can usually be written inline. Here I think it's cleaner to use a helper function that returns Spans:

fn join_with_spaces<'a, I: Iterator<Item = Span<'a>>>(iter: I) -> Spans<'a> {
    let mut spans = Vec::new();
    for elem in iter {
        if !spans.is_empty() {
            spans.push(Span::raw(" "));
        }
        spans.push(elem);
    }
    spans.into()
}

// ..

context.parts.left = join_with_spaces(
    element_ids
        .iter()
        .map(|element_id| get_render_function(*element_id))
        .flat_map(|render| render(context).0),
);

[hamrik]

Okay, I have reversed the inclusion and also replaced the hand-rolled interspersing iterator with your helper function.

[hamrik]

Debating whether to increase spacing between elements from one to two spaces.

[hamrik]

Increased spacing to two spaces, which actually looks closer to how spacing looked before the PR.

[hamrik]

Double spacing breaks consistency next to mode when color-modes is on. Argh!

This PR became a mess, so I'm closing it and will make a new one with a smaller scope.

EDIT: Superseded by #9122