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.

Sign up for GitHub

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

Jump to bottom

Add SPACESHIP_JOBS_AMOUNT_THRESHOLD variable #486

Merged
merged 1 commit into from
Jul 29, 2018
Merged

Add SPACESHIP_JOBS_AMOUNT_THRESHOLD variable #486

merged 1 commit into from
Jul 29, 2018

Conversation

mimame
Copy link
Contributor

@mimame mimame commented Jul 21, 2018 •
edited
Loading

First of all, thanks a lot for this amazing project!

Description

I don't like the used jobs symbol and I prefer see the word 'jobs' (with the suffix variable) but the number of jobs isn't shown when there is only one.

I propose don't reset the $jobs_amount variable if the $SPACESHIP_JOBS_SHOW_AMOUNT_ALWAYS is yes

Config example:

export SPACESHIP_JOBS_SYMBOL=''
export SPACESHIP_JOBS_SHOW_AMOUNT_ALWAYS='yes'
export SPACESHIP_JOBS_SUFFIX=' \e[36mjobs\e[0m '

Screenshot

1_jobs

I hope to be useful but perhaps another solution is better

@mimame mimame mentioned this pull request Jul 21, 2018
Closed
maximbaz
maximbaz reviewed Jul 21, 2018
View reviewed changes
docs/Options.md Outdated
@@ -523,6 +523,7 @@ This section show only when there are active jobs in the background.
| `SPACESHIP_JOBS_SUFFIX` | ` ` | Suffix after the jobs indicator |
| `SPACESHIP_JOBS_SYMBOL` | `✦` | Character to be shown when jobs are hiding |
| `SPACESHIP_JOBS_COLOR` | `blue` | Color of background jobs section |
| `SPACESHIP_JOBS_SHOW_AMOUNT_ALWAYS` | `false` | Show number of jobs (`true`, `false`) |
Copy link
Contributor

@maximbaz maximbaz Jul 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

_SHOW_AMOUNT with values of true | false | always is more extensible and in line with other scripts, like you had in #459 :)

I like this idea better: https://github.com/denysdovhan/spaceship-prompt/pull/486/files#r204212086

salmanulfarzy
salmanulfarzy reviewed Jul 21, 2018
View reviewed changes
sections/jobs.zsh Outdated
@@ -13,6 +13,7 @@ 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=""}"
SPACESHIP_JOBS_SHOW_AMOUNT_ALWAYS="${SPACESHIP_JOBS_SHOW_AMOUNT_ALWAYS=false}"
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this how about using _THRESHOLD variable for number of jobs after which count shows up ?

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 suggestion, although semantically only 0 and 1 would be the used thresholds, the code would be much more readable than figuring out what "SHOW=always" vs "SHOW=true" mean.

Copy link
Member

Choose a reason for hiding this comment

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

May be SPACESHIP_JOBS_AMOUNT_THRESHOLD for the variable name ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for the feedback, I've updated the commit

@salmanulfarzy salmanulfarzy added the improvement A PR that make small changes for improving UX, performance, readability, etc label Jul 21, 2018
salmanulfarzy
salmanulfarzy reviewed Jul 21, 2018
View reviewed changes
docs/Options.md Outdated
@@ -523,6 +523,7 @@ This section show only when there are active jobs in the background.
| `SPACESHIP_JOBS_SUFFIX` | ` ` | Suffix after the jobs indicator |
| `SPACESHIP_JOBS_SYMBOL` | `✦` | Character to be shown when jobs are hiding |
| `SPACESHIP_JOBS_COLOR` | `blue` | Color of background jobs section |
| `SPACESHIP_JOBS_AMOUNT_THRESHOLD` | `1` | Number of jobs which job section will be shown |
Copy link
Member

Choose a reason for hiding this comment

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

Wording isn't proper. jobs section will be displayed if there are any background jobs, It's only the count we're hiding. May be something like "Number of jobs after which job count will be displayed/shown" ?

@maximbaz might be able to provide better options.

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 your suggestion is fine

@salmanulfarzy salmanulfarzy changed the title jobs.zsh: Add $SPACESHIP_JOBS_SHOW_AMOUNT_ALWAYS variable Add SPACESHIP_JOBS_AMOUNT_THRESHOLD variable Jul 29, 2018
@salmanulfarzy salmanulfarzy merged commit f30d6f4 into spaceship-prompt:master Jul 29, 2018
@salmanulfarzy
Copy link
Member

Thank you for contributing @mimadrid 🎉

@salmanulfarzy salmanulfarzy mentioned this pull request Jul 29, 2018
Merged
denysdovhan pushed a commit that referenced this pull request May 27, 2021
@mimame @salmanulfarzy
Option to show job count only if greater than threshold (#486)
7b3247d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@salmanulfarzy salmanulfarzy salmanulfarzy left review comments

@maximbaz maximbaz maximbaz approved these changes

Assignees

@mimame mimame

Labels
improvement A PR that make small changes for improving UX, performance, readability, etc
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mimame @salmanulfarzy @maximbaz