[RFC] mconf: make extensive use of ncurses' variables LINES and COLS.

Commit Message

Dirk Gouders May 11, 2013, 11:27 a.m. UTC

Dirk Gouders <[e-mail]> writes:

> "Yann E. MORIN" <[e-mail]> writes:
>
>> If we read the manpage strictly, the LINES and COLS are set by initsrc,
>> and nothing else updates them. So the manpage does not state what
>> happens when the terminal is resized. The only mention of 'COLS' in the
>> man page is this paragraph:
>>
>>     ---8<---
>>     The integer variables LINES and COLS are defined in <curses.h>
>>     and will be filled in by initscr with the size of the screen.
>>     ---8<---
>>
>> After looking at the code of ncurses, the LINES and COLS are also
>> updated upon a resize. But as this is not documented, I think we should
>> *not* rely on that behaviour.
>>
>> I believe we should use the functions, not the variables.
>>
>> Also note that the getmaxyx() familly are not functions, they are macros.
>
> Thank you for your review and comments.
>
> I changed the patch so that the variables are used (or not used)
> strictly according to the documentation.  Of course, the change of
> init_dialog() is not really necessary to comply with the documentation;
> I will send a v3 if it shold remain unchanged.

I'm sorry, I forgot to insert a Signed-off-by line, again.

Dirk
From a8f5ff2e666d467c84ece16e1fe783ac89ceaa60 Mon Sep 17 00:00:00 2001
From: Dirk Gouders <[e-mail]>
Date: Sat, 11 May 2013 12:46:12 +0200
Subject: [PATCH v3] mconf: Use ncurses' variables LINES and COLS according to the
 documentation.

According to the documentation [1], LINES and COLS are initialized by
initscr().  So, use these variables in init_dialog().

The documentation does not say anything about the behavior when
windows are resized.  Do not rely on the current implementation
of ncurses that updates these variables on resize, but use the propper
function calls to get window dimensions.

[1] ncurses(3X)

Signed-off-by: Dirk Gouders <[e-mail]>
---
 scripts/kconfig/lxdialog/checklist.c | 4 ++--
 scripts/kconfig/lxdialog/inputbox.c  | 4 ++--
 scripts/kconfig/lxdialog/menubox.c   | 4 ++--
 scripts/kconfig/lxdialog/textbox.c   | 4 ++--
 scripts/kconfig/lxdialog/util.c      | 5 +----
 scripts/kconfig/lxdialog/yesno.c     | 4 ++--
 6 files changed, 11 insertions(+), 14 deletions(-)

Comments

Yann E. MORIN May 11, 2013, 8:58 p.m. UTC | #1

Dirk, All,

On 2013-05-11 13:27 +0200, Dirk Gouders spake thusly:
> Dirk Gouders <[e-mail]> writes:
[--SNIP--]
> From a8f5ff2e666d467c84ece16e1fe783ac89ceaa60 Mon Sep 17 00:00:00 2001
> From: Dirk Gouders <[e-mail]>
> Date: Sat, 11 May 2013 12:46:12 +0200
> Subject: [PATCH v3] mconf: Use ncurses' variables LINES and COLS according to the
>  documentation.
> 
> According to the documentation [1], LINES and COLS are initialized by
> initscr().  So, use these variables in init_dialog().
> 
> The documentation does not say anything about the behavior when
> windows are resized.  Do not rely on the current implementation
> of ncurses that updates these variables on resize, but use the propper
> function calls to get window dimensions.

You missed three occurences of COLS in:
    scripts/kconfig/lxdialog/util.c:dialog_clear


