[pacman-dev,v2] Hide cursor while pacman is running

Message ID 20200306003919.432468-1-anatol.pomozov@gmail.com
State Superseded, archived
Headers show
Series [pacman-dev,v2] Hide cursor while pacman is running | expand

Commit Message

Anatol Pomozov March 6, 2020, 12:39 a.m. UTC
Use ASCII control codes to hide cursor at the pacman start and then
show the cursor when pacman finishes.

It helps to avoid annoying blinking when progress bars are re-drawn.

Cursor is reenabled if pacman expects user's input.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 src/common/util-common.c | 22 ++++++++++++++++++++++
 src/common/util-common.h |  3 +++
 src/pacman/pacman.c      |  2 ++
 src/pacman/sighandler.c  |  5 +++++
 4 files changed, 32 insertions(+)

Comments

Allan McRae March 8, 2020, 5:04 a.m. UTC | #1
On 6/3/20 10:39 am, Anatol Pomozov wrote:
> Use ASCII control codes to hide cursor at the pacman start and then
> show the cursor when pacman finishes.
> 
> It helps to avoid annoying blinking when progress bars are re-drawn.
> 
> Cursor is reenabled if pacman expects user's input.
> 
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
>  src/common/util-common.c | 22 ++++++++++++++++++++++
>  src/common/util-common.h |  3 +++
>  src/pacman/pacman.c      |  2 ++
>  src/pacman/sighandler.c  |  5 +++++
>  4 files changed, 32 insertions(+)
> 
> diff --git a/src/common/util-common.c b/src/common/util-common.c
> index 7d43ac0d..09a41ea6 100644
> --- a/src/common/util-common.c
> +++ b/src/common/util-common.c
> @@ -21,6 +21,7 @@
>  #include <errno.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <unistd.h>
>  
>  #include "util-common.h"
>  
> @@ -104,6 +105,18 @@ int llstat(char *path, struct stat *buf)
>  	return ret;
>  }
>  
> +void console_hide_cursor(void) {
> +	if(isatty(fileno(stdout))) {

I don't think we should run this every line we read.

In fact, I doubt that is needed anywhere in libalpm currently.
safe_fgets is used to read the localdb and when grepping install files
for functions.  And libalpm should really not output anything...

> +		printf("\x1B[?25l");
> +	}
> +}
> +
> +void console_show_cursor(void) {
> +	if(isatty(fileno(stdout))) {
> +		printf("\x1B[?25h");
> +	}
> +}
> +
>  /** Wrapper around fgets() which properly handles EINTR
>   * @param s string to read into
>   * @param size maximum length to read
> @@ -114,6 +127,12 @@ char *safe_fgets(char *s, int size, FILE *stream)
>  {
>  	char *ret;
>  	int errno_save = errno, ferror_save = ferror(stream);
> +	int is_tty = isatty(fileno(stream));
> +
> +	if(is_tty) > +		/* Show cursor if read from STDIN */
> +		console_show_cursor();
> +	}

I don't like having to do this every time we read something.  And now we
are checking that "stream" is a tty and then "stdout" is a tty in the
console_*_cursor() functions.

It would be better to add a "safe_fgets_stdin()" in src/pacman/util.c
that wraps safe_fgets() and does the cursor on/off when pacman is asking
for input.  That is where we need it.

A
Anatol Pomozov March 8, 2020, 8:22 p.m. UTC | #2
Hello

On Sat, Mar 7, 2020 at 9:55 PM Allan McRae <allan@archlinux.org> wrote:
>
> On 6/3/20 10:39 am, Anatol Pomozov wrote:
> > Use ASCII control codes to hide cursor at the pacman start and then
> > show the cursor when pacman finishes.
> >
> > It helps to avoid annoying blinking when progress bars are re-drawn.
> >
> > Cursor is reenabled if pacman expects user's input.
> >
> > Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> > ---
> >  src/common/util-common.c | 22 ++++++++++++++++++++++
> >  src/common/util-common.h |  3 +++
> >  src/pacman/pacman.c      |  2 ++
> >  src/pacman/sighandler.c  |  5 +++++
> >  4 files changed, 32 insertions(+)
> >
> > diff --git a/src/common/util-common.c b/src/common/util-common.c
> > index 7d43ac0d..09a41ea6 100644
> > --- a/src/common/util-common.c
> > +++ b/src/common/util-common.c
> > @@ -21,6 +21,7 @@
> >  #include <errno.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > +#include <unistd.h>
> >
> >  #include "util-common.h"
> >
> > @@ -104,6 +105,18 @@ int llstat(char *path, struct stat *buf)
> >       return ret;
> >  }
> >
> > +void console_hide_cursor(void) {
> > +     if(isatty(fileno(stdout))) {
>
> I don't think we should run this every line we read.

We actually need this particular check when we print the line.

The rationale here is to avoid printing the escape sequence if user
redirects pacman output to a file. Escape symbols make sense only for
a terminal.

> In fact, I doubt that is needed anywhere in libalpm currently.
> safe_fgets is used to read the localdb and when grepping install files
> for functions.  And libalpm should really not output anything...
>
> > +             printf("\x1B[?25l");
> > +     }
> > +}
> > +
> > +void console_show_cursor(void) {
> > +     if(isatty(fileno(stdout))) {
> > +             printf("\x1B[?25h");
> > +     }
> > +}
> > +
> >  /** Wrapper around fgets() which properly handles EINTR
> >   * @param s string to read into
> >   * @param size maximum length to read
> > @@ -114,6 +127,12 @@ char *safe_fgets(char *s, int size, FILE *stream)
> >  {
> >       char *ret;
> >       int errno_save = errno, ferror_save = ferror(stream);
> > +     int is_tty = isatty(fileno(stream));
> > +
> > +     if(is_tty) > +          /* Show cursor if read from STDIN */
> > +             console_show_cursor();
> > +     }
>
> I don't like having to do this every time we read something.  And now we
> are checking that "stream" is a tty and then "stdout" is a tty in the
> console_*_cursor() functions.
>
> It would be better to add a "safe_fgets_stdin()" in src/pacman/util.c
> that wraps safe_fgets() and does the cursor on/off when pacman is asking
> for input.  That is where we need it.

Yep, it makes sense. PTAL to upcoming patch.

Patch

diff --git a/src/common/util-common.c b/src/common/util-common.c
index 7d43ac0d..09a41ea6 100644
--- a/src/common/util-common.c
+++ b/src/common/util-common.c
@@ -21,6 +21,7 @@ 
 #include <errno.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
 
 #include "util-common.h"
 
@@ -104,6 +105,18 @@  int llstat(char *path, struct stat *buf)
 	return ret;
 }
 
+void console_hide_cursor(void) {
+	if(isatty(fileno(stdout))) {
+		printf("\x1B[?25l");
+	}
+}
+
+void console_show_cursor(void) {
+	if(isatty(fileno(stdout))) {
+		printf("\x1B[?25h");
+	}
+}
+
 /** Wrapper around fgets() which properly handles EINTR
  * @param s string to read into
  * @param size maximum length to read
@@ -114,6 +127,12 @@  char *safe_fgets(char *s, int size, FILE *stream)
 {
 	char *ret;
 	int errno_save = errno, ferror_save = ferror(stream);
+	int is_tty = isatty(fileno(stream));
+
+	if(is_tty) {
+		/* Show cursor if read from STDIN */
+		console_show_cursor();
+	}
 	while((ret = fgets(s, size, stream)) == NULL && !feof(stream)) {
 		if(errno == EINTR) {
 			/* clear any errors we set and try again */
@@ -125,6 +144,9 @@  char *safe_fgets(char *s, int size, FILE *stream)
 			break;
 		}
 	}
+	if(is_tty) {
+		console_hide_cursor();
+	}
 	return ret;
 }
 
diff --git a/src/common/util-common.h b/src/common/util-common.h
index 483d5da4..8eacde7e 100644
--- a/src/common/util-common.h
+++ b/src/common/util-common.h
@@ -28,6 +28,9 @@  char *mdirname(const char *path);
 
 int llstat(char *path, struct stat *buf);
 
+void console_hide_cursor(void);
+void console_show_cursor(void);
+
 char *safe_fgets(char *s, int size, FILE *stream);
 
 void wordsplit_free(char **ws);
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c
index d58c428a..aafd9e63 100644
--- a/src/pacman/pacman.c
+++ b/src/pacman/pacman.c
@@ -300,6 +300,7 @@  static void cleanup(int ret)
 
 	/* free memory */
 	FREELIST(pm_targets);
+	console_show_cursor();
 	exit(ret);
 }
 
@@ -1084,6 +1085,7 @@  int main(int argc, char *argv[])
 	int ret = 0;
 	uid_t myuid = getuid();
 
+	console_hide_cursor();
 	install_segv_handler();
 
 	/* i18n init */
diff --git a/src/pacman/sighandler.c b/src/pacman/sighandler.c
index 46e6481f..59aaa61f 100644
--- a/src/pacman/sighandler.c
+++ b/src/pacman/sighandler.c
@@ -27,6 +27,9 @@ 
 #include "sighandler.h"
 #include "util.h"
 
+/* the same ANSI sequence as used in console_show_cursor */
+#define SHOW_CURSOR_ANSI "\x1B[?25h"
+
 /** Write function that correctly handles EINTR.
  */
 static ssize_t xwrite(int fd, const void *buf, size_t count)
@@ -60,6 +63,7 @@  static void soft_interrupt_handler(int signum)
 		const char msg[] = "\nHangup signal received\n";
 		xwrite(STDERR_FILENO, msg, ARRAYSIZE(msg) - 1);
 	}
+	xwrite(STDOUT_FILENO, SHOW_CURSOR_ANSI, sizeof(SHOW_CURSOR_ANSI) - 1);
 	if(alpm_trans_interrupt(config->handle) == 0) {
 		/* a transaction is being interrupted, don't exit pacman yet. */
 		return;
@@ -95,6 +99,7 @@  static void segv_handler(int signum)
 	const char msg[] = "\nerror: segmentation fault\n"
 		"Please submit a full bug report with --debug if appropriate.\n";
 	xwrite(STDERR_FILENO, msg, sizeof(msg) - 1);
+	xwrite(STDOUT_FILENO, SHOW_CURSOR_ANSI, sizeof(SHOW_CURSOR_ANSI) - 1);
 
 	/* restore the default handler */
 	_reset_handler(signum);