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 write_perm func and doc #340

Merged
merged 11 commits into from Sep 29, 2018

Conversation

phineas0fog
Copy link
Contributor

@phineas0fog phineas0fog commented Jan 21, 2018

Description

Add segment who display a padlock (by default) if current dir is write protected or if current user has not enought rights to write in current directory.

Screenshot

scrot

Copy link
Contributor

@maximbaz maximbaz left a comment

Choose a reason for hiding this comment

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

So simple and yet so cool - definitely going to use myself!

spaceship.zsh Outdated
@@ -42,6 +42,7 @@ if [ -z "$SPACESHIP_PROMPT_ORDER" ]; then
SPACESHIP_PROMPT_ORDER=(
time # Time stampts section
user # Username section
write_perm # Write permission in current directory
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes more sense to show the lock icon after the directory name, not before.

docs/Options.md Outdated
| `SPACESHIP_WRITE_PERM_PREFIX` | `` | Prefix before section |
| `SPACESHIP_WRITE_PERM_SUFFIX` | `·` | Suffix after section |
| `SPACESHIP_WRITE_PERM_COLOR` | `red` | Color of the icon |
| `SPACESHIP_WRITE_PERM_ICON` | `` | Icon displayed |
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this icon look like? In my terminal, that doesn't look like a padlock, but 🔒 does (link).

Also, I think this should be SYMBOL, not ICON (for consistency with the rest of spaceship).

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently the currently used icon is from FontAwesome and I didn't even notice this because I have FontAwesome installed, but I agree that the default icon should be from Unicode standard.

Copy link
Contributor

Choose a reason for hiding this comment

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

The lock symbol from Powerline font actually looks not bad too, since other sections use Powerline symbols already maybe it can be a default?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I would still prefer default unicode symbols when suitable ones exist. Many users forget to install powerline fonts. Some things are fairly obvious (e.g.  master), but some things aren't so clear
(e.g. /etc  vs. /etc 🔒)

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed 👍

Copy link
Member

Choose a reason for hiding this comment

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

We already require powerline font, so why not? Powerline lock icon looks more native that Unicode one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Powerline lock icon looks more native that Unicode one.

I agree, but I think the argument was that it's you and me who installed Powerline fonts, but others might decided to not install it. So far it was mostly needed for git branch icon, and it is possible that many users decided that they are fine not installing Powerline font and seeing  master instead of  master. Such users will now start seeing /etc .

I'll let @nfischer continue, I'm fine with any icon as default 😛

@denysdovhan
Copy link
Member

@phineas0fog pull request template requires attaching a screenshot when you someone propose a new section. You're not an exception, so, please, provide a screenshot.

Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

Possibility about combining with dir section.

directory with write permission

spaceship.zsh Outdated
@@ -43,6 +43,7 @@ if [ -z "$SPACESHIP_PROMPT_ORDER" ]; then
time # Time stampts section
user # Username section
dir # Current directory section
write_perm # Write permission in current directory
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't have to be a separate section, Can we combine this dir section ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this idea!

Copy link
Contributor

@maximbaz maximbaz Jan 21, 2018

Choose a reason for hiding this comment

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

Another reason why I think it would be nice to have this as part of the dir section is because I'd like to move the "lock sign" closer to the directory name:

image

It's not as important for lock symbol, but personally I think I'll be using *, as lock is too massive in my mind:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this ?

spaceship_dir() {
  [[ $SPACESHIP_DIR_SHOW == false ]] && return

  local dir

  # Threat repo root as a top-level directory or not
  if [[ $SPACESHIP_DIR_TRUNC_REPO == true ]] && spaceship::is_git; then
    local git_root=$(git rev-parse --show-toplevel)
    dir="$git_root:t${$(expr $(pwd) : "$git_root\(.*\)")}"
  else
    dir="%${SPACESHIP_DIR_TRUNC}~"
  fi

  spaceship::section \
    "$SPACESHIP_DIR_COLOR" \
    "$SPACESHIP_DIR_PREFIX" \
    "$dir" \
    "$SPACESHIP_DIR_SUFFIX"

  [[ $SPACESHIP_WRITE_PERM_SHOW == false ]] && return

  [[ -w . ]] && return

  spaceship::section \
    "$SPACESHIP_WRITE_PERM_COLOR" \
    "$SPACESHIP_WRITE_PERM_PREFIX" \
    "$SPACESHIP_WRITE_PERM_SYMBOL" \
    "$SPACESHIP_WRITE_PERM_SUFFIX"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Such approach won't allow moving PERM_SYMBOL closer to the directory name, but I can live without it, I think I'll be using Powerline's lock symbol and it looks better with a space anyway.

image

Wait for @salmanulfarzy to reply if this is what he had in mind code-wise though.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need all this options ? In my opinion, Only SPACESHIP_WRITE_PERM_COLOR and SPACESHIP_WRITE_PERM_SYMBOL are needed.

I would rather replace/append lock symbol to SPACESHIP_DIR_SUFFIX than making it a separate section. Altering SPACESHIP_DIR_PREFIX might not be visible if dir is first section, Which is common.

SPACESHIP_DIR_WRITE_LOCK_SYMBOL="${SPACESHIP_DIR_WRITE_LOCK_SYMBOL=" 🔒 "}"

# Before invoking the section
if [[ ! -w . ]]; then
  SPACESHIP_DIR_SUFFIX="${SPACESHIP_DIR_WRITE_LOCK_SYMBOL}"
fi

It doesn't allow to change colour of symbol, But I don't see myself changing colour of lock symbol. If needed we could make use of escape codes in assignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better if we can change color of lock, to make it more visible...

Copy link
Member

Choose a reason for hiding this comment

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

Now we have two options

  if [[ ! -w . ]]; then
    SPACESHIP_DIR_SUFFIX="%F{$SPACESHIP_DIR_LOCK_COLOR}$SPACESHIP_DIR_WRITE_LOCK_SYMBOL%f"
  fi

dir-write-lock

I'm open to both options. Suggestion ?


Is it just me or the lock symbol stays opaque ? Tried with iTerm and Terminal.

@maximbaz maximbaz added the new-feature A PR that implement feature (section, specific behavior, etc). label Jan 21, 2018
phineas0fog added 2 commits January 22, 2018 12:46
@phineas0fog
Copy link
Contributor Author

phineas0fog commented Jan 22, 2018

I think is now better (?)

And for the icon, personnally I use FuraMonoForPowerline NF (it's from Nerd Fonts, a set of powerline fonts with more glyphs). But the padlock unicode symbol (🔒, U+1F512) is not displayed and I dont know why. This is why I put that of Powerline.

sections/dir.zsh Outdated
@@ -23,6 +25,8 @@ spaceship_dir() {

local dir

[[ -w . ]] && SPACESHIP_WRITE_PERM_SYMBOL=''
Copy link
Contributor

Choose a reason for hiding this comment

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

this line is not needed, right?

sections/dir.zsh Outdated
@@ -31,6 +35,10 @@ spaceship_dir() {
dir="%${SPACESHIP_DIR_TRUNC}~"
fi

if [[ ! -w . ]]; then
SPACESHIP_DIR_SUFFIX="%F{$SPACESHIP_DIR_LOCK_COLOR} $SPACESHIP_DIR_LOCK_SYMBOL %f"
Copy link
Contributor

@maximbaz maximbaz Jan 22, 2018

Choose a reason for hiding this comment

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

Since you are using this approach, I suggest these improvements:

  1. Remove space here before and after $SPACESHIP_DIR_LOCK_SYMBOL, instead add spaces before and after the lock icon in the default value of SPACESHIP_DIR_LOCK_SYMBOL above - this will allow this:

    image

  2. You are overriding the value of $SPACESHIP_DIR_SUFFIX, I think you should keep it in case users configured it.

So this line I guess should become something like:

    SPACESHIP_DIR_SUFFIX="%F{$SPACESHIP_DIR_LOCK_COLOR}${SPACESHIP_DIR_LOCK_SYMBOL}%f${SPACESHIP_DIR_SUFFIX}"

Copy link
Contributor

Choose a reason for hiding this comment

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

@phineas0fog I updated the last line ^^ a little after publishing, just so you know.

For readability, I think this is the best:

SPACESHIP_DIR_SUFFIX="%F{$SPACESHIP_DIR_LOCK_COLOR}${SPACESHIP_DIR_LOCK_SYMBOL}%f${SPACESHIP_DIR_SUFFIX}"

docs/Options.md Outdated
| `SPACESHIP_DIR_COLOR` | `cyan` | Color of directory section |
| `SPACESHIP_DIR_COLOR` | `cyan` | Color of directory section |''
| `SPACESHIP_DIR_LOCK_SYMBOL` | `🔒` | The symbol displayed if directory is write-protected |
| `SPACESHIP_DIR_LOCK_COLOR` | `red` | Color for the symbol |
Copy link
Contributor

Choose a reason for hiding this comment

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

for the lock symbol

docs/Options.md Outdated
@@ -127,6 +127,7 @@ Hostname is shown only when you're connected via SSH unless you change this beha
### Directory (`dir`)

Directory is always shown and truncated to the value of `SPACESHIP_DIR_TRUNC`. While you are in repository, it shows only root directory and folders inside it.
If current directory is write-protected or if current user has not enought rights to write in, a padlock (by default) s displayed as suffix.
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny: if current user has not enought rights to write in it, a padlock (by default) is displayed as a suffix

maximbaz
maximbaz previously approved these changes Jan 22, 2018
Copy link
Contributor

@maximbaz maximbaz left a comment

Choose a reason for hiding this comment

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

awesome

docs/Options.md Outdated
@@ -135,7 +136,9 @@ Directory is always shown and truncated to the value of `SPACESHIP_DIR_TRUNC`. W
| `SPACESHIP_DIR_SUFFIX` | `$SPACESHIP_PROMPT_DEFAULT_SUFFIX` | Suffix after current directory |
| `SPACESHIP_DIR_TRUNC` | `3` | Number of folders of cwd to show in prompt, 0 to show all |
| `SPACESHIP_DIR_TRUNC_REPO` | `true` | While in `git` repo, show only root directory and folders inside it |
| `SPACESHIP_DIR_COLOR` | `cyan` | Color of directory section |
| `SPACESHIP_DIR_COLOR` | `cyan` | Color of directory section |''
| `SPACESHIP_DIR_LOCK_SYMBOL` | `🔒` | The symbol displayed if directory is write-protected |
Copy link
Contributor

Choose a reason for hiding this comment

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

Space into the docs ·🔒 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry :/

docs/Options.md Outdated
@@ -135,7 +136,9 @@ Directory is always shown and truncated to the value of `SPACESHIP_DIR_TRUNC`. W
| `SPACESHIP_DIR_SUFFIX` | `$SPACESHIP_PROMPT_DEFAULT_SUFFIX` | Suffix after current directory |
| `SPACESHIP_DIR_TRUNC` | `3` | Number of folders of cwd to show in prompt, 0 to show all |
| `SPACESHIP_DIR_TRUNC_REPO` | `true` | While in `git` repo, show only root directory and folders inside it |
| `SPACESHIP_DIR_COLOR` | `cyan` | Color of directory section |
| `SPACESHIP_DIR_COLOR` | `cyan` | Color of directory section |''
Copy link
Member

Choose a reason for hiding this comment

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

Remove empty string after the last column separator.

sections/dir.zsh Outdated
@@ -13,6 +13,8 @@ SPACESHIP_DIR_SUFFIX="${SPACESHIP_DIR_SUFFIX="$SPACESHIP_PROMPT_DEFAULT_SUFFIX"}
SPACESHIP_DIR_TRUNC="${SPACESHIP_DIR_TRUNC=3}"
SPACESHIP_DIR_TRUNC_REPO="${SPACESHIP_DIR_TRUNC_REPO=true}"
SPACESHIP_DIR_COLOR="${SPACESHIP_DIR_COLOR="cyan"}"
SPACESHIP_DIR_LOCK_SYMBOL="${SPACESHIP_DIR_LOCK_SYMBOL=" 🔒"}"
SPACESHIP_DIR_LOCK_COLOR="${SPACESHIP_DIR_LOCK_COLOR=red}"
Copy link
Member

Choose a reason for hiding this comment

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

Please quote the color variable.

@denysdovhan
Copy link
Member

I wouldn't agree with @salmanulfarzy. As you may notice from git and hg sections, the section should be as atomic and composable as it can. That's why we have git_branch and git_status.

I like the idea to have it as a separate section, but write_perm as a name is not obvious (write permission for what?). By the way, it might be useful for some users to see other permissions (rwx) as a status (like in git section).

As an alternative solution, we can use a diferent color for dir section when write-access is not provides.

@maximbaz
Copy link
Contributor

rwx is probably an overkill... I think you won't be able to enter a unreadable directory, so r will always be present. As for x, it is possible, but very very few directories are mounted with noexec, so I don't really think it is worth even considering this until at least one person shows up and says that they want it for such and such reason.

As an alternative solution, we can use a diferent color for dir section when write-access is not provides.

This will make directory names stand out too much, I personally prefer a very small indicator, such as * or poweline lock icon.

Sections have spaces between them, that's also why I like the icon being part of the dir module, I think it looks kinda neat 🙂 But as mentioned earlier, I'm fine with it being a separate section too.

image

@salmanulfarzy
Copy link
Member

salmanulfarzy commented Jan 23, 2018

As I said before, I'm open to both options. Just that I don't see what possible options _SUFFIX and _PREFIX could get here and find it better with this suggestion by @maximbaz

Agree with @maximbaz on rwx functionality.

@phineas0fog
Copy link
Contributor Author

So, what should I do ?

Copy link
Member

@denysdovhan denysdovhan left a comment

Choose a reason for hiding this comment

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

Consider my suggestions

docs/Options.md Outdated
@@ -136,6 +137,8 @@ Directory is always shown and truncated to the value of `SPACESHIP_DIR_TRUNC`. W
| `SPACESHIP_DIR_TRUNC` | `3` | Number of folders of cwd to show in prompt, 0 to show all |
| `SPACESHIP_DIR_TRUNC_REPO` | `true` | While in `git` repo, show only root directory and folders inside it |
| `SPACESHIP_DIR_COLOR` | `cyan` | Color of directory section |
| `SPACESHIP_DIR_LOCK_SYMBOL` | `·🔒` | The symbol displayed if directory is write-protected |
Copy link
Member

Choose a reason for hiding this comment

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

Please, use powerline lock icon.

Copy link
Contributor

Choose a reason for hiding this comment

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

@denysdovhan just to confirm, have you seen this thread? I'm personally using powerline lock icon too, but I kinda agree with the argument there.

Copy link
Member

Choose a reason for hiding this comment

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

We already require powerline patched font, why don't we use all of its power?

Copy link
Member

Choose a reason for hiding this comment

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

Updated with powerline lock symbol, Allows the color to be modified.

docs/README.md Outdated
@@ -56,4 +56,3 @@
* [spaceship::displaytime](/docs/API.md#spaceshipdisplaytime-seconds)
* [spaceship::union](/docs/API.md#spaceshipunion-arr1-arr2-)
* [Troubleshooting](/docs/Troubleshooting.md)

Copy link
Member

Choose a reason for hiding this comment

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

Do not remove the last empty line. You can use .editorconfig which is already present in our repo.

Copy link
Member

Choose a reason for hiding this comment

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

Reverted to original file.

sections/dir.zsh Show resolved Hide resolved
@denysdovhan
Copy link
Member

We will definitely have some users that would open tons of issues like:

What is a strange * appears right after dir? Am I doing something wrong?

How to disable my broken dir section?

@salmanulfarzy
Copy link
Member

@phineas0fog still interested in updating this ?

@salmanulfarzy
Copy link
Member

salmanulfarzy commented Sep 29, 2018

This has been open for awhile, Fixed the suggestions and merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature A PR that implement feature (section, specific behavior, etc).
Development

Successfully merging this pull request may close these issues.

None yet

5 participants