> diff --git a/scripts/kconfig/lxdialog/util.c b/scripts/kconfig/lxdialog/util.c
> index a0e97c2..9b528a2 100644
> --- a/scripts/kconfig/lxdialog/util.c
> +++ b/scripts/kconfig/lxdialog/util.c
> @@ -309,15 +309,12 @@ void dialog_clear(void)
>   */
>  int init_dialog(const char *backtitle)
>  {
> -	int height, width;
> -
>  	initscr();		/* Init curses */
>  
>  	/* Get current cursor position for signal handler in mconf.c */
>  	getyx(stdscr, saved_y, saved_x);
>  
> -	getmaxyx(stdscr, height, width);
> -	if (height < 19 || width < 80) {
> +	if (LINES < 19 || COLS < 80) {

I know that COLS and LINES are correct right here since we just called
initscr, but for the sake of consistency, I think we'd better use the
functions here, too.

Also, it's a pain to apply your patches, since 'git am' will use the
entire body of the mail as the commit message.

I know you have issues using 'git send-email', but could you please at
least add a scissor-line as the last line of your email body, which will
make 'git am' ignore whatever is above that line (and that line itself):
    ---8<---        <- This is a scissor-line

And please do not attach the patch, add it in the body of the mail.
Use 'git format-patch' to prepare your patch(es), and just send the
resulting file(s) as the mail (hopefully, gnus is able to use an
existing file as the mail to send).

Otherwise, LGTM.

Regards,
Yann E. MORIN.

Patch

diff --git a/scripts/kconfig/lxdialog/checklist.c b/scripts/kconfig/lxdialog/checklist.c
index a2eb80f..af8e8a6 100644
--- a/scripts/kconfig/lxdialog/checklist.c
+++ b/scripts/kconfig/lxdialog/checklist.c
@@ -140,8 +140,8 @@  do_resize:
 	max_choice = MIN(list_height, item_count());
 
 	/* center dialog box on screen */
-	x = (COLS - width) / 2;
-	y = (LINES - height) / 2;
+	x = (getmaxx(stdscr) - width) / 2;
+	y = (getmaxy(stdscr) - height) / 2;
 
 	draw_shadow(stdscr, y, x, height, width);
 
diff --git a/scripts/kconfig/lxdialog/inputbox.c b/scripts/kconfig/lxdialog/inputbox.c
index 21404a0..7a8c6b3e 100644
--- a/scripts/kconfig/lxdialog/inputbox.c
+++ b/scripts/kconfig/lxdialog/inputbox.c
@@ -62,8 +62,8 @@  do_resize:
 		return -ERRDISPLAYTOOSMALL;
 
 	/* center dialog box on screen */
-	x = (COLS - width) / 2;
-	y = (LINES - height) / 2;
+	x = (getmaxx(stdscr) - width) / 2;
+	y = (getmaxy(stdscr) - height) / 2;
 
 	draw_shadow(stdscr, y, x, height, width);
 
diff --git a/scripts/kconfig/lxdialog/menubox.c b/scripts/kconfig/lxdialog/menubox.c
index 48d382e..e068251 100644
--- a/scripts/kconfig/lxdialog/menubox.c
+++ b/scripts/kconfig/lxdialog/menubox.c
@@ -203,8 +203,8 @@  do_resize:
 	max_choice = MIN(menu_height, item_count());
 
 	/* center dialog box on screen */
-	x = (COLS - width) / 2;
-	y = (LINES - height) / 2;
+	x = (getmaxx(stdscr) - width) / 2;
+	y = (getmaxy(stdscr) - height) / 2;
 
 	draw_shadow(stdscr, y, x, height, width);
 
diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
index a48bb93..ad6e199 100644
--- a/scripts/kconfig/lxdialog/textbox.c
+++ b/scripts/kconfig/lxdialog/textbox.c
@@ -98,8 +98,8 @@  do_resize:
 			width = 0;
 
 	/* center dialog box on screen */
-	x = (COLS - width) / 2;
-	y = (LINES - height) / 2;
+	x = (getmaxx(stdscr) - width) / 2;
+	y = (getmaxy(stdscr) - height) / 2;
 
 	draw_shadow(stdscr, y, x, height, width);
 
diff --git a/scripts/kconfig/lxdialog/util.c b/scripts/kconfig/lxdialog/util.c
index a0e97c2..9b528a2 100644
--- a/scripts/kconfig/lxdialog/util.c
+++ b/scripts/kconfig/lxdialog/util.c
@@ -309,15 +309,12 @@  void dialog_clear(void)
  */
 int init_dialog(const char *backtitle)
 {
-	int height, width;
-
 	initscr();		/* Init curses */
 
 	/* Get current cursor position for signal handler in mconf.c */
 	getyx(stdscr, saved_y, saved_x);
 
-	getmaxyx(stdscr, height, width);
-	if (height < 19 || width < 80) {
+	if (LINES < 19 || COLS < 80) {
 		endwin();
 		return -ERRDISPLAYTOOSMALL;
 	}
diff --git a/scripts/kconfig/lxdialog/yesno.c b/scripts/kconfig/lxdialog/yesno.c
index 4e6e809..67b7203 100644
--- a/scripts/kconfig/lxdialog/yesno.c
+++ b/scripts/kconfig/lxdialog/yesno.c
@@ -51,8 +51,8 @@  do_resize:
 		return -ERRDISPLAYTOOSMALL;
 
 	/* center dialog box on screen */
-	x = (COLS - width) / 2;
-	y = (LINES - height) / 2;
+	x = (getmaxx(stdscr) - width) / 2;
+	y = (getmaxy(stdscr) - height) / 2;
 
 	draw_shadow(stdscr, y, x, height, width);