[pacman-dev] [PATCH v3] cache terminal column width and handle SIGWINCH

[Allan McRae]

On 23/11/13 16:38, Simon Gomizelj wrote:
> Instead of doing ioctl each time, cache the result. Handle SIGWINCH to
> we know the size of the terminal has changed (clear the cache).
> ---
> Whitespace problems.
> 
> God damn it, sorry for the spam, its been a while since I've done this.
> I'll take more care in the future.
> 


Finally got around to this...   Sorry for the delay.

Apart from the minor comments below, I notice a lot of the
variable/function names are a bit vague.  As an example, this change:

-	unsigned short termwidth = 0;
+	unsigned short columns = 0;

The old variable name is much more descriptive.  Please prefix "columns"
in all its uses with (e.g.) term so we know what columns is referring to.

>  src/pacman/pacman.c | 10 ++++++++++
>  src/pacman/util.c   | 24 +++++++++++++++++++-----
>  src/pacman/util.h   |  1 +
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c
> index df73bcf..1643a63 100644
> --- a/src/pacman/pacman.c
> +++ b/src/pacman/pacman.c
> @@ -1044,6 +1044,16 @@ int main(int argc, char *argv[])
>  		}
>  	}
>  

Adding a comment here would be nice.  e.g.:

/* Set up signal handler to deal with terminal column width changes */

> +	new_action.sa_handler = columns_reset;
> +	sigemptyset(&new_action.sa_mask);
> +	new_action.sa_flags = 0;
> +
> +	sigaction(SIGWINCH, NULL, &old_action);
> +	if(old_action.sa_handler != SIG_IGN) {
> +		sigaction(SIGWINCH, &new_action, NULL);
> +	}
> +
> +
>  	/* i18n init */
>  #if defined(ENABLE_NLS)
>  	localize();
> diff --git a/src/pacman/util.c b/src/pacman/util.c
> index 23eea98..b08ad43 100644
> --- a/src/pacman/util.c
> +++ b/src/pacman/util.c
> @@ -46,6 +46,7 @@
>  #include "conf.h"
>  #include "callback.h"
>  
> +static volatile unsigned cached_columns = 0;
>  
>  struct table_cell_t {
>  	char *label;
> @@ -155,29 +156,42 @@ static int flush_term_input(int fd)
>  	return 0;
>  }
>  
> +/* intended to be used as a SIGWINCH handler */
> +void columns_reset(int signum)
> +{
> +	cached_columns = 0;
> +}
> +

util.c: In function ‘columns_reset’:
util.c:160:24: error: unused parameter ‘signum’ [-Werror=unused-parameter]
 void columns_reset(int signum)
                        ^

We need:
void columns_reset(int __attribute__((unused)) signum)

or we could move the UNUSED define into src/common/util-common.h


>  /* gets the current screen column width */
>  unsigned short getcols(int fd)
>  {
>  	const unsigned short default_tty = 80;
>  	const unsigned short default_notty = 0;
> -	unsigned short termwidth = 0;
> +	unsigned short columns = 0;
> +
> +	if(cached_columns > 0) {
> +		return cached_columns;
> +	}
>  
>  	if(!isatty(fd)) {
> -		return default_notty;
> +		cached_columns = default_notty;
> +		return cached_columns;
>  	}
>  
>  #if defined(TIOCGSIZE)
>  	struct ttysize win;
>  	if(ioctl(fd, TIOCGSIZE, &win) == 0) {
> -		termwidth = win.ts_cols;
> +		columns = win.ts_cols;
>  	}
>  #elif defined(TIOCGWINSZ)
>  	struct winsize win;
>  	if(ioctl(fd, TIOCGWINSZ, &win) == 0) {
> -		termwidth = win.ws_col;
> +		columns = win.ws_col;
>  	}
>  #endif
> -	return termwidth == 0 ? default_tty : termwidth;
> +
> +	cached_columns = columns == 0 ? default_tty : columns;
> +	return cached_columns;
>  }
>  
>  /* does the same thing as 'rm -rf' */
> diff --git a/src/pacman/util.h b/src/pacman/util.h
> index e2297f8..d7f7b3c 100644
> --- a/src/pacman/util.h
> +++ b/src/pacman/util.h
> @@ -49,6 +49,7 @@ int trans_init(alpm_transflag_t flags, int check_valid);
>  int trans_release(void);
>  int needs_root(void);
>  int check_syncdbs(size_t need_repos, int check_valid);
> +void columns_reset(int signum);
>  unsigned short getcols(int fd);
>  int rmrf(const char *path);
>  void indentprint(const char *str, unsigned short indent, unsigned short cols);
>