GNU bug report logs -
#62994
Support styled underlines on TTY frames
Previous Next
Full log
Message #142 received at 62994 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> 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
This bug report was last modified 1 year and 50 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.