Re: [Nano-devel] [PATCH] Some improvements

[Benno Schulenberg]

Hello Liu,

Op 06-12-17 om 10:41 schreef Liu Hao:
> I am maintaining the Windows port [1] of GNU nano.
>
> [1] https://github.com/lhmouse/nano-win

Nice.

> Here I would like to share some patches with you. These patches address a few 
> issues about the editor itself. I do not have the FSF copyright assignment 
> paper for GNU nano so please evaluate, edit or apply them at your disposal.

No copyright assignment is required.  Just make sure to follow README.GIT
and add a "Signed-off-by:" line to your commit messages, to indicate that
the code is yours and that you submit under the same license that covers
nano.

Thanks for all the patches.  But... some of them bundle several changes
together.  It's better to cut patches up into smaller ones, of which
each covers a single coherent change.

Below I will give some comments on the patches.

> # 0001-Added-highlighting-of-C-11-keywords-and-special-iden.patch
>
> This patch adds highlighting for a number of keywords and typedefs introduced 
> by C99 (_Bool, _Complex, _Imaginary, u?int(8,16,32,64)_t, etc), C11
> (_Generic, _Atomic, _Thread_local, _Static_assert, etc), C++11 (constexpr,
> decltype, etc), as well as two identifiers with special meaning in C++11
> (override and final).

I myself make no use of these features.  Anyone any objections against
adding them?

> Highlighting of angle brackets blots C++ template parameters, so it is 
> removed.

These are meant to highlight the names of include files.  But since you
change the coloring of these directives below...

> Highlighting of preprocessor directives are moved after everything but 
> comments, so an entire directive is always colored, as preprocessor
> directives do not follow the other syntactical rules.

Hmmm...  It is quite a change, but I can't say that I dislike it.  In fact,
I like it.  Anyone against making this change?

Please provide the last two changes above as a single patch.

> # 0002-Added-highlighting-of-trailing-whitespace-in-the-def.patch
>
> This patch enables highlighting of trailing whitespaces in the default 
> scheme.

No; will not be applied.  Some people already freak out when typing in
a C file and there is constantly a flash of green when typing a space.

> # 0003-Fix-up-c.nanorc-a-little.patch
>
> This patches reverts the old font color for (FIXME|TODO|XXX). It used to be 
> red,

??  The highlighting of FIXME et al was added three years ago, and the
color was never changed, it has always been just a yellow background.

> but is omitted as defaulted now. On quite a few terminals it is grey on yellow
> thus much harder to read.

For me, yellow is a kind of brown, and red is another kind of brown,
which means that red on yellow is completely unreadable.  True, grey
on yellow is not very good, but it's better than black on yellow, and
far far better than red on yellow.  When using a dark background, using
brightwhite on yellow makes the FIXME stand out prominently, so I would
accept a patch that makes that change.  (Separate signed-off patch.)

> Highlighting of labels in C blots C++ (private|protected|public):, so it is 
> removed.

Hm...  Maybe C and C++ should have separate syntaxes?

> # 0004-When-untabifying-lines-strip-all-leading-spaces-if-t.patch
>
> When unindenting lines, nano refuses to do anything if at least one line 
> cannot be unindented. This troublesome when adjusting code that was
> misaligned (for example, when it was copy-n-pasted over an SSH terminal with
> autoindent turned on). This patch makes nano unindent those lines
> unconditionally. If there are fewer spaces than a TAB in the front of a line,
> all of them are deleted. So if you press M-{ enough times, all lines selected
> will have no leading spaces.

Currently, nano tries to prevent the user from destroying relative
indentations.  But I must say that this kind of gets in the way: it
doesn't let the user quickly equalize uneven indentations.  If the
user unintentionally destroys some relative indentation, an undo
will nowadays quickly restore it.  So I will accept this patch --
if you write it in a way that doesn't make unnecessary changes.
(Why replace while (TRUE) with for (;;)?)

Also, with your patch applied, and something is indented say three
steps, and you type M-{ ten times, you will have to type M-U ten
times to undo the unindents again (the first seven undo-s nothing
will happen).  When an M-{ has zero effect, it should not add an
undo item.

> # 0005-Reload-last-search-string-if-any.patch
>
> With the search history option turned on, if a new file is opened and M-W is 
> pressed, nano tries to reload the last string searched as the search string. 
> This is not the case when you press ^W or ^\ to open the interactive search
> or replace input box, where it will be empty and pressing Enter will cancel
> the operation. This patch makes nano reload the last search string when asked
> to open the search input box, following the behavior when M-W is pressed.

I don't like it.  When I start nano and type ^W, I don't want to see
the string that I last searched for on the prompt bar.  If I want that
search string again (to edit it), it is one <Up> away.  If I don't want
to edit it, I can just use M-W.  So I don't see the advantage of putting
up the last search string also for the first ^W.  (If it were up to me,
it would be removed from all invocations of ^W.)

Benno