Re: [PATCH] state flag related modifications - TAB key status

[ObeliX]

On 24.12.20 14:44, Benno Schulenberg wrote:
> So... a space should mean <Tab> does what it normally does: it enters
> a \t.

OK

> When Alt+O is toggled... well I think it should show an "O",
> because that is what the other toggles (Alt+I, Alt+L, and Alt+S) do.

hmm, I think the visual representation of a flag should be associated to
the function itself, not to the shortkey used to toggle or change it.
"O" would be counterintuitive for a TAB-setting. if you are unfamiliar
with nano you don't know that M-O is the default shortkey to toggle it.
and even if you are a nano profi, chance is, you set your own shortkey
for it. (I'm by no means a profi, but I use the toggle alot and thought
Alt-O is somehow awkward and changed it quite early to M-Space (which
requires to press Shift-M on my system for some reason).

how about to use the center-dot "·", that is generally used by editors
(including nano) to represent space characters in show-whitespaces mode?

if that is not to your liking, I support Peters suggestion of using a
lower-case "s" (even that 'softwrap' already uses the upper-case "S").

> When 'tabgives' is active, it could show a "t" when the first character
> in the tabgives string is a tab, an "s" when it is a space, and an "x"
> otherwise.  But I'm open to better suggestions.

the idea for the TAB-flag was to indicate to the user what he gets when
hitting the TAB-key, not to precisely disclose the configuration. that's
way my implementation sets the same flag in default TAB-key='\t' mode,
as well when 'tabgives' is active and set to a SINGLE '\t'. if the tab
character comes from the TAB-key itself or from the tabgives string,
does not matter to the user. but when 'tabgives' will result in
something different (including one or more '\t' characters together with
other chars, the flag "t" indicates, that some tabgives string gets
inserted.

only the special case were the tabgives string is set to nothing, got an
additional flag symbol, so the user is able to see, that it's not a
broken TAB-key on his keyboard, why nothing happens when hitting that key.

in addition it would be possible, to also show the 'tabs-to-spaces'
symbol, when 'tabgives' is active and all charaters are set to " ".
was only to keep it simple, that I decided not to check for that special
case and indicate it just as "tabgives mode is active".

BTW: if it is preferred that the state flags use upper case letters,
maybe instead "t" for 'tabgives', "Y" could be used - because tabgives
is a sYntax configuration setting only.

> The order of IMLRS was chosen on purpose.
> [...]
> so they are separated by two flags that normally are on only for a
> short time

thanx for the info. as I didn't know this background story, the natural
way was to simply append the new flag to the existing ones.

> To conclude, some small remarks about the code in the patch:
>
> > -       if (ISSET(STATEFLAGS) && (flag == AUTOINDENT ||
> > -                                               flag == BREAK_LONG_LINES || 
> > flag == SOFTWRAP))
> > +       if (ISSET(STATEFLAGS) &&
> > +           (flag == AUTOINDENT || flag == TABS_TO_SPACES ||
> > +            flag == BREAK_LONG_LINES || flag == SOFTWRAP))
>
> Do not change the whitespace in any line if you are not changing anything
> else in the line.

not sure if you refer to the hunk above. but if so, then I actually did
change the line and not only the whitespaces. by adding the additional
condition to the if statement, it made sense to turn the two-line if
condition into a three-line one. I indented the second AND-block in a
(in my opinion) meaningful way to align with the first line of the if
and placed the new third line to match that.

I did not use the indentation of the original second line for the new,
additional one, as the line does not align to anything from the first
one. not using 8 spaces (like in this mail or displaying it using less),
nor with nano's 4 space tab width. it seemed to be arbitrary and
possibly a remnant from older edits.

if there is a rule, I could not deduct it. but I'm happy to hear about
it, for future occasions.

> Also, you unnecessarily wrap the first line -- just add
> the extra condition at its end, then it's much clearer what gets changed.

just appending the additional condition would have made the second line
quite long. thus it seemed a good idea to introduce a third line for the
if statement anyway. and with that third line, it made sense to have the
first AND-condition on the first line and place the second AND-block of
OR'ed conditions in the second and third line. improves readability IMHO.

> > +       const char* cT="-";
> > +       if (openfile->syntax && openfile->syntax->tab) {      // tabgives mode
> > +               if (openfile->syntax->tab[0]!='\0') {
> > +                       if (strlen(openfile->syntax->tab)==1 &&
> > +                           openfile->syntax->tab[0]=='\t') cT="T";
> > +                       else
> > +                               cT="t";
> > +               }                                     // else tab key is 
> > disabled
> > +       } else                                                // non tabgives
> > +    cT = ISSET(TABS_TO_SPACES) ? " " : "T";
> > +       waddstr(window, cT);
>
> This is ugly.  The tab flag should be computed at the start of the
> function, and then simply have six 'waddstr' statements in a row.

yes, I agree. this was a bit because of me copying code from my locally
used version, into the one to create the patch. in my local version I
create the entire state string upfront and then add it in a single
waddstr() call.

> > -                       wmove(topwin, 0, COLS + 2 - statelen);
> > +                       wmove(topwin, 0, COLS + 1 - statelen);
>
> This is not right.  The placeholder for the stateflags should get an
> extra "x".

when I mentioned in an earlier post, that I don't understand why there
is a 7 character long placeholder for the 5 state flags, you told me not
to fiddle with that code. because of that, I did not add a "x" to it and
took care of the flag alignment by changing the 'COLS + 2' into 'COLS +
1'. as far as I can see, the +2 sole purpose is to adjust for the
mismatch between placeholder size and real state flag length. and as
this difference is only 1 with the additional flag, it was a valid
solution.


when you have decided all points (especially regarding the char to use
for the 'tabs-to-spaces' mode flag) or if you want to add the TAB-flag
at all, let me know and I will adjust everything for a new patch. or you
simply use my patch as suggestion and change everything directly to your
liking.


best regards
Obel