fix issue when the # of jobs is greater than 1 #445
Conversation
sections/jobs.zsh
Outdated
|
|
||
| [[ $jobs_amount -gt 0 ]] || return | ||
| [[ $jobs_amount -eq 1 ]] && jobs_amount='' | ||
|
|
||
| if [[ $jobs_amount -gt 1 ]]; then | ||
| jobs_amount=" ${jobs_amount}" |
There was a problem hiding this comment.
With my symbol it looks better without a space, maybe instead of doing this append a space to SPACESHIP_JOBS_SYMBOL in your config?
There was a problem hiding this comment.
This is the output of jobs without appending a space. Seems odd to me.
What do you think? Should I keep the space or remove it?
There was a problem hiding this comment.
Let's make it optional, have or not space before the job count. Maybe call it SPACESHIP_JOBS_AMOUNT_PREFIX and set it to empty string by default, not to change behavior for existing users. Maybe create SPACESHIP_JOBS_AMOUNT_SUFFIX as well, not to return to this matter in future again 🙂
Here's how it looks for me:
There was a problem hiding this comment.
Done! This last commit also fixes #272
sections/jobs.zsh
Outdated
| @@ -20,14 +21,14 @@ SPACESHIP_JOBS_COLOR="${SPACESHIP_JOBS_COLOR="blue"}" | |||
| spaceship_jobs() { | |||
| [[ $SPACESHIP_JOBS_SHOW == false ]] && return | |||
|
|
|||
| local jobs_amount=$( (jobs) | wc -l ) | |||
| local jobs_amount=$( jobs -d | awk '!/pwd/' | wc -l | tr -d " ") | |||
|
|
|||
| [[ $jobs_amount -gt 0 ]] || return | |||
| [[ $jobs_amount -eq 1 ]] && jobs_amount='' | |||
There was a problem hiding this comment.
I think you'd want to empty the SPACESHIP_JOBS_SPACE_PREFIX when jobs amount is 1, wouldn't you?
Because if not, instead of creating this new variable you could have as well just configured
SPACESHIP_JOBS_SYMBOL="✦ " (with an extra space in the end, which is a common practice in spaceship)
There was a problem hiding this comment.
Yeah, that makes sense
There was a problem hiding this comment.
Maybe name it SPACESHIP_JOBS_AMOUNT_PREFIX then, to indicate that it has effect only when amount is visible? And also maybe add SPACESHIP_JOBS_AMOUNT_SUFFIX, just for consistency and completeness 🙂
sections/jobs.zsh
Outdated
| @@ -11,6 +11,8 @@ SPACESHIP_JOBS_PREFIX="${SPACESHIP_JOBS_PREFIX=""}" | |||
| SPACESHIP_JOBS_SUFFIX="${SPACESHIP_JOBS_SUFFIX=" "}" | |||
| SPACESHIP_JOBS_SYMBOL="${SPACESHIP_JOBS_SYMBOL="✦"}" | |||
| SPACESHIP_JOBS_COLOR="${SPACESHIP_JOBS_COLOR="blue"}" | |||
| SPACESHIP_JOBS_AMOUNT_PREFIX="${SPACESHIP_JOBS_AMOUNT_PREFIX=" "}" | |||
There was a problem hiding this comment.
Let's please keep the current behavior and have empty default prefix 😛
sections/jobs.zsh
Outdated
| if [[ $jobs_amount -eq 1 ]]; then | ||
| jobs_amount='' ; | ||
| SPACESHIP_JOBS_AMOUNT_PREFIX='' ; | ||
| SPACESHIP_JOBS_AMOUNT_SUFFIX='' ; |
There was a problem hiding this comment.
you don't need semicolons
|
Hi @maximbaz, is there anything else you would like me to add/modify? |
|
No, I think it's perfect. I wish your PR was merged long time ago, but I don't have the blessing to do so myself — it's the prerogative of @denysdovhan or @salmanulfarzy. |
| @@ -11,6 +11,8 @@ SPACESHIP_JOBS_PREFIX="${SPACESHIP_JOBS_PREFIX=""}" | |||
| SPACESHIP_JOBS_SUFFIX="${SPACESHIP_JOBS_SUFFIX=" "}" | |||
| SPACESHIP_JOBS_SYMBOL="${SPACESHIP_JOBS_SYMBOL="✦"}" | |||
| SPACESHIP_JOBS_COLOR="${SPACESHIP_JOBS_COLOR="blue"}" | |||
| SPACESHIP_JOBS_AMOUNT_PREFIX="${SPACESHIP_JOBS_AMOUNT_PREFIX=""}" | |||
There was a problem hiding this comment.
Only value that I can think for this variable is space and find no need for SPACESHIP_JOBS_AMOUNT_SUFFIX. Why not just make it SPACESHIP_JOBS_SYMBOL="✦ " as suggested by @maximbaz.
This patch also have the same effect right ?
diff --git a/sections/jobs.zsh b/sections/jobs.zsh
index 3f24470..d41cf77 100644
--- a/sections/jobs.zsh
+++ b/sections/jobs.zsh
@@ -9,10 +9,8 @@
SPACESHIP_JOBS_SHOW="${SPACESHIP_JOBS_SHOW=true}"
SPACESHIP_JOBS_PREFIX="${SPACESHIP_JOBS_PREFIX=""}"
SPACESHIP_JOBS_SUFFIX="${SPACESHIP_JOBS_SUFFIX=" "}"
-SPACESHIP_JOBS_SYMBOL="${SPACESHIP_JOBS_SYMBOL="✦"}"
+SPACESHIP_JOBS_SYMBOL="${SPACESHIP_JOBS_SYMBOL="✦ "}"
SPACESHIP_JOBS_COLOR="${SPACESHIP_JOBS_COLOR="blue"}"
-SPACESHIP_JOBS_AMOUNT_PREFIX="${SPACESHIP_JOBS_AMOUNT_PREFIX=""}"
-SPACESHIP_JOBS_AMOUNT_SUFFIX="${SPACESHIP_JOBS_AMOUNT_SUFFIX=""}"
# ------------------------------------------------------------------------------
# Section
@@ -28,13 +26,12 @@ spaceship_jobs() {
if [[ $jobs_amount -eq 1 ]]; then
jobs_amount=''
- SPACESHIP_JOBS_AMOUNT_PREFIX=''
- SPACESHIP_JOBS_AMOUNT_SUFFIX=''
+ SPACESHIP_JOBS_SYMBOL="✦"
fi
spaceship::section \
"$SPACESHIP_JOBS_COLOR" \
"$SPACESHIP_JOBS_PREFIX" \
- "${SPACESHIP_JOBS_SYMBOL}${SPACESHIP_JOBS_AMOUNT_PREFIX}${jobs_amount}${SPACESHIP_JOBS_AMOUNT_SUFFIX}" \
+ "${SPACESHIP_JOBS_SYMBOL}${jobs_amount}" \
"$SPACESHIP_JOBS_SUFFIX"
}
There was a problem hiding this comment.
Not exactly @salmanulfarzy, with your proposal you are hardcoding the jobs symbol and not allowing user to override it.
if [[ $jobs_amount -eq 1 ]]; then
jobs_amount=''
SPACESHIP_JOBS_SYMBOL="✦"
fi
|
No problem, thanks for the review! |
|
This commit seems to have broken jobs on macOS. See #498 |
Description
This PR fixes #444
Screenshot