bug#62994: [PATCH v6] Add support for colored and styled underlines on tty frames

[Mohsin Kaleem <mohkale@kisara.moe>]

Eli Zaretskii <eliz@gnu.org> writes:

> The Subject line is longer than 78 characters, so our commit hooks
> didn't allow me to apply this.  Please make the Subject line shorter,
> possibly by dropping the bug number (mention the bug number somewhere
> in the rest of the log message).

Sorry, I'm not quite sure what you mean. Are you referring to the
subject line of the email message? That's 85 characters, but removing re
and the [PATCH X] part condenses it to 70 characters. The original
commit I'm making in git is only 58 characters. For reference I've gone
ahead and added a reference to the bug number in the commit message body
but I'm not sure that's enough to fix this. Would you like me to
re-submit with a custom --subject string matching the commit message in
git-send-email. I presume that would exclude the re, patch and bug
sections?

>> * src/xterm.c (x_draw_glyph_string): Updated to use renamed
>> FACE_UNDERLINE_SINGLE and FACE_UNDERLINE_WAVE face_underline_type
>> enumerations.
>
> These lines are also too long, and don't follow our conventions.  I
> suggest to use change-log-mode to format and fill those correctly.

To confirm this is what I did, I'm not sure if it's exactly what you
were recommending:

1. emacs -Q .git/COMMIT_EDITMSG
2. M-x change-log-mode
3. C-SPC to end of buffer message
4. C-M-\
5. Manually remove the leading indentation Emacs seems to have inserted
(other commit messages didn't seem to have this).

The end result doesn't seem to have changed the line length of anything,
although it did highlight one point where the entry was malformed.

> Period missing at the end of the comment.  Also, please leave two
> spaces between the end of the comment and the closing "*/", per our
> conventions.

Added.

>
>> +  /* Check supported underline styles. */
>> +  val = attrs[LFACE_UNDERLINE_INDEX];
>> +  if (!UNSPECIFIEDP (val))
>> +    if (EQ (CAR_SAFE (val), QCstyle))
>> +      if (!(EQ (CAR_SAFE (CDR_SAFE (val)), Qline)
>> +	    || EQ (CAR_SAFE (CDR_SAFE (val)), Qwave)))
>> +	return false; /* Unsupported underline style.  */
>
> Can this be written in a cleaner way, as a single 'if' with all the
> conditions combined together, instead of nesting them?
>

I think so, yes. This was added from the corresponding tty code which
had more nesting because it also supported more checks against the face
attribute.

Updated.

>> -	return false;		/* same as default */
>> +	return false;  /* same as default */
>
> Why this whitespace change?

I suspect this was a byproduct of me re-indenting comments in an earlier patch.

Removed now.

-- 
Mohsin Kaleem