RFC: let lxdialog/util.c:print_autowrap() handle newline characters
Hello Michal,
this is just a cosmetical issue and probably I oversee a reason why print_autowrap() ignores newline characters:
When I exit menuconfig with unsaved changes I get a dialog that looks like
Do you wish to save your new configuration ? <ESC><ESC> to continue.
and I thought that could look prettier (and probably should use complete sentences).
In mconf.c, the above text is defined with a newline, so I guess the author expected it to be processed, but the comment before print_autowrap() clearly states that newline characters will be replaced by spaces. I am not sure if this is a must -- I did not find a reason why print_autowrap() should not process newline characters and gave it a try.
Attached is a first version of a patch. There are other (indirect) users of print_autowrap() but for now I just modified the text for the exit dialog.
Dirk
From b09a6b6bb171ebc3cd0db0605a9ab32554eadb6b Mon Sep 17 00:00:00 2001
From: Dirk Gouders <[email protected]>
Date: Wed, 8 May 2013 13:04:44 +0200
Subject: [PATCH] mconf: let print_autowrap() handle newline characters.
When exiting menuconfig with unsaved changes, a dialog like the following is shown
Do you wish to save your new configuration ? <ESC><ESC> to continue.
The author of the dialog text specified a newline and probably expected it to be processed, so let print_autowrap() handle newlines propperly.
--- scripts/kconfig/lxdialog/util.c | 34 ++++++++++++++++++++++------------ scripts/kconfig/mconf.c | 8 ++++---- 2 files changed, 26 insertions(+), 16 deletions(-)
scripts/kconfig/lxdialog/util.c +22 -12
@@ -371,27 +371,19 @@ void print_title(WINDOW *dialog, const char *title, int width)/* * Print a string of text in a window, automatically wrap around to the * next line if the string is too long to fit on one line. Newline -* characters '\n' are replaced by spaces. We start on a new line +* characters '\n' are propperly processed. We start on a new line * if there is no room for at least 4 nonblanks following a double-space. */ void print_autowrap (WINDOW * win,const char *prompt,int width,int y,int x) {int newl, cur_x, cur_y; -int i, prompt_len, room, wlen; -char tempstr[MAX_LEN +1 ], *word, *sp, *sp2; +int prompt_len, room, wlen; +char tempstr[MAX_LEN +1 ], *word, *sp, *sp2, *newline;strcpy (tempstr, prompt); prompt_len =strlen (tempstr); -/* -* Remove newlines -*/ -for (i =0 ; i < prompt_len; i++) { -if (tempstr[i] =='\n' ) - tempstr[i] =' ' ; - } -if (prompt_len <= width - x *2 ) {/* If prompt is short */ wmove (win, y, (width - prompt_len) /2 );waddstr (win, tempstr);
@@ -402,6 +394,17 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x) word = tempstr;while (word && *word) { sp =strchr (word,' ' ); + newline =strchr (word,'\n' ); + +/* If we found a newline, then check if there are other +word separators (spaces) in front of it that we must respect. */ +if (newline) { +if (sp && (newline > sp)) + newline =0 ; +else { + sp = newline; + } + }if (sp) *sp++ =0 ;
@@ -421,7 +424,14 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)wmove (win, cur_y, cur_x);waddstr (win, word);getyx (win, cur_y, cur_x); - cur_x++; + +/* Move to the next line if the word separator was a newline */ +if (newline) { + cur_y++; + cur_x = x; + }else + cur_x++; +if (sp && *sp ==' ' ) { cur_x++;/* double space */ while (*++sp ==' ' ) ;
scripts/kconfig/mconf.c +4 -4
@@ -955,10 +955,10 @@ static int handle_exit(void)reset_subtitle ();dialog_clear ();if (conf_get_changed ()) - res =dialog_yesno (NULL , -_ ("Do you wish to save your new configuration ?\n" -"<ESC><ESC> to continue." ), -6 ,60 ); + res =dialog_yesno ("Exit Kernel Configuration" , +_ ("Do you wish to save your new configuration?\n\n" +"(Press <ESC><ESC> to continue kernel configuration.)" ), +8 ,60 );else res =-1 ;
Signature · 1 line
1.8.2.1
Quoted 3 lines · Dirk Gouders
Attached is a first version of a patch. There are other (indirect) users of
print_autowrap()but for now I just modified the text for the exit dialog.
Sorry for the noise, but I noticed that my patch did not propperly handle cases with short first words of new sentences. While at it I noticed that strpbrk() makes the code more readable.
Dirk
From 490668696c253bef74e8acd2ff943f20cc4da26b Mon Sep 17 00:00:00 2001
From: Dirk Gouders <[email protected]>
Date: Wed, 8 May 2013 14:28:30 +0200
Subject: [PATCH] mconf: let print_autowrap() handle newline characters.
When exiting menuconfig with unsaved changes, a dialog like the following is shown
Do you wish to save your new configuration ? <ESC><ESC> to continue.
The author of the dialog text specified a newline and probably expected it to be processed, so let print_autowrap() handle newlines propperly.
--- scripts/kconfig/lxdialog/util.c | 31 +++++++++++++++++-------------- scripts/kconfig/mconf.c | 4 ++-- 2 files changed, 19 insertions(+), 16 deletions(-)
scripts/kconfig/lxdialog/util.c +17 -14
@@ -371,27 +371,19 @@ void print_title(WINDOW *dialog, const char *title, int width)/* * Print a string of text in a window, automatically wrap around to the * next line if the string is too long to fit on one line. Newline -* characters '\n' are replaced by spaces. We start on a new line +* characters '\n' are propperly processed. We start on a new line * if there is no room for at least 4 nonblanks following a double-space. */ void print_autowrap (WINDOW * win,const char *prompt,int width,int y,int x) {int newl, cur_x, cur_y; -int i, prompt_len, room, wlen; -char tempstr[MAX_LEN +1 ], *word, *sp, *sp2; +int prompt_len, room, wlen; +char tempstr[MAX_LEN +1 ], *word, *sp, *sp2, *newline_separator;strcpy (tempstr, prompt); prompt_len =strlen (tempstr); -/* -* Remove newlines -*/ -for (i =0 ; i < prompt_len; i++) { -if (tempstr[i] =='\n' ) - tempstr[i] =' ' ; - } -if (prompt_len <= width - x *2 ) {/* If prompt is short */ wmove (win, y, (width - prompt_len) /2 );waddstr (win, tempstr);
@@ -401,7 +393,10 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x) newl =1 ; word = tempstr;while (word && *word) { - sp =strchr (word,' ' ); + sp =strpbrk (word,"\n " ); +if (sp && *sp =='\n' ) + newline_separator = sp; +if (sp) *sp++ =0 ;
@@ -413,7 +408,7 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)if (wlen > room || (newl && wlen <4 && sp && wlen +1 +strlen (sp) > room - && (!(sp2 =strchr (sp,' ' )) + && (!(sp2 =strpbrk (sp,"\n " )) || wlen +1 + (sp2 - sp) > room))) { cur_y++; cur_x = x;
@@ -421,7 +416,15 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)wmove (win, cur_y, cur_x);waddstr (win, word);getyx (win, cur_y, cur_x); - cur_x++; + +/* Move to the next line if the word separator was a newline */ +if (newline_separator) { + cur_y++; + cur_x = x; + newline_separator =0 ; + }else + cur_x++; +if (sp && *sp ==' ' ) { cur_x++;/* double space */ while (*++sp ==' ' ) ;
scripts/kconfig/mconf.c +2 -2
@@ -956,8 +956,8 @@ static int handle_exit(void)dialog_clear ();if (conf_get_changed ()) res =dialog_yesno (NULL , -_ ("Do you wish to save your new configuration ?\n" -"<ESC><ESC> to continue." ), +_ ("Do you wish to save your new configuration?\n" +"(Press <ESC><ESC> to continue kernel configuration.)" ),6 ,60 );else res =-1 ;
Signature · 1 line
1.8.2.1
Dirk, All,
Quoted 7 lines · Dirk Gouders
Quoted 3 lines
Attached is a first version of a patch. There are other (indirect) users of
print_autowrap()but for now I just modified the text for the exit dialog.Sorry for the noise, but I noticed that my patch did not propperly handle cases with short first words of new sentences. While at it I noticed that
strpbrk()makes the code more readable.
Next time you have to send a patch, could you please:
- use 'git send-email': that will properly format the patch for sending via email, especially, it does not attach the patch, but in-lines it in the body of the mail, which makes it easier to review
- when you send a second (or third...) version of a patch, append a 'v2' (or v3, and so on...) to the subject, like: [PATCH v2] Bla blabla...
Quoted 10 lines
From
490668696c253bef74e8acd2ff943f20cc4da26bMon Sep 17 00:00:00 2001
From: Dirk Gouders <[email protected]>
Date: Wed, 8 May 2013 14:28:30 +0200
Subject: [PATCH] mconf: letprint_autowrap()handle newline characters.When exiting menuconfig with unsaved changes, a dialog like the following is shown
Do you wish to save your new configuration ? <ESC><ESC> to continue.
Did you try to replace all the spaces there with newlines, and see what happens? Hint: it's ugly, but not because of the newlines: the window is not resized accordingly to the new number of lines, so better fix that before, too.
Quoted 2 lines
The author of the dialog text specified a newline and probably expected it to be processed, so let
print_autowrap()handle newlines propperly.
Also, please add your SoB (Signed-off-by) line:
http://elinux.org/Developer_Certificate_Of_Origin
Otherwise, looks good. I'll do a more thorough review (and testing) later (hopefully when you have fixed that window height issue).
Thank you!
Regards,
Yann E. MORIN.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'
Quoted 11 lines · Yann E. MORIN
Dirk, All,
Quoted 7 lines · Dirk Gouders
Quoted 3 lines
Attached is a first version of a patch. There are other (indirect) users of
print_autowrap()but for now I just modified the text for the exit dialog.Sorry for the noise, but I noticed that my patch did not propperly handle cases with short first words of new sentences. While at it I noticed that
strpbrk()makes the code more readable.
Quoted 6 lines
Next time you have to send a patch, could you please:
- use 'git send-email': that will properly format the patch for sending via email, especially, it does not attach the patch, but in-lines it in the body of the mail, which makes it easier to review
- when you send a second (or third...) version of a patch, append a 'v2' (or v3, and so on...) to the subject, like: [PATCH v2] Bla blabla...
Sorry for the inconveniences, I will do my best with the revised version of the patch.
Quoted 7 lines
Quoted 2 lines
Do you wish to save your new configuration ? <ESC><ESC> to continue.
Did you try to replace all the spaces there with newlines, and see what happens? Hint: it's ugly, but not because of the newlines: the window is not resized accordingly to the new number of lines, so better fix that before, too.
Yes, I noticed that when I put just two newlines into that text. In general it happens for any text (also without newlines) that does not fit the window height.
I solved the problem by adjusting the third parameter in the call to dialog_yesno() and thought that that is the correct way to handle the window-height-problem if the text does not fit, because print_autowrap() currently does not care at all if the given text breaks the window.
But I agree, I would be nice if print_autowrap() could handle such cases and I will see if I find a way to do that. I am unsure if that should go into one single patch/commit, though.
Quoted 8 lines
Quoted 2 lines
The author of the dialog text specified a newline and probably expected it to be processed, so let
print_autowrap()handle newlines propperly.Also, please add your SoB (Signed-off-by) line:
http://elinux.org/Developer_Certificate_Of_OriginOtherwise, looks good. I'll do a more thorough review (and testing) later (hopefully when you have fixed that window height issue).
Thank you for your first prompt feedback.
Dirk
Dirk, All,
Quoted 17 lines · Dirk Gouders
Quoted 9 lines · Yann E. MORIN
Quoted 7 lines · Dirk Gouders
Quoted 3 lines
Attached is a first version of a patch. There are other (indirect) users of
print_autowrap()but for now I just modified the text for the exit dialog.Sorry for the noise, but I noticed that my patch did not propperly handle cases with short first words of new sentences. While at it I noticed that
strpbrk()makes the code more readable.Quoted 6 lines
Next time you have to send a patch, could you please:
- use 'git send-email': that will properly format the patch for sending via email, especially, it does not attach the patch, but in-lines it in the body of the mail, which makes it easier to review
- when you send a second (or third...) version of a patch, append a 'v2' (or v3, and so on...) to the subject, like: [PATCH v2] Bla blabla...
Quoted 2 lines
Sorry for the inconveniences, I will do my best with the revised version of the patch.
No problem, I was just giving some hints! :-)
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
Do you wish to save your new configuration ? <ESC><ESC> to continue.
Did you try to replace all the spaces there with newlines, and see what happens? Hint: it's ugly, but not because of the newlines: the window is not resized accordingly to the new number of lines, so better fix that before, too.
Quoted 8 lines
Yes, I noticed that when I put just two newlines into that text. In general it happens for any text (also without newlines) that does not fit the window height.
I solved the problem by adjusting the third parameter in the call to
dialog_yesno()and thought that that is the correct way to handle the window-height-problem if the text does not fit, becauseprint_autowrap()currently does not care at all if the given text breaks the window.
You are right: it is a non-issue so far; all rendered text currently fit in their respective windows. If those texts get updated, it will be the responsibility to the author to adjust the windows height.
Note: the english texts do fit, but I did not check that all translations do. But are there anyone really using the localised texts at all?
I'll get your patch as-is in my tree. Depending on Michal, maybe we can queue it in for 3.10. Not too sure, though: the merge window is on the verge of closing, and I'd prefer changes linger for a while in Michal' then next's trees before getting mainlined.
Quoted 3 lines
But I agree, I would be nice if
print_autowrap()could handle such cases and I will see if I find a way to do that. I am unsure if that should go into one single patch/commit, though.
Not needed for now, but if you want to work on this, then please do one patch for each semantically self-contained change. Adjusting the height of the window is not tied to rendering the '\n'.
Regards,
Yann E. MORIN.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'
PATCH v3 1/1 let lxdialog/util.c:print_autowrap() handle newline characters
Quoted 11 lines · Yann E. MORIN
Quoted 9 lines
Quoted 6 lines
Next time you have to send a patch, could you please:
- use 'git send-email': that will properly format the patch for sending via email, especially, it does not attach the patch, but in-lines it in the body of the mail, which makes it easier to review
- when you send a second (or third...) version of a patch, append a 'v2' (or v3, and so on...) to the subject, like: [PATCH v2] Bla blabla...Sorry for the inconveniences, I will do my best with the revised version of the patch.
No problem, I was just giving some hints! :-)
Please do not get it as if I am ignoring your hints, Yann, but I still need some time for "git send-email", because of certificate-based authorization. So, for this mail I still use gnus...
But I hope I got the Subject right and will do my best to get the patch attached inline.
Quoted 21 lines
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
Do you wish to save your new configuration ? <ESC><ESC> to continue.
Did you try to replace all the spaces there with newlines, and see what happens? Hint: it's ugly, but not because of the newlines: the window is not resized accordingly to the new number of lines, so better fix that before, too.
Quoted 8 lines
Yes, I noticed that when I put just two newlines into that text. In general it happens for any text (also without newlines) that does not fit the window height.
I solved the problem by adjusting the third parameter in the call to
dialog_yesno()and thought that that is the correct way to handle the window-height-problem if the text does not fit, becauseprint_autowrap()currently does not care at all if the given text breaks the window.You are right: it is a non-issue so far; all rendered text currently fit in their respective windows. If those texts get updated, it will be the responsibility to the author to adjust the windows height.
Quoted 8 lines
Note: the english texts do fit, but I did not check that all translations do. But are there anyone really using the localised texts at all?
I'll get your patch as-is in my tree. Depending on Michal, maybe we can queue it in for 3.10. Not too sure, though: the merge window is on the verge of closing, and I'd prefer changes linger for a while in Michal' then next's trees before getting mainlined.
I found another issue in my patch and that is a missing initialization of the variable "newline_separator", so you probably prefer this third version. I did not notice problems with this issue, but I could well imagine, others will.
Quoted 7 lines
Quoted 3 lines
But I agree, I would be nice if
print_autowrap()could handle such cases and I will see if I find a way to do that. I am unsure if that should go into one single patch/commit, though.Not needed for now, but if you want to work on this, then please do one patch for each semantically self-contained change. Adjusting the height of the window is not tied to rendering the '\n'.
I agree and I expect it to be a more serious change to mconf.
Dirk
From dcaab3b8c6c9df84cba26fcc47c244932642f292 Mon Sep 17 00:00:00 2001
From: Dirk Gouders <[email protected]>
Date: Wed, 8 May 2013 17:06:37 +0200
Subject: [PATCH v3] When exiting menuconfig with unsaved changes, a dialog like
the following is shown
Do you wish to save your new configuration ? <ESC><ESC> to continue.
The author of the dialog text specified a newline and probably expected it to be processed, so let print_autowrap() handle newlines propperly.
--- scripts/kconfig/lxdialog/util.c | 31 +++++++++++++++++-------------- scripts/kconfig/mconf.c | 4 ++-- 2 files changed, 19 insertions(+), 16 deletions(-)
scripts/kconfig/lxdialog/util.c +17 -14
@@ -371,27 +371,19 @@ void print_title(WINDOW *dialog, const char *title, int width)/* * Print a string of text in a window, automatically wrap around to the * next line if the string is too long to fit on one line. Newline -* characters '\n' are replaced by spaces. We start on a new line +* characters '\n' are propperly processed. We start on a new line * if there is no room for at least 4 nonblanks following a double-space. */ void print_autowrap (WINDOW * win,const char *prompt,int width,int y,int x) {int newl, cur_x, cur_y; -int i, prompt_len, room, wlen; -char tempstr[MAX_LEN +1 ], *word, *sp, *sp2; +int prompt_len, room, wlen; +char tempstr[MAX_LEN +1 ], *word, *sp, *sp2, *newline_separator =0 ;strcpy (tempstr, prompt); prompt_len =strlen (tempstr); -/* -* Remove newlines -*/ -for (i =0 ; i < prompt_len; i++) { -if (tempstr[i] =='\n' ) - tempstr[i] =' ' ; - } -if (prompt_len <= width - x *2 ) {/* If prompt is short */ wmove (win, y, (width - prompt_len) /2 );waddstr (win, tempstr);
@@ -401,7 +393,10 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x) newl =1 ; word = tempstr;while (word && *word) { - sp =strchr (word,' ' ); + sp =strpbrk (word,"\n " ); +if (sp && *sp =='\n' ) + newline_separator = sp; +if (sp) *sp++ =0 ;
@@ -413,7 +408,7 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)if (wlen > room || (newl && wlen <4 && sp && wlen +1 +strlen (sp) > room - && (!(sp2 =strchr (sp,' ' )) + && (!(sp2 =strpbrk (sp,"\n " )) || wlen +1 + (sp2 - sp) > room))) { cur_y++; cur_x = x;
@@ -421,7 +416,15 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)wmove (win, cur_y, cur_x);waddstr (win, word);getyx (win, cur_y, cur_x); - cur_x++; + +/* Move to the next line if the word separator was a newline */ +if (newline_separator) { + cur_y++; + cur_x = x; + newline_separator =0 ; + }else + cur_x++; +if (sp && *sp ==' ' ) { cur_x++;/* double space */ while (*++sp ==' ' ) ;
scripts/kconfig/mconf.c +2 -2
@@ -956,8 +956,8 @@ static int handle_exit(void)dialog_clear ();if (conf_get_changed ()) res =dialog_yesno (NULL , -_ ("Do you wish to save your new configuration ?\n" -"<ESC><ESC> to continue." ), +_ ("Do you wish to save your new configuration?\n" +"(Press <ESC><ESC> to continue kernel configuration.)" ),6 ,60 );else res =-1 ;
Signature · 1 line
1.8.2.1
Re: [PATCH v3] let lxdialog/util.c:print_autowrap() handle newline characters
Dirk, All,
Quoted 6 lines · Dirk Gouders
From
dcaab3b8c6c9df84cba26fcc47c244932642f292Mon Sep 17 00:00:00 2001
From: Dirk Gouders <[email protected]>
Date: Wed, 8 May 2013 17:06:37 +0200
Subject: [PATCH v3] When exiting menuconfig with unsaved changes, a dialog like
the following is shown
Quoted 7 lines
Do you wish to save your new configuration ? <ESC><ESC> to continue.
The author of the dialog text specified a newline and probably expected it to be processed, so let
print_autowrap()handle newlines propperly.Signed-off-by Dirk Gouders <[email protected]>
I've taken this in my tree:
git://gitorious.org/linux-kconfig/linux-kconfig.git yem-kconfig-for-next
I had to edit the commit log, as 'git am' pulled the complete mail, and I further edited it a little teeny bit:
(The repository is unbrowseable on gitorious, but you can see the commit
here: https://www.gitorious.org/linux-kconfig/linux-kconfig/commit/87d46f0 )
Regards,
Yann E. MORIN.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'