8000 Introduce option to opt-out of spinners by BagToad · Pull Request #10773 · cli/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Introduce option to opt-out of spinners #10773

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

Merged
merged 7 commits into from
Apr 16, 2025
Merged

Conversation

BagToad
Copy link
Member
@BagToad BagToad commented Apr 11, 2025

Description

Introduce GH_SPINNER_DISABLED environment variable to opt-out of displaying a "spinner" animation as a progress indicator. Instead, print Working... and any other contextual progress indication (AKA the "label").

Examples

# No added label / prefix
gh search prs  test  -R  cli/cli  -L 300 
Working...

# Additional label / prefix
gh release download -R cli/cli --archive zip
Finding assets to download...
Downloading ...

out

@BagToad BagToad temporarily deployed to cli-automation April 11, 2025 22:00 — with GitHub Actions Inactive
@BagToad BagToad force-pushed the kw/opt-into-no-spinners branch from 3ab0c62 to 3aadd51 Compare April 11, 2025 22:19
@BagToad BagToad marked this pull request as ready for review April 11, 2025 22:22
@BagToad BagToad requested a review from a team as a code owner April 11, 2025 22:22
@BagToad BagToad requested a review from babakks April 11, 2025 22:22
@BagToad BagToad temporarily deployed to cli-automation April 11, 2025 22:22 — with GitHub Actions Inactive
@BagToad BagToad changed the title Option to opt out of spinners Introduce option to opt-out of spinners Apr 11, 2025
@iamazeem
Copy link
Contributor

Instead of Working..., the respective operation e.g.:

  • Searching...
  • Finding assets to download...
  • Downloading...
  • etc.

seem more appropriate here.

@BagToad BagToad requested a review from andyfeller April 14, 2025 15:26
Copy link
Member
@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

Thanks for getting this pull request opened so quickly, @BagToad! ❤️

I see what you mean about the new experience from these efforts, which I agree we likely want something different. 🤔 Honestly, I think we should probably bypass spinner all together if disabled.

At most, we could duplicate the looping effort to print a message every X duration to stderr based on the logic already being used.

Thoughts?

@andyfeller
Copy link
Member

User Experience feedback

  1. I think Working... should only show up if a progress indicator is used without a label (StartProgressIndicator()); otherwise use the label provided

    So instead of:

    Downloading Working...

    we should see

    Downloading
  2. I think spinner is redrawing the screen, which I'm unsure is something gh should avoid...

  3. ...but I suppose there is a question whether progress indicators should stay on the screen once the progress indicator is stopped 🤔

  4. I wonder if we should use the actual ellipsis Unicode character 🤔 (not the end of the world but just something that popped in my head)

BagToad added 2 commits April 14, 2025 15:07
- Move env var evaluation to the factory defaults.
- Use only the label for the spinner instead of adding
"working..." every time. "working..." remains the default
when no label is provided.
This bypasses the `spinner` package for the textual progress indicator for users
with the spinner disabled out of concerns for accessibility, specifically
with screen readers:

- The `spinner` package will continuously re-draw the screen. I wasn't
able to have this cause problems with my Mac screen reader, but it's
nonetheless a concern that other screen readers may not handle this
screen re-drawing well.
- The `spinner` package clears any progress indicator messages from the screen
when stopping the progress indicator or changing its label.
This is a problem because it interrupts screen readers and leaves no way
to recover what the loading message was by scrolling up in the terminal.

NOTE: this new implementation still interrupts the screen reader when
the a new label is printed, but it does not clear the screen. This makes
the loading messages recoverable, at least.
@BagToad BagToad requested review from andyfeller and Copilot April 15, 2025 01:39
Copy link
Contributor
@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces the GH_SPINNER_DISABLED environment variable to let users disable the spinner animation in favor of a textual progress indicator. Key changes include:

  • Adding a spinnerDisabled field with getter and setter methods to IOStreams.
  • Modifying StartProgressIndicatorWithLabel to conditionally use a textual progress indicator when spinners are disabled.
  • Adding unit tests and updating the factory to correctly interpret various GH_SPINNER_DISABLED environment variable values.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
pkg/iostreams/iostreams_progress_indicator_test.go Added tests to check spinner behavior and textual output across different modes.
pkg/iostreams/iostreams.go Introduced spinnerDisabled flag and added logic for a textual progress indicator.
pkg/cmd/factory/default_test.go Added tests to verify the correct parsing of the GH_SPINNER_DISABLED env var.
pkg/cmd/factory/default.go Updated ioStreams to determine spinner behavior based on the GH_SPINNER_DISABLED env var.

Copy link
Member
@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

Everything looks really good especially with the PTY test! The only actual change requested is making sure the new env var shows up in gh environment; everything is just random thoughts that don't really block this PR.

@BagToad BagToad requested a review from andyfeller April 16, 2025 17:48
@andyfeller
Copy link
Member
andyfeller commented Apr 16, 2025

Instead of Working..., the respective operation e.g.:

  • Searching...
  • Finding assets to download...
  • Downloading...
  • etc.

seem more appropriate here.

I would like to follow up this issue with a more extensive realignment of progress indicator language to avoid the default Working... wherever possible, @iamazeem 💯

Copy link
Member
@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

shipit-squirrel-fireworks

@BagToad BagToad merged commit bff4db1 into trunk Apr 16, 2025
16 checks passed
@BagToad BagToad deleted the kw/opt-into-no-spinners branch April 16, 2025 20:44
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request May 10, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [cli/cli](https://github.com/cli/cli) | minor | `v2.69.0` -> `v2.72.0` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>cli/cli (cli/cli)</summary>

### [`v2.72.0`](https://github.com/cli/cli/releases/tag/v2.72.0): GitHub CLI 2.72.0

[Compare Source](cli/cli@v2.71.2...v2.72.0)

#### :accessibility: Accessibility public preview

This release marks the public preview of several accessibility improvements to the GitHub CLI that have been under development over the past year in partnership with our friends at [Charm](https://github.com/charmbracelet) including:

-   customizable and contrasting colors
-   non-interactive user input prompting
-   text-based spinners

These new experiences are captured in a new `gh a11y` help topic command, which goes into greater detail into the motivation behind each of them as well as opt-in configuration settings / environment variables.

We would like you to share your feedback and join us on this journey through one of [GitHub Accessibility feedback channels](https://accessibility.github.com/feedback)! 🙌

#### What's Changed

##### ✨ Features

-   Introduce `gh accessibility` help topic highlighting GitHub CLI accessibility experiences by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10890
-   \[gh pr view] Support `closingIssuesReferences` JSON field by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10544

##### 🐛 Fixes

-   Fix expected error output of `TestRepo/repo-set-default` by [@&#8203;aconsuegra](https://github.com/aconsuegra) in cli/cli#10884
-   Ensure accessible password and auth token prompters disable echo mode by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10885
-   Fix: Accessible multiselect prompt respects default selections by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10901

#### New Contributors

-   [@&#8203;aconsuegra](https://github.com/aconsuegra) made their first contribution in cli/cli#10884

**Full Changelog**: cli/cli@v2.71.2...v2.72.0

### [`v2.71.2`](https://github.com/cli/cli/releases/tag/v2.71.2): GitHub CLI 2.71.2

[Compare Source](cli/cli@v2.71.1...v2.71.2)

#### What's Changed

-   Fix pr create when push.default tracking and no merge ref by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10863

**Full Changelog**: cli/cli@v2.71.1...v2.71.2

### [`v2.71.1`](https://github.com/cli/cli/releases/tag/v2.71.1): GitHub CLI 2.71.1

[Compare Source](cli/cli@v2.71.0...v2.71.1)

#### What's Changed

-   Fix pr create when branch name contains slashes by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10859

**Full Changelog**: cli/cli@v2.71.0...v2.71.1

### [`v2.71.0`](https://github.com/cli/cli/releases/tag/v2.71.0): GitHub CLI 2.71.0

[Compare Source](cli/cli@v2.70.0...v2.71.0)

#### What's Changed

##### ✨ Features

-   `gh pr create`: Support Git's `@{push}` revision syntax for determining head ref by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10513
-   Introduce option to opt-out of spinners by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10773
-   Update configuration support for accessible colors by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10820
-   `gh config`: add config settings for accessible prompter and disabling spinner by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10846

##### 🐛 Fixes

-   Fix multi pages search for gh search by [@&#8203;leudz](https://github.com/leudz) in cli/cli#10767
-   Fix: `project` commands use shared progress indicator by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10817
-   Issue commands should parse args early by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10811
-   Feature detect v1 projects on `issue view` by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10813
-   Feature detect v1 projects on non web-mode `issue create` by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10815
-   Feature detect v1 projects on web mode issue create by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10818
-   Feature detect v1 projects on issue edit by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10819

##### 📚 Docs & Chores

-   Refactor Sigstore verifier logic by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10750

##### :dependabot: Dependencies

-   chore(deps): bump github.com/sigstore/sigstore-go from 0.7.1 to 0.7.2 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10787
-   Bump google.golang.org/grpc from 1.71.0 to 1.71.1 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10758

#### New Contributors

-   [@&#8203;leudz](https://github.com/leudz) made their first contribution in cli/cli#10767

**Full Changelog**: cli/cli@v2.70.0...v2.71.0

### [`v2.70.0`](https://github.com/cli/cli/releases/tag/v2.70.0): GitHub CLI 2.70.0

[Compare Source](cli/cli@v2.69.0...v2.70.0)

#### Accessibility

This release contains dark shipped changes that are part of a larger GitHub CLI accessibility preview still under development.  More information about these will be announced later this month including various channels to work with GitHub and GitHub CLI maintainers on shaping these experiences.

##### Ensure table headers are thematically contrasting

[#&#8203;8292](cli/cli#8292) is a long time issue where table headers were difficult to see in terminals with light background.  Ahead of the aforementioned preview, `v2.70.0` has shipped changes that improve the out-of-the-box experience based on terminal background detection.

The following screenshots demonstrate the Mac Terminal using the Basic profile, which responds to user's appearance preferences:

<img width="1512" alt="Screenshot of gh repo list in light background terminal" src="https://github.com/user-attachments/assets/87413dde-eec8-43eb-9c16-dc84f8249ddf" />

<img width="1512" alt="Screenshot of gh repo list in dark background terminal" src="https://github.com/user-attachments/assets/7430b42c-7267-402b-b565-a296beb4d5ea" />

For more information including demos from various official distributions, see [#&#8203;10649](cli/cli#10649).

#### What's Changed

##### ✨ Features

-   Update go-gh and document available sprig funcs by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10680
-   Introducing experimental support for rendering markdown with customizable, accessible colors by [@&#8203;andyfeller](https://github.com/andyfeller) [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10680
-   Ensure table datetime columns have thematic, customizable muted text by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10709
-   Ensure table headers are thematically contrasting by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10649
-   Introduce configuration setting for displaying issue and pull request labels in rich truecolor by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10720
-   Ensure muted text is thematic and customizable by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10737
-   \[gh repo create] Show host name in repo creation prompts by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10516
-   Introduce accessible prompter for screen readers (preview) by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10710

##### 🐛 Fixes

-   `run list`: do not fail on organization/enterprise ruleset imposed workflows by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10660
-   Implement safeguard for `gh alias delete` test, prevent wiping out GitHub CLI configuration by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10683
-   Pin third party actions to commit sha by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10731
-   Fallback to job run logs when step logs are missing by [@&#8203;babakks](https://github.com/babakks) in cli/cli#10740
-   \[gh ext] Fix `GitKind` extension directory path by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10609
-   Fix job log resolution to skip legacy logs in favour of normal/new ones by [@&#8203;babakks](https://github.com/babakks) in cli/cli#10769

##### 📚 Docs & Chores

-   `./script/sign` cleanup by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10599
-   Fix typos in CONTRIBUTING.md by [@&#8203;rylwin](https://github.com/rylwin) in cli/cli#10657
-   Improve `gh at verify --help`, document json output by [@&#8203;phillmv](https://github.com/phillmv) in cli/cli#10685
-   Acceptance test issue/pr create/edit with project by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10707
-   Escape dots in regexp pattern in `README.md` by [@&#8203;babakks](https://github.com/babakks) in cli/cli#10742
-   Simplify cosign verification example by not using a regex. by [@&#8203;kommendorkapten](https://github.com/kommendorkapten) in cli/cli#10759
-   Document UNKNOWN STEP in run view by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10770

##### :dependabot: Dependencies

-   Update github.com/sigstore/sigstore-go to 0.7.1 and fix breaking function change by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10749

#### New Contributors

-   [@&#8203;rylwin](https://github.com/rylwin) made their first contribution in cli/cli#10657

**Full Changelog**: cli/cli@v2.69.0...v2.70.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4yNTkuMCIsInVwZGF0ZWRJblZlciI6IjM5LjI2NC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
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.

3 participants
0