Re: [Nano-devel] [PATCH] enable searching (^W) in a help text (^G)

[Benno Schulenberg]

Hello Rishabh,

On Sat, Nov 5, 2016, at 18:59, Rishabh Dave wrote:
> Due considerable much rewriting and reordering, the patch is not on
> top of previous one.

That is the right way.

> Specifically this patch adds the ability to
> delete the temporary file/s generated (handled in do_help()), wraps
> the help text as per the screen (handled in help_init()).

Good.

> One question
> here: why don't we allow wraps greater than 74 *even* when screen can
> accommodate it?

Personal preference.  And it has been shown that very long lines
are harder to read than shorter ones.  For this reason newspapers
use columns instead of printing text over the entire width of the
page.

> There is one problem with this patch. After applying this patch, one
> can open another help screen by doing ^G, ^W and ^G. Should this help
> option be disabled or it should allow this nested help?

It should be disabled; no nesting should be allowed, also because
that help text talks about "Replace" too, and one cannot replace
anything in a help text.

Furthermore, you should temporarily unset LINENUMBERS when
showing a help text.  And suppress the "xx lines read" message.
And don't precede the headline with "Help: ".  And strip the
headline (that goes into the titlebar) from the text itself,
as it is a useless duplication.  And disable at least the ^R and
M-J functions, and probably also the case/regex/direction toggles.

Your patch has a bug.  Run for example 'LANGUAGE=es src/nano'
and type ^W ^G.  See how the second line of the help text ends
with "la pantalla se Si ha".  Compare it with 'LANGUAGE=es nano'
and see how your version has two half paragraphs missing.
I suspect that your patch assumes that every hx[n] ends in a space.
But this cannot be guaranteed -- translators make mistakes.

Some comments about details in your patch:

+    FILE *fp;
+    char *tempfilename = safe_tempfile(&fp);

Declare variables at the start of a function or block, not in
the middle of the code, and add a comment to each of them that
explains its purpose.

+    if (tempfilename == NULL) {

Do this before initiliazing the help text; there is no point
in composing the help text when no temp file can be created.

+    char *title = charalloc(50 * sizeof(char));
+    title = fgets(title, 50, fp);

Why fifty?  Use a constant, MAX_BUF_SIZE for example.

+    titlebar(title);
+    while (TRUE) {

Use a blank line before larger blocks of code.

+    /* Add the paragraph/s to the help_text. */
+    int i, line_size;

The comment does not describe the statement.  And again:
declare variables at the start of the function or block.
And use a blank line after any declaration.

+    /* Get rid of garbage value for the sake of strlen(help_text). */
+    for (i = 0; i <= allocsize; ++i)
+       help_text[i] = '\0';

Garbage value?  How, where, what is garbage?  Probably this
is the bug that was referred to above.

The rest I haven't tried to understand -- too much code.
Can't you reuse the help_line_len() function?  Maybe, to
make things easier, you should first concatenate the three
htx-es into a single allocated string?

Benno

-- 
http://www.fastmail.com - Choose from over 50 domains or use your own