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

[Mohsin Kaleem <mohkale@kisara.moe>]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks.  I think in addition to NEWS, we'd need to update the ELisp
> Reference manual, because the new underline styles are not currently
> mentioned there.

Added. Should I mention display support between GUI and TTY frames or is
it sufficient just to mention these are the options?

> Here and elsewhere in the patch, you use indentation style slightly
> different from ours, so please reindent to follow our style (which
> uses TABs and SPACEs, not just TABs).

I did just run M-x tabify over all the lines I changed but looks like
that was a mistake. I've tried just manually re-adjusting all the
changesets in the next patchset but still not fully clear if it adheres
to the conventions wanted here 😅.

>> +		p = tparam(tty->TF_set_underline_style, NULL, 0, face->underline, 0, 0, 0);
>                          ^^
> Our style is to leave a single SPACE between the name of a function
> and the opening parenthesis.  Several places in the patch don't leave
> that SPACE.

Did a quick grep for `[^ ](` and fixed all offending lines.

>
>> +  /* Styled underlines.	 Support for this is provided either by the
>                           ^^^^^^^
> Please don't use TABs inside comments, except as indentation.

Seems to have been a byproduct of `M-x tabify`. Apologies.

>> +			return false; /* Unsupported underline style */
>   			                                            ^
> Period and one more SPACE are missing there.

Added.

>
>> +	if (!(EQ (CAR_SAFE (CDR_SAFE (val)), Qline)
>> +		  || EQ (CAR_SAFE (CDR_SAFE (val)), Qdouble)
>> +		  || EQ (CAR_SAFE (CDR_SAFE (val)), Qwave)
>> +		  || EQ (CAR_SAFE (CDR_SAFE (val)), Qdotted)
>> +		  || EQ (CAR_SAFE (CDR_SAFE (val)), Qdashed)))
>> +	  {
>> +		return false; /* Face uses an unsupported underline style.	*/
>> +	  }
>
> Our style is not to use braces for single-statement blocks.

Removed.

>
>> +/* Map the specified color COLOR of face FACE on frame F to a tty
>> +   color index.  IDX is one of LFACE_FOREGROUND_INDEX,
>> +   LFACE_BACKGROUND_INDEX or LFACE_UNDERLINE_INDEX, and specifies
>> +   which color to map.  Set *DEFAULTED to true if mapping to the
>> +   default foreground/background colors. */
>                                           ^^
> One more SPACE there.

Added.

>
>> -  if (foreground_p)
>> -    face->foreground = pixel;
>> -  else
>> -    face->background = pixel;
>> +  switch (idx)
>> +	{
>> +	case LFACE_FOREGROUND_INDEX:
>> +	  face->foreground = pixel;
>> +	  break;
>> +	case LFACE_BACKGROUND_INDEX:
>> +	  face->background = pixel;
>> +	  break;
>> +	case LFACE_UNDERLINE_INDEX:
>> +	  face->underline_color = pixel;
>> +	  break;
>> +	default:
>> +	  emacs_abort ();
>
> The original code didn't call emacs_abort, but instead simply used
> PIXEL as the background color.  Why would we do something different
> now?

Earlier in the function we call eassert on the index parameter. I
might've misunderstood but I thought that check would terminate the
function at that point so this line where we handle a index value
outside the supported range should never be called. If preferred I can
fallthrough into the background statement to stay consistent?

>
>> +static void
>> +map_tty_color2 (struct frame *f, struct face *face, Lisp_Object color,
>> +		enum lface_attribute_index idx)
>> +{
>> +	bool face_colors_defaulted = false;
>> +	map_tty_color (f, face, color, idx, &face_colors_defaulted);
>>  }
>
> Is this function really justified? why not call map_tty_color?

The extra parameter face_colors_defaulted doesn't really make sense for
anything but foreground/background color calls. Using the existing one
would make the callsite for the underline color set more noisy with an
extra output param that we then just ignore. I thought this was a
slightly nicer solution from the caller side. If preferred I can remove
and go the alternate route?

-- 
Mohsin Kaleem