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

Move Git action after dirty with space and default color #506

Merged
merged 4 commits into from
Apr 12, 2020

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Oct 29, 2019

Summary
Moves the dirty indicator to be always adjacent to the branch. Separate the git action with a space instead of a pipe. Bestow a distinct default color to the git action, yellow.

This is following up to other recent changes #491 and #486. (which are great!)

I feel that the re-arranging would be a less welcome change without the default color also being changed.

Before:
image

After:
image

Of course this is all subjective but here is my rationale...

The dirty indicator should be adjacent to the branch since they are very closely related in my mind. Together they tell me the status of the working tree. The git action seems like it belongs at the end since it is a "special state", relatively uncommon, important to notice. A space and a distinct color is cleaner and more readable than a pipe.

I chose yellow instead of red (as in #486) for the git action since red looks like an error.

If these changes are agreed upon, I can update the readme too.

@sindresorhus
Copy link
Owner

I agree it makes sense that * is always after the branch, also highlighting the rebase action. I'm not sure about using just space between them though. It makes it seem like they're not connected. Is there any other Unicode symbol we could use that is more subtle than |?

@camsteffen
Copy link
Contributor Author

Is there any other Unicode symbol we could use that is more subtle than |?

Here's a few: , , », ,

@camsteffen
Copy link
Contributor Author

camsteffen commented Feb 10, 2020

My vote would still be for just using a space. Putting another symbol next to the "dirty" asterisk looks messy.

image

@mafredri
Copy link
Collaborator

How about rebase-i(master*)? To be honest I'm not too opinionated about this, I think the default display of master|rebase-i* makes sense. The way I think about it is that we're on branch master, which is in a state of rebase, and it's dirty.

@camsteffen
Copy link
Contributor Author

How about rebase-i(master*)?

I think it's best for the git action to be on the right so that the branch name stays in the same position (more or less) as you work.

The way I think about it is that we're on branch master, which is in a state of rebase, and it's dirty.

Maybe we have different mental models. To me, the current git action is orthogonal to the current git branch. I would not say that a branch is in a state of rebase or merge.

@sindresorhus
Copy link
Owner

How about using gray and dimmed parens around the rebase state?

Screenshot 2020-02-14 at 14 14 46

@camsteffen
Copy link
Contributor Author

How about using gray and dimmed parens around the rebase state?

Good! I'll code that up.

@camsteffen
Copy link
Contributor Author

Umm can you give me a hint for getting dimmed gray? Do I need to add an entry to prompt_pure_colors?

@sindresorhus
Copy link
Owner

You can see the ansi codes for dim and gray style here: https://github.com/chalk/ansi-styles/blob/74d421cf32342ac6ec7b507bd903a9e1105f74d7/index.js#L68

@sindresorhus
Copy link
Owner

Do I need to add an entry to prompt_pure_colors?

No, I don't think we need the parens to be customizable.

@camsteffen
Copy link
Contributor Author

Okay, ready for review.

@sindresorhus
Copy link
Owner

@mafredri LGTY?

Copy link
Collaborator

@mafredri mafredri 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 good, but I'd like some clarity on the escape codes used.

pure.zsh Outdated Show resolved Hide resolved
@camsteffen
Copy link
Contributor Author

I updated the code to...

  1. Properly escape using %{...%}
  2. Separate the git action code from the git branch code (seems more consistent, readable)
  3. Add a comment

@romkatv raised an issue about the "faint" escape code not being portable. I don't really understand that issue well enough to comment...

@camsteffen
Copy link
Contributor Author

Rebased again to update the readme color diagram with user@host at the beginning.

@mafredri
Copy link
Collaborator

@romkatv raised an issue about the "faint" escape code not being portable. I don't really understand that issue well enough to comment...

I can't really speak for how well it's supported either, but I do think there is potential for trouble. Thinking of terminals like the Emacs terminal or (u)rxvt.

But also, I do think the color needs to be configurable, otherwise it's kind of anti-theme-ability (that one pesky color you can't change). And if it's configurable, then there's really no reason for it to be faded either (as that would also go against theme-ability). We'd just pick a darker / more faded color.

@sindresorhus
Copy link
Owner

If the dim style is not supported, it will just be gray, which is fine too. I don't really see the problem.


We'd just pick a darker / more faded color.

The benefit of dimming gray is that we just take the gray the user has chosen and that fits with their theme and make it dimmer. If we pick a darker color, it might not look good when put in front of some terminal background colors.

@romkatv
Copy link
Contributor

romkatv commented Mar 9, 2020

If the dim style is not supported, it will just be gray

Color 7 is "white". This is often the same color as default foreground, so it's rather bright. On terminals without dim style parentheses will stand out. They'll be much brighter than branch name, for example.

Pure currently uses color 242 in several places (including branch name), which is always #6c6c6c (grey). Pure doesn't use dim style or any kind of grey color that is affected by the terminal theme. Pure doesn't use color 7 (white, affected by the terminal theme) anywhere.

I don't have a strong opinion here. I personally wouldn't define default parameters that make prompt looks weird on terminals without dim style support. I'll survive if you do that, so don't mind me. I'll deal with potential complaints from powerlevel10k users who enable Pure style with the usual retort -- "that's how Pure does it."

@sindresorhus
Copy link
Owner

If it’s white, that’s wrong. I asked for gray, which is technically called “ansi bright black” (code 90).

@romkatv
Copy link
Contributor

romkatv commented Mar 9, 2020

If it’s white, that’s wrong. I asked for gray, which is technically called “ansi bright black” (code 90).

That would be color 8 (%F{8}). Pure doesn't currently use it.

Note that color 8 (without dim) is invisible in Solarized Dark. Color 8 with dim is almost invisible in Tango Dark. I believe these color schemes are among the most popular, if not the most popular.

@camsteffen
Copy link
Contributor Author

In my test, dim 7, 8, and 90 all look the same and are all invisible in Solarized Dark. That does seem like a non-starter. @romkatv do you have any suggestions?

@romkatv
Copy link
Contributor

romkatv commented Mar 9, 2020

@romkatv do you have any suggestions?

I wouldn't presume to suggest but I can tell you what I would do if this was up to me. I wouldn't use parentheses. They are out of place in Pure prompt as it currently has no brackets of any kind. Different parts of prompt are simply separated by spaces and distinguished by colors. The original screenshot you've posted looks fine to me:

image

I respect the right of @sindresorhus to run this project the way he likes. I don't use Pure myself, so my opinion carries no weight.

@camsteffen
Copy link
Contributor Author

camsteffen commented Mar 9, 2020

I agree 100% 👆, except I am a Pure user. I could take or leave the parenthesis.

@sindresorhus
Copy link
Owner

Note that color 8 (without dim) is invisible in Solarized Dark. Color 8 with dim is almost invisible in Tango Dark. I believe these color schemes are among the most popular, if not the most popular.

That's a problem with those color schemes and not really a concern for Pure.

@sindresorhus
Copy link
Owner

And if it's configurable, then there's really no reason for it to be faded either (as that would also go against theme-ability). We'd just pick a darker / more faded color.

The benefit of dimming instead of choosing a darker colors is that it works no matter whether the terminal background is dark or light.

@sindresorhus
Copy link
Owner

Seems like the parens cause too much problems. Let's just drop them. We can tweak the look in the future if we think of a better way to present it.

The git action now displays after the "dirty" symbol.
Before: branch|rebase-i*
After: branch* (rebase-i)
@sindresorhus sindresorhus changed the title Move git action after dirty with space and default color Move Git action after dirty with space and default color Apr 12, 2020
@sindresorhus sindresorhus merged commit d04fbf6 into sindresorhus:master Apr 12, 2020
sileht pushed a commit to sileht/pure that referenced this pull request Apr 18, 2020
…s#506)

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
wezzynl added a commit to wezzynl/pure that referenced this pull request May 1, 2020
* upstream/master:
  1.12.0
  Respawn Pure async worker on crash (sindresorhus#543)
  Use HTTPS links
  Fix stash during git action (sindresorhus#535)
  Move Git action after dirty with space and default color (sindresorhus#506)
wezzynl added a commit to wezzynl/pure that referenced this pull request Sep 28, 2020
…ster

* 'master' of https://github.com/sindresorhus/pure:
  1.14.0
  More system report improvements (sindresorhus#568)
  Use GIT_OPTIONAL_LOCKS=0 instead of --no-optional-locks (sindresorhus#569)
  Show user@host when running in a container (sindresorhus#564)
  Update zsh-async to v1.8.4 (sindresorhus#566)
  Add zsh-async version to system report (sindresorhus#567)
  Bump Pure internal version when publishing package (sindresorhus#565)
  Update readme.md
  1.13.0
  Respect Git config `status.showUntrackedFiles` (sindresorhus#542)
  Add forivall/pure to Ports section in the readme (sindresorhus#556)
  Fix '--no-optional-locks' argument position (sindresorhus#553)
  Eliminate unnecessary Git locking (sindresorhus#549)
  Readme tweaks
  1.12.0
  Respawn Pure async worker on crash (sindresorhus#543)
  Use HTTPS links
  Fix stash during git action (sindresorhus#535)
  Move Git action after dirty with space and default color (sindresorhus#506)
kutsan pushed a commit to kutsan/pure that referenced this pull request Jun 19, 2023
…s#506)

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
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.

None yet

4 participants