Fixed segfault when terminal small enough to cause label cutoff and refactored drawing code

[nibroc]

There is currently a bug in progressbar where it will crash if resized too small when using a label that is a string literal (and thus modifiable memory). Really though, even though this bug is caused by using a string literal for the label, it shows a problem with how the progressbar views the label string. It either needs to take ownership of it, meaning that is responsible for memory management of it once the user has provided it (which requires the user to pass a malloc'd string and not a literal...), or it needs to leave the memory management of the label up to the caller.

The second approach seems more reasonable so that we can avoid unnecessary copying of strings or putting a burden on the user, but this means that the amount of the string that should be rendered must be either computed or stored in the bar. In other words, we can't str[idx] = 0 a string we don't own.

To this end, I started reworking how the progressbar handles rendering of the label, and I might have gotten a bit carried away and ended up refactoring most of the drawing code.

Technical changes:

• progressbar no longer modifies the given label at all. Also, it is now explicitly documented that the user is responsible for ensuring that the label continues to exist throughout the existence of the progressbar.
• progressbar no longer stores the terminal type. That's an implementation detail of rendering, and struct progressbar is responsible for modeling a progressbar.
• step and steps are no longer stored on the progressbar. Again, they're artifacts of rendering, and they do not belong in the progressbar object. They are now calculated during rendering.
• Some #define macros were converted to be enum-style constants. This allows for better typesafety and a higher likelihood that the symbols will have names in a debugger.
• The format of the ETA string along with some magic constants (amount of whitespace, bar border width, etc) have been converted to constants.
• progressbar_draw is no longer exposed in the header or exported in the source. This is a backward compatibility breaking change. Anyone who used the progressbar_draw method will have to alter their code to be compatible with new versions of progressbar if this PR is accepted. However, the method does not make much sense to call directly anyway, and it's documented as internal, so I'm hoping this is acceptable.
• progressbar_draw no longer takes in an ETA but rather calculates it itself. This allowed for removing a lot of replicated code at various calling sites to calculate elapsed time, items remaining, etc.
• The actual bar component of the progressbar no longer requires a buffer to render. Instead, it is rendered straight to stderr.
• Width calculations have been broken out to be a bit clearer and more contained.

Improvements in functionality:

• The bar no longer segfaults when sized below a certain width.
• The bar now resizes instead of sticking to the size of the terminal when the bar first rendered. Currently it only resizes when the bar is redrawn, but it should be trivial to add a listener for the terminal resizing and redraw appropriately if someone were so inclined.
• The bar is now the full width of the terminal instead of having two trailing blank spaces (I believe there was a bug with the width calculations where the ETA was given more space than it needed).

[marcomorain]

Nice!

On Friday, September 4, 2015, Corbin notifications@github.com wrote:

There is currently a bug in progressbar where it will crash if resized too
small when using a label that is a string literal (and thus modifiable
memory). Really though, even though this bug is caused by using a string
literal for the label, it shows a problem with how the progressbar views
the label string. It either needs to take ownership of it, meaning that is
responsible for memory management of it once the user has provided it
(which requires the user to pass a malloc'd string and not a literal...),
or it needs to leave the memory management of the label up to the caller.

The second approach seems more reasonable so that we can avoid unnecessary
copying of strings or putting a burden on the user, but this means that the
amount of the string that should be rendered must be either computed or
stored in the bar. In other words, we can't str[idx] = 0 a string we
don't own.

To this end, I started reworking how the progressbar handles rendering of
the label, and I might have gotten a bit carried away and ended up
refactoring most of the drawing code.

Technical changes:

• progressbar no longer modifies the given label at all. Also, it is
  now explicitly documented that the user is responsible for ensuring that
  the label continues to exist throughout the existence of the progressbar.
• progressbar no longer stores the terminal type. That's an
  implementation detail of rendering, and struct progressbar is
  responsible for modeling a progressbar.
• step and steps are no longer stored on the progressbar. Again,
  they're artifacts of rendering, and they do not belong in the
  progressbar object. They are now calculated during rendering.
• Some #define macros were converted to be enum-style constants. This
  allows for better typesafety and a higher likelihood that the symbols will
  have names in a debugger.
• The format of the ETA string along with some magic constants (amount
  of whitespace, bar border width, etc) have been converted to constants.
• progressbar_draw is no longer exposed in the header or exported in
  the source. This is a backward compatibility breaking change. Anyone
  who used the progressbar_draw method will have to alter their code to
  be compatible with new versions of progressbar if this PR is accepted.
  However, the method does not make much sense to call directly anyway, and
  it's documented as internal, so I'm hoping this is acceptable.
• progressbar_draw no longer takes in an ETA but rather calculates it
  itself. This allowed for removing a lot of replicated code at various
  calling sites to calculate elapsed time, items remaining, etc.
• The actual bar component of the progressbar no longer requires a
  buffer to render. Instead, it is rendered straight to stderr.
• Width calculations have been broken out to be a bit clearer and more
  contained.

Improvements in functionality:

• The bar no longer segfaults when sized below a certain width.
• The bar now resizes instead of sticking to the size of the terminal
  when the bar first rendered. Currently it only resizes when the bar is
  redrawn, but it should be trivial to add a listener for the terminal
  resizing and redraw appropriately if someone were so inclined.
• The bar is now the full width of the terminal instead of having two
  trailing blank spaces (I believe there was a bug with the width
  calculations where the ETA was given more space than it needed).

---

You can view, comment on, or merge this pull request online at:

#17
Commit Summary

• Fixed segfault when terminal small enough to cause label cutoff and
  refactored drawing code.

File Changes

• M include/progressbar.h
  https://github.com/doches/progressbar/pull/17/files#diff-0 (48)
• M lib/progressbar.c
  https://github.com/doches/progressbar/pull/17/files#diff-1 (212)

Patch Links:

• https://github.com/doches/progressbar/pull/17.patch
• https://github.com/doches/progressbar/pull/17.diff

—
Reply to this email directly or view it on GitHub
#17.

[doches]

This looks great, and is a fantastic refactor. Thanks for this!