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

[Mike Scalora]

On Tue, Apr 26, 2016 at 5:55 AM, Benno Schulenberg <address@hidden> wrote:

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 would never interpret the existing help text that way. I would have to suggest:

^T    (F12)     Invoke the formatter, if available,

                    otherwise invoke the linter, if available,
                    otherwise invoke spell checker, if available

 

> 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 was planning on adding undo support for indent/unindent soon/next.

> 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.

I'll take care of that. Moving the declaration to the top of the function fixed the no-debug issue.

 

-    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.

I changed this so that users wouldn't get the wrong idea after reading the comment description. I can move it to 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.

That IS strange. I'll put it back. I should have figured it out from the field names but the cognitive dissonance was too great I think.

 

+/* 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.

It supports quoted strings with escaped quotes (or other chars such as backslash) like:

    comment ".?\""

or are you asking about the mallocstrcpy(NULL, ...)? which is my attempt to find a strndup() replacement with nmalloc(). Is there something better I missed? I could use nmalloc(), strlen() and strcpy() or add nstrdup() to util.c if you would prefer.

maybe I shouldn't try to reuse pick_up_name() but I thought quotes support would be appreciated by users of linter and formatter commands. I find the lack of standard tokenization in the nanorc a bit unsettling.

-    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.

ok.

 

+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.

ok.

 

+    /* 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?

I took it out, logging is enough.

 

+    /* 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?

done.

 

+       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.

I'm not sure I appreciate distinction between "undoing" and "doing the opposite" when they result in the same change in the document. The undo struct could be a union but so there are not different meanings for the same fields but I don't think it would make the impl more clean overall.

I'm happy to adjust the code if there is a way to implement that you can suggest that would be more harmonious with the existing code/design.

I thought about refactoring undo so that ADD/DEL/ENTER/JOIN are all added as a list of sub-undo structs so that continuous typing (including enter) or continuous deleting would be undoable with a single invocation like most other editors & word processors I have used. I was thinking this would be good for undo of indent and/or justify. Sometimes undo line-by-line is more useful though and I didn't want to start such a radical change. I also thought about a way to group multiple undo structs into a single 'action'.  In the end, I thought a way to specify multiple line ranges for identical changes was cleanest for both the comment & indent functions.

The u->strdata doesn't contain any line data, as in
all the other undo items, so you will need a comment there.

Well, it is the char that got added or removed which is what it is for typing and deleting. It is a bit different for bracket style comments but very similar. I'll add a comment to make it clear.

 

+    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.

Removed.

 

+       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.

It is used when uncomment modifies discontiguous lines. I was also thinking ahead about unindent and handling stray/invisible spaces and empty looking lines. Most lines might have just a tab but some are empty (lines) or have an invisible space that I would keep track of in a range of lines. I wanted a mechanism that can restore the file to the exact characters it was before but also not have tons of data for long blocks of the same changes.

----

    print "foo"

    print "bar"

----

Might have two tabs or three (or more) tabs, refusing to unindent one case would suck all the value out of the feature.

-fprintf(stderr, "  >> Updating... action = "" openfile->last_action = %d, openfile->current->lineno = %ld",
+    fprintf(stderr, "  >> Updating... action = "" 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.

The cursor is now positioned at the top line that was changed by the undo. 

 

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.

Of course I don't believe it is a small and niche feature or I wouldn't have spent time working on it. At this point in nano's life, any new feature is going to used by a subset of users. The relative size and importance of that subset can be debated but hard to estimate ahead of time with much confidence. I do find other programmers I know tend to use comment/uncomment (and indent/unindent) when it is available, it's one of the features that you show someone once and they usually start using it. The kids (young programmers my children's age) expect it.

Googling "nano comment block" and similar variations is interesting. It's hard to interpret frequency from the anecdotal evidence but I believe for every person asking there are 100s or 1000s wondering.

Benno

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

I tested quite a bit with all of the permutations of TINY and DISABLE_COMMENTthat I thought might be affected.

I also added man page changes and syntax file comment additions.

I created the patch from the latest code in git.

Let me know what other changes you would like.

-Mike

comment_7.patch
Description: Binary data