Re: [Nano-devel] Comment/Uncomment feature patch

[Benno Schulenberg]

On Tue, Apr 26, 2016, at 04:30, Mike Scalora wrote:
> OK, this new patch only tries to detect "//" "#" and "<!--|-->" style
> comments.
> The xml/html are important because I beleive nano is used a lot to shh into
> servers and tweak both HTML and XML. (<90 bytes) By explicitly detecting
> "#" the default syntax comment setting remains useful for settings at a cost
> of only a few 100 bytes.

I don't care much about the resultant number of bytes, more
about the number of lines of source text.

> A while back I tried to setup a "formatter" and I couldn't figure out how
> to invoke it. When I read the following part of "help" I assumed the 2nd
> and 3rd lines are unbound features,

That is correct.  The features are not bindable with "bind", though;
they are bound through the syntax files.

> not a continuation of the first line.

They are not.

> Perhaps indenting and using a lowercase "i" would make the 2nd & 3rd lines
> more understandable.
> 
> ^T    (F12)     Invoke the spell checker, if available
>                 Invoke the linter, if available
>                 Invoke formatter, if available
> 
> The nanorc man page has some hints but if you are looking for how to bind
> or invoke the "formatter" it doesn't help that much. I think something like
> this would have helped me "get it".
> 
> ^T    (F12)     Invoke the spell checker, if available,
>                     otherwise invoke the linter, if available,
>                     otherwise invoke formatter, if available

It's the other way around.  If a syntax defines a formatter, ^T invokes it;
otherwise, if the syntax defines a linter, ^T invokes it; otherwise, ^T
invokes a speller (the one defined in your nanorc, or otherwise a default
one).  Look at ^G again when editing a Python file, for example; one that 
uses a syntax that defines a linter.

> I added a undo stack dumper that runs when you invoke the stats command
> (only in debug builds of course). How do you feel about leaving that in the
> source?

Oof...  Well, we have other things too for debugging purposes only,
so I could hardly protest.  But at the moment such undo-stack dumping
wouldn't be needed: undo is working flawlessly, as far as I can tell.

> I could just apply it from a patch as needed but it might be useful
> to have around. Is there a better way/place to attach invokable logging?

The place seems good.  But it's not called the stats command, it's the
position command, or "current cursor position" command.


Some comments about details of the patch.  During compilation I got
one warning and one error:

text.c: In function ‘get_comment_sequence’:
text.c:496: warning: unused variable ‘rc’
text.c: In function ‘undo_comment_action’:
text.c:670: error: ‘for’ loop initial declarations are only allowed in C99 mode

After fixing the latter (and renaming edit_refresh_needed, which has
changed in git), things compiled fine.

-    const char *nano_indent_msg = N_("Indent the current line");
-    const char *nano_unindent_msg = N_("Unindent the current line");
+    const char *nano_indent_msg = N_("Indent the current line or marked 
lines");
+    const char *nano_unindent_msg = N_("Unindent the current line or marked 
lines");

Please resist changing lines that do not /need/ to be changed to
get the functionality you want.  The above would be a separate
patch.

     size_t mark_begin_x;
-       /* Another shadow variable. */
+       /* Comment specifc stuff. */
+    undo_group *grouping;

It may be strange, but these indented comments comment on the
variable declaration on the /preceding/ line.

+/* Name of the syntax file type used by default. */
+#define DEFAULT_SYNTAX "default"

This doesn't seem to add anything; the name can't be changed,
it will always be "default".

+    else if (*ptr=='"') {
+       *storage = mallocstrcpy(NULL, ++ptr);
+       /* allow escaped double quotes */
[...]

I don't get what this part is doing.

-    if (opensyntax && lastcolor == NULL)
-       rcfile_error(N_("Syntax \"%s\" has no color commands"),
+    // TODO: Is this a unnecessary restriction? Isn't there value in using 
just non-color functionality like tabsize?

Syntaxes cannot define tabsize or anything else, just colors.  (And,
yes, also header and magic and linter and formatter, but those were
all afterthoughts -- syntax files were meant to just contain regexes
that colored things.)

+    // TODO: How to handle the color only, comment only configs?

Color only is fine; it is the standard.  But comment only...  if you take
the trouble to make a syntax file to define the comment sequence,
you might as well define a regex to give that comment some color.
So I would leave that original test and message in place.  It does no
harm.

+const char* get_comment_sequence() {
+
+    static const char *fileext_map[] = {
[...]

The line lengths after that are much too large; keep them within 80
characters, so that it is clear how much freaking data it includes.

+    /* fisrt check if the syntax data tells us the comment syntax */
+    if (openfile->syntax && openfile->syntax->comment && 
strcmp(DEFAULT_SYNTAX, openfile->syntax->name)!=0) {
+       statusbar(_("File type based on .nanorc data: %s"), 
openfile->syntax->name);

This statusbar printing, shouldn't that be debug only?

+    /* Mark the file as modified. */
+    if (modified)
+       set_modified();
+
+    /* Update the screen. */
+    refresh_needed = TRUE;

Probably a refresh is only needed when modified is true?

+       for (f = fsfromline(group->top_line);
+               f->lineno <= group->bottom_line; f = f->next) {
+           comment_line(undoing ^ add_comment, f, u->strdata);
+       }

You're not actually undoing anything here, you're just doing the
opposite of what was done.  Which will work, but it is abusing the
undo struct.  The u->strdata doesn't contain any line data, as in
all the other undo items, so you will need a comment there.

+    case COMMENT:
+    case UNCOMMENT:
+       undo_comment_action(u, TRUE, u->type==COMMENT);
+       undidmsg = u->type==COMMENT ? _("comment") : _("uncomment");
+       statusbar(u->type==COMMENT ? _("Comment was undone") : _("Uncomment was 
undone"));
+       break;

That statusbar call is unneeded; that is what undidmsg is for.

+       undo_group *new = (undo_group *)nmalloc(sizeof(undo_group));
+       new->next = u->grouping;
+       u->grouping = new;
+       new->top_line = lineno;
+       new->bottom_line = lineno;

Why does this grouping thing have a next element?  It is never used.

-fprintf(stderr, "  >> Updating... action = %d, openfile->last_action = %d, 
openfile->current->lineno = %ld",
+    fprintf(stderr, "  >> Updating... action = %d, openfile->last_action = %d, 
openfile->current->lineno = %ld",

Again, don't change lines that don't need to be changed.


Now, running the patch...

Run 'src/nano src/winio.c', then type: M-3 PageDown M-3.
Then type: M-U M-U.

Huh?  The first thing is undone okay.  The second is undone too,
but you can't see it because it happens offscreen.  But, whenever
something is undone, the screen should center on the line that is
being changed (if it is offscreen).

I have not run further tests.  I still feel that this is /far/ too much
code for such a small and niche feature.

Benno

-- 
http://www.fastmail.com - The professional email service