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

[BUG] #91 has removed layout flexibility making previous layout impossible #97

Open
jpcirrus opened this issue Apr 6, 2023 · 13 comments
Labels
bug Something isn't working

Comments

@jpcirrus
Copy link

jpcirrus commented Apr 6, 2023

Describe the bug
#91 added spaces between every layout item, which broke the previous flexibility of being able to configure where you wanted spaces, which could have been configured in the layout, in the styles, in the symbols, or nowhere.

To Reproduce

For example, you want ... to separate the local and remote branch names with no spaces between, but you obviously only want the ... when there is also a remote branch. This used to be able to be configured by prefixing the remote style with .... But since #91, this still works with no remote branch, but when there is a remote branch a space is appended to the local branch name and before the ....

Expected behavior
With local and remote branches they are concatenated with ... between. With only a local branch the ... should not be appended to the local branch name. No spaces should be added in either case.

Screenshots
Before:
20230406T165205-Alacritty@2x

Now:
20230406T165316-Alacritty@2x

Environment:

  • gitmux version: 0.10.2
  • tmux version: tmux 3.3a
  • OS: macOS 12.6.3
  • shell: bash 5.2.15
@jpcirrus jpcirrus added the bug Something isn't working label Apr 6, 2023
@arl
Copy link
Owner

arl commented Apr 6, 2023

Yeah I agree in retrospect we should have handled this differently.

The thing is that the problem stated in #91 is not going to go away just by adding spaces in the layout. Why? Because some layout components disappear in certain conditions, and so if you add spaces in the layout and let's say you've got 2 successive components that disappear, you'll find yourself with 2 successive spaces in your status string.

I think a good way to solve this, and to please everybody :-) would be to ask users to add spaces where they want in-between components in their layout section, but also add a step to gitmux formatting that compresses consecutive whitespaces into a single one. This makes it impossible to have consecutive spaces on purpose but I dont think this is a legit use case.

What do you all think?
@jpcirrus
@joshmedeski

@jpcirrus
Copy link
Author

jpcirrus commented Apr 6, 2023

@arl, thanks for the quick response and good suggestions.

I agree, the intention was good. And, when I started using gitmux I thought that would be a good feature, but after some use and customization realized that I personally liked the way it was with being able to specify where you wanted to configure the spaces (or other decoration): layout, styles, or symbols.

@arl your idea of specifying in the layout where the layout item spaces should go is what I was using pre #91., e.g:

layout: [branch, remote, ' ', flags]

and I really like your suggestion of compressing multiple spaces as that would solve issues if spaces were not only configured in the layout, but also in the styles and symbols.

So, I support your suggestions, thank you, and good luck making everybody happy 😃

@joshmedeski
Copy link
Contributor

So manually including ' ' caused unnecessary spaces when things like flags never rendered (in your example above).

I can see some might not want this feature, I can work to make it an optional feature you opt-into instead of being the default behavior.

@joshmedeski
Copy link
Contributor

joshmedeski commented Apr 6, 2023

@arl I have two ideas.

1. Explicitly Spaces in Layout

From your suggestion, I could manually add spaces between my layout items explicitly.

  layout: [branch, ' ', divergence, ' ', flags, ' ', stats, ' ']

Then update the code to programmatically strip out a extra space if any of these items don't render. But I imagine this code is going to look messy and not be intuitive to the end user.

2. Opt-In to Space Separate Layout

My other idea is setting a option that lets you opt-in to

  layout: [branch, divergence, flags, stats, ' ']
  options:
    layout_separator: ' '

Layout separator would allow people to choose whatever separate they'd like (ex: / - etc...)

This feels more intuitive because you're defining what character you want to use to separate items, however I notice that you can't choose ... in some cases and in another, so it's not too flexible.


Before I did the work, I just noticed that gitmux sometimes added spaces between items and other times didn't. I prefer consistency over an opinionated format that I couldn't predict.

So let me know your thoughts, I'm happy to make a new pull request if we can agree to how we want to handle separating layout items (that's both easy-to-understand, flexible, and predictable).

@arl
Copy link
Owner

arl commented Apr 6, 2023

I'd go for option 1.
I prefer things to be explicit than hidden behind some option.

So exactly as before #91, but we'd just need to deduplicate successive spaces.

I believe that would solve both problems, without creating new ones:

  • you want a single space between some or all layout components but you don't want spaces to be doubled if some component temporarily disappears due to the current git state.
  • you want to be able to concatenate some components (like branch and remote) with some non-whitespace character (like ...)

@joshmedeski thanks for the proposition but I've got some time on my hands

I've tagged you to have your opinion on the resolution of the problem you had for #91 in case I've missed something.

@joshmedeski
Copy link
Contributor

Sweet, I'll be on the lookout for the release. Happy to do code review too

@jpcirrus
Copy link
Author

jpcirrus commented Apr 6, 2023

The other aspect to the compression of spaces is to ensure that there are no leading or trailing spaces to the gitmux ouput, as these can be handled as desired in tmux.conf.

@arl
Copy link
Owner

arl commented Apr 6, 2023

The other aspect to the compression of spaces is to ensure that there are no leading or trailing spaces to the gitmux ouput, as these can be handled as desired in tmux.conf.

Good point. I totally agree.

@arl
Copy link
Owner

arl commented Jun 9, 2024

I was thinking about this again and I guess the most important is to:

  • make sure that the default output remains the same (most people use default config)
  • ensure that people with custom config won't see any change after introducing that feature

Currently, spaces are being added automatically in-between certain layout components.
We could have a new option manual_space (opt-in with default:false).

When enabled, no spaces won't be added automatically. Which means that users can add spaces in their customized configuration.

Finally, I'd add the following simple heuristic for space compression, let me illustrate it with the example from @jpcirrus

  layout: [branch, ' ', divergence, ' ', flags, ' ', stats, ' ']

If a string that only contains spaces is found in the layout (like these above), then add it to the output, but when adding it, trim out the same number of extra spaces that may come from the previous and next layout components, so that there can't be more than N successive spaces at that place.

Most likely N=1 but people might have deliberately added spaces string with more than a single space.

To sum it up, when you'll add a space string between 2 layout components, it'll be like indicating that there will always be that amount of space at this place, and in case of missing layout elements, spaces will be compressed.

Let me know what you think.

Not sure when I'll be able to do this, but I wanted to have a clear kind-of-spec, for future me, or in case anyone is interested to give it a try. I can help with code and review of course.

@jpcirrus
Copy link
Author

  • make sure that the default output remains the same (most people use default config)

Agreed, or at least improved.

  • ensure that people with custom config won't see any change after introducing that feature

I would assume that people with custom config know what they are doing and would read the release notes either before updating, or at least afterwards if they noticed any changes. As a version 1 hasn't been released yet, breaking changes would presumably still be acceptable in order to improve future functionality and maintenance.

Currently, spaces are being added automatically in-between certain layout components.
We could have a new option manual_space (opt-in with default:false).

What about not having automatic spacing, and making all spaces explicit, as in tmux's formats? These could be configured in the layout, styles and symbols, as is currently possible.

Regarding space compression, what about enabling compression of all regular spaces (U+0020) but not of non-breaking spaces (U+00A0) to allow for multiple adjacent spaces (does anybody actually want this, as "space" is usually limited?), and with leading and trailing spaces always trimmed as they can be handled in tmux.conf? Because tmux trims trailing spaces in copy-mode I use a non-breaking space as the suffix of my terminal prompt to enable navigation between prompts in the scrollback buffer, which works nicely.

@arl
Copy link
Owner

arl commented Jun 12, 2024

Thanks @jpcirrus these are all good points.

I'm in favor or reverting #91 but adding compression of successive 0x20 spaces in the output string. Users that wants more spaces could add breaking spaces in special place, those wouldn't be compressed.

Running gitmux (using the default config) should keep giving the same output as now, even if the default config has actually changed (spaces will be added to it)

@joshmedeski
Copy link
Contributor

joshmedeski commented Jun 12, 2024

I'm open to whatever is best for the community. I found Nerd Font icons don't render correctly in a lot of cases if there isn't a space. That is why I originally did this work.

@arl
Copy link
Owner

arl commented Jun 12, 2024

Of course @joshmedeski and I thank you for that. I also have seen that some wide runes require a space afterwards to rend correctly.

I think the described solution will be ok since it should be flexible enough for all use cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

3 participants