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

Jump to bottom

curl: progress bar refresh #2242

Closed
wants to merge 5 commits into from
Closed

curl: progress bar refresh #2242

wants to merge 5 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Jan 16, 2018 •
edited

... for when the total transfer size is unknown.

Also attempts to get the terminal width properly first using ioctl() now.

The -=O=- "ship" moves when data is transferred. The four flying "hashes" move on each refresh, independent of data.

Demo video: https://youtu.be/8QnL9-5Nl9g

@bagder
curl: progress bar refresh
36b760f 
... also attempts to get the terminal width properly first using ioctl()
now.

The "-=O=-" ship moves when data is transferred. The four flying
"hashes" move on each refresh, independent of data.
jay
jay reviewed Jan 16, 2018
View reviewed changes
src/tool_cb_prg.c
buf[pos] = '#';

fputs(buf, stderr);
bar->tick += 2;
Copy link
Member

Choose a reason for hiding this comment

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

where is the check on tick it seems like this could result in signed overflow

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, I'll add a:

  if(bar->tick >= 200)
    bar->tick -= 200;

@bagder
Copy link
Member Author

bagder commented Jan 18, 2018

The programmatic way of getting the terminal width of course makes it annoying to test (since it'll vary depending on width), since I put the environment variable use as a backup instead of the other way around... Test 1148 for example.

Should I instead allow the presence of a COLUMNS variable use that? Then we can set that in the test suite...

@bagder
fixup: let COLUMNS be first choice, then ioctl()
d8719b8 
... and make the test suite set fixed 79 columns.
@bagder
Copy link
Member Author

bagder commented Jan 20, 2018

I decided to reverse the order. Now the COLUMNS environment variable is checked and used if set, and only if not set curl will use ioctl() to figure out the actual width. This allows us to set COLUMNS to a fixed value in the test suite and make that work independently of the screen width where the tests are run!

@bagder
Copy link
Member Author

bagder commented Jan 22, 2018

Again the macos builds never start so I've killed them. The other builds seem pleased now.

@bagder bagder closed this in 993dd56 Jan 22, 2018
@jay
Copy link
Member

jay commented Jan 22, 2018

Again the macos builds never start so I've killed them. The other builds seem pleased now.

Supposedly it was resolved (again) at some point on the 19th. https://www.traviscistatus.com/incidents/mc9x2wmpnvhg

@bagder bagder deleted the bagder/progress-bar-fly branch January 22, 2018 07:43
Andersbakken pushed a commit to Andersbakken/curl that referenced this pull request Jan 23, 2018
@bagder @Andersbakken
curl: progress bar refresh, get width using ioctl()
a94c45c 
Get screen width from the environment variable COLUMNS first, if set. If
not, use ioctl(). If nether works, assume 79.

Closes curl#2242

The "refresh" is for the -# output when no total transfer size is
known. It will now only use a single updated line even for this case:

The "-=O=-" ship moves when data is transferred. The four flying
"hashes" move (on a sine wave) on each refresh, independent of data.
@mercutiodesign
Copy link

It looks like the manpage is wrong now:

   -#, --progress-bar
          Make  curl  display  transfer  progress as a simple progress bar
          instead of the standard, more informational, meter.

          This progress bar draws a single line of '#'  characters  across
          the screen and shows a percentage if the transfer size is known.
          For transfers without a known size, it will instead  output  one
          '#' character for every 1024 bytes transferred.

Not sure if that is just me, but I would personally prefer the old output style over this one but it does not look like it's configurable.

@bagder bagder mentioned this pull request Jan 29, 2018
Closed
bagder added a commit that referenced this pull request Jan 30, 2018
@bagder
progress-bar.d: update to match implementation
34ecdff 
... since commit 993dd56

Reported-by: Martin Dreher
Bug: #2242 (comment)

Closes #2271
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jay jay jay left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bagder @jay @mercutiodesign