{"action":"created","issue":{"url":"https://api.github.com/repos/doches/progressbar/issues/17","labels_url":"https://api.github.com/repos/doches/progressbar/issues/17/labels{/name}","comments_url":"https://api.github.com/repos/doches/progressbar/issues/17/comments","events_url":"https://api.github.com/repos/doches/progressbar/issues/17/events","html_url":"https://github.com/doches/progressbar/pull/17","id":104981530,"number":17,"title":"Fixed segfault when terminal small enough to cause label cutoff and refactored drawing code","user":{"login":"nibroc","id":892339,"avatar_url":"https://avatars.githubusercontent.com/u/892339?v=3","gravatar_id":"","url":"https://api.github.com/users/nibroc","html_url":"https://github.com/nibroc","followers_url":"https://api.github.com/users/nibroc/followers","following_url":"https://api.github.com/users/nibroc/following{/other_user}","gists_url":"https://api.github.com/users/nibroc/gists{/gist_id}","starred_url":"https://api.github.com/users/nibroc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nibroc/subscriptions","organizations_url":"https://api.github.com/users/nibroc/orgs","repos_url":"https://api.github.com/users/nibroc/repos","events_url":"https://api.github.com/users/nibroc/events{/privacy}","received_events_url":"https://api.github.com/users/nibroc/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":0,"created_at":"2015-09-04T22:35:58Z","updated_at":"2015-09-05T01:15:33Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/doches/progressbar/pulls/17","html_url":"https://github.com/doches/progressbar/pull/17","diff_url":"https://github.com/doches/progressbar/pull/17.diff","patch_url":"https://github.com/doches/progressbar/pull/17.patch"},"body":"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.\r\n\r\nThe 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.\r\n\r\nTo 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. \r\n\r\nTechnical changes:\r\n\r\n* `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.\r\n* `progressbar` no longer stores the terminal type. That's an implementation detail of rendering, and `struct progressbar` is responsible for modeling a progressbar.\r\n* `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.\r\n* 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.\r\n* The format of the ETA string along with some magic constants (amount of whitespace, bar border width, etc) have been converted to constants.\r\n* `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.\r\n* `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.\r\n* The actual bar component of the `progressbar` no longer requires a buffer to render. Instead, it is rendered straight to `stderr`.\r\n* Width calculations have been broken out to be a bit clearer and more contained.\r\n\r\nImprovements in functionality:\r\n\r\n* The bar no longer segfaults when sized below a certain width.\r\n* 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.\r\n* 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)."},"comment":{"url":"https://api.github.com/repos/doches/progressbar/issues/comments/137890929","html_url":"https://github.com/doches/progressbar/pull/17#issuecomment-137890929","issue_url":"https://api.github.com/repos/doches/progressbar/issues/17","id":137890929,"user":{"login":"marcomorain","id":448001,"avatar_url":"https://avatars.githubusercontent.com/u/448001?v=3","gravatar_id":"","url":"https://api.github.com/users/marcomorain","html_url":"https://github.com/marcomorain","followers_url":"https://api.github.com/users/marcomorain/followers","following_url":"https://api.github.com/users/marcomorain/following{/other_user}","gists_url":"https://api.github.com/users/marcomorain/gists{/gist_id}","starred_url":"https://api.github.com/users/marcomorain/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/marcomorain/subscriptions","organizations_url":"https://api.github.com/users/marcomorain/orgs","repos_url":"https://api.github.com/users/marcomorain/repos","events_url":"https://api.github.com/users/marcomorain/events{/privacy}","received_events_url":"https://api.github.com/users/marcomorain/received_events","type":"User","site_admin":false},"created_at":"2015-09-05T01:15:33Z","updated_at":"2015-09-05T01:15:33Z","body":"Nice!\n\nOn Friday, September 4, 2015, Corbin <notifications@github.com> wrote:\n\n> There is currently a bug in progressbar where it will crash if resized too\n> small when using a label that is a string literal (and thus modifiable\n> memory). Really though, even though this bug is caused by using a string\n> literal for the label, it shows a problem with how the progressbar views\n> the label string. It either needs to take ownership of it, meaning that is\n> responsible for memory management of it once the user has provided it\n> (which requires the user to pass a malloc'd string and not a literal...),\n> or it needs to leave the memory management of the label up to the caller.\n>\n> The second approach seems more reasonable so that we can avoid unnecessary\n> copying of strings or putting a burden on the user, but this means that the\n> amount of the string that should be rendered must be either computed or\n> stored in the bar. In other words, we can't str[idx] = 0 a string we\n> don't own.\n>\n> To this end, I started reworking how the progressbar handles rendering of\n> the label, and I might have gotten a bit carried away and ended up\n> refactoring most of the drawing code.\n>\n> Technical changes:\n>\n>    - progressbar no longer modifies the given label at all. Also, it is\n>    now explicitly documented that the user is responsible for ensuring that\n>    the label continues to exist throughout the existence of the progressbar.\n>    - progressbar no longer stores the terminal type. That's an\n>    implementation detail of rendering, and struct progressbar is\n>    responsible for modeling a progressbar.\n>    - step and steps are no longer stored on the progressbar. Again,\n>    they're artifacts of rendering, and they do not belong in the\n>    progressbar object. They are now calculated during rendering.\n>    - Some #define macros were converted to be enum-style constants. This\n>    allows for better typesafety and a higher likelihood that the symbols will\n>    have names in a debugger.\n>    - The format of the ETA string along with some magic constants (amount\n>    of whitespace, bar border width, etc) have been converted to constants.\n>    - progressbar_draw is no longer exposed in the header or exported in\n>    the source. *This is a backward compatibility breaking change*. Anyone\n>    who used the progressbar_draw method will have to alter their code to\n>    be compatible with new versions of progressbar if this PR is accepted.\n>    However, the method does not make much sense to call directly anyway, and\n>    it's documented as internal, so I'm hoping this is acceptable.\n>    - progressbar_draw no longer takes in an ETA but rather calculates it\n>    itself. This allowed for removing a lot of replicated code at various\n>    calling sites to calculate elapsed time, items remaining, etc.\n>    - The actual bar component of the progressbar no longer requires a\n>    buffer to render. Instead, it is rendered straight to stderr.\n>    - Width calculations have been broken out to be a bit clearer and more\n>    contained.\n>\n> Improvements in functionality:\n>\n>    - The bar no longer segfaults when sized below a certain width.\n>    - The bar now resizes instead of sticking to the size of the terminal\n>    when the bar first rendered. Currently it only resizes when the bar is\n>    redrawn, but it should be trivial to add a listener for the terminal\n>    resizing and redraw appropriately if someone were so inclined.\n>    - The bar is now the full width of the terminal instead of having two\n>    trailing blank spaces (I believe there was a bug with the width\n>    calculations where the ETA was given more space than it needed).\n>\n> ------------------------------\n> You can view, comment on, or merge this pull request online at:\n>\n>   https://github.com/doches/progressbar/pull/17\n> Commit Summary\n>\n>    - Fixed segfault when terminal small enough to cause label cutoff and\n>    refactored drawing code.\n>\n> File Changes\n>\n>    - *M* include/progressbar.h\n>    <https://github.com/doches/progressbar/pull/17/files#diff-0> (48)\n>    - *M* lib/progressbar.c\n>    <https://github.com/doches/progressbar/pull/17/files#diff-1> (212)\n>\n> Patch Links:\n>\n>    - https://github.com/doches/progressbar/pull/17.patch\n>    - https://github.com/doches/progressbar/pull/17.diff\n>\n> —\n> Reply to this email directly or view it on GitHub\n> <https://github.com/doches/progressbar/pull/17>.\n>\n"}}