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

curl: progress bar refresh #2242

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@bagder
Member

bagder commented Jan 16, 2018

... 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

curl: progress bar refresh
... 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.
buf[pos] = '#';
fputs(buf, stderr);
bar->tick += 2;

This comment has been minimized.

@jay

jay Jan 16, 2018

Member

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

This comment has been minimized.

@bagder

bagder Jan 17, 2018

Member

Indeed, I'll add a:

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

bagder added some commits Jan 17, 2018

@bagder

This comment has been minimized.

Member

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...

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

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 Jan 22, 2018

Andersbakken added a commit to Andersbakken/curl that referenced this pull request Jan 23, 2018

curl: progress bar refresh, get width using ioctl()
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

This comment has been minimized.

mercutiodesign commented Jan 28, 2018

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 added a commit that referenced this pull request Jan 30, 2018

progress-bar.d: update to match implementation
... 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.