GNU bug report logs - #28033
[PATCH] Add new face 'header-line-highlight'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 28033 in the body.
You can then email your comments to 28033 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Add new face 'header-line-highlight'
Date: Wed, 09 Aug 2017 17:29:19 -0600

[Message part 1 (text/plain, inline)]

Some header-line configurations don't interact nicely with the
'highlight' face, particularly when they use the :box attribute.

[0001-Add-new-face-header-line-highlight.patch (text/x-diff, attachment)]

Message #8 received at 28033 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex <agrambot <at> gmail.com>
Cc: 28033 <at> debbugs.gnu.org
Subject: Re: bug#28033: [PATCH] Add new face 'header-line-highlight'
Date: Thu, 10 Aug 2017 07:13:23 +0300

> From: Alex <agrambot <at> gmail.com>
> Date: Wed, 09 Aug 2017 17:29:19 -0600
> 
> Some header-line configurations don't interact nicely with the
> 'highlight' face, particularly when they use the :box attribute.

Can you elaborate about the problem and why introducing a new face is
the solution for it?

Thanks.

Message #11 received at 28033 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28033 <at> debbugs.gnu.org
Subject: Re: bug#28033: [PATCH] Add new face 'header-line-highlight'
Date: Wed, 09 Aug 2017 23:36:19 -0600

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Alex <agrambot <at> gmail.com>
>> Date: Wed, 09 Aug 2017 17:29:19 -0600
>> 
>> Some header-line configurations don't interact nicely with the
>> 'highlight' face, particularly when they use the :box attribute.
>
> Can you elaborate about the problem and why introducing a new face is
> the solution for it?
>
> Thanks.

The problem is that the highlight face, which is used for highlighting
links and other mouse-sensitive buffer text frequently, may not always
look nice with the header-line face. I've attached a picture
demonstrating one possible issue. In it, the header-line face has a :box
attribute with line-width 4, but the highlight face does not. This
results in the characters at the ends being clipped.

A new face allows for users to customize it to match the customization
of the header-line face while not affecting all other uses of the
highlight face.

I believe the use case of this face is very similar to
'mode-line-highlight', which was added quite a while ago.

[Screenshot_2017-08-09_23-20-30.png (image/png, attachment)]

Message #14 received at 28033 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex <agrambot <at> gmail.com>
Cc: 28033 <at> debbugs.gnu.org
Subject: Re: bug#28033: [PATCH] Add new face 'header-line-highlight'
Date: Fri, 11 Aug 2017 19:15:30 +0300

> From: Alex <agrambot <at> gmail.com>
> Cc: 28033 <at> debbugs.gnu.org
> Date: Wed, 09 Aug 2017 23:36:19 -0600
> 
> The problem is that the highlight face, which is used for highlighting
> links and other mouse-sensitive buffer text frequently, may not always
> look nice with the header-line face. I've attached a picture
> demonstrating one possible issue. In it, the header-line face has a :box
> attribute with line-width 4, but the highlight face does not. This
> results in the characters at the ends being clipped.
> 
> A new face allows for users to customize it to match the customization
> of the header-line face while not affecting all other uses of the
> highlight face.
> 
> I believe the use case of this face is very similar to
> 'mode-line-highlight', which was added quite a while ago.

Thanks, this makes sense.  But please add some of this rationale to
the documentation.

Also, it is preferable to have the first line of a NEWS item be a full
sentence, if possible.  In this case, I would just say

  ** New face 'header-line-highlight'.

and then follow that by the details.

Message #17 received at 28033 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28033 <at> debbugs.gnu.org
Subject: Re: bug#28033: [PATCH] Add new face 'header-line-highlight'
Date: Fri, 11 Aug 2017 16:10:02 -0600

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

> Thanks, this makes sense.  But please add some of this rationale to
> the documentation.

I'm not sure exactly what you're looking for, but I added a brief
explanation to the doc.

> Also, it is preferable to have the first line of a NEWS item be a full
> sentence, if possible.  In this case, I would just say
>
>   ** New face 'header-line-highlight'.
>
> and then follow that by the details.

Sure. It seems that a lot of nearby entries don't follow that style,
though...

[0001-Add-new-face-header-line-highlight.patch (text/x-diff, attachment)]

Message #22 received at 28033-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex <agrambot <at> gmail.com>
Cc: 28033-done <at> debbugs.gnu.org
Subject: Re: bug#28033: [PATCH] Add new face 'header-line-highlight'
Date: Sat, 12 Aug 2017 10:20:03 +0300

> From: Alex <agrambot <at> gmail.com>
> Cc: 28033 <at> debbugs.gnu.org
> Date: Fri, 11 Aug 2017 16:10:02 -0600
> 
> > Thanks, this makes sense.  But please add some of this rationale to
> > the documentation.
> 
> I'm not sure exactly what you're looking for, but I added a brief
> explanation to the doc.

That's what I was looking for (although I made minor wording changes
in the actual commit).

> > Also, it is preferable to have the first line of a NEWS item be a full
> > sentence, if possible.  In this case, I would just say
> >
> >   ** New face 'header-line-highlight'.
> >
> > and then follow that by the details.
> 
> Sure. It seems that a lot of nearby entries don't follow that style,
> though...

Yes, something to work on.  Patches welcome.

I've pushed the changes.  A couple of minor comments for the future:

 . Please mention the bug number in the log message.
 . The order of the references to various parts of the changes in the
   log message should assume the reading order of top to bottom, so
   the log message might need some minor reordering.  In this case,
   your original order:

> * doc/emacs/display.texi (Standard Faces):
> * etc/NEWS: Document the face.
> * lisp/emacs-lisp/tabulated-list.el (tabulated-list-init-header):
> * lisp/info.el (Info-fontify-node): Use the face.
> * lisp/faces.el: Define the face.

   refers to "the face" before it was defined.  I've reordered it to
   put the reference to the lisp/faces.el change before all the rest.

Thanks again for working on this.

Message #25 received at 28033-done <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28033-done <at> debbugs.gnu.org
Subject: Re: bug#28033: [PATCH] Add new face 'header-line-highlight'
Date: Sat, 12 Aug 2017 21:05:42 -0600

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>  . The order of the references to various parts of the changes in the
>    log message should assume the reading order of top to bottom, so
>    the log message might need some minor reordering.  In this case,
>    your original order:

I was going for a mostly alphabetical ordering. I take it that doesn't
matter?

>> * doc/emacs/display.texi (Standard Faces):
>> * etc/NEWS: Document the face.
>> * lisp/emacs-lisp/tabulated-list.el (tabulated-list-init-header):
>> * lisp/info.el (Info-fontify-node): Use the face.
>> * lisp/faces.el: Define the face.
>
>    refers to "the face" before it was defined.  I've reordered it to
>    put the reference to the lisp/faces.el change before all the rest.

I don't really understand this part (though I don't mind following it).
Only the commit summary line references the face by name, and the
summary is already at the top. Why does it matter that, in the actual
program execution, the face has to be defined first?

P.S. I happened upon two more places to add this face to. Would you
please push this as well?

Thanks.

[0001-Use-header-line-highlight-in-proced-and-erc.patch (text/x-diff, attachment)]

Message #28 received at 28033 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex <agrambot <at> gmail.com>
Cc: 28033 <at> debbugs.gnu.org
Subject: Re: bug#28033: [PATCH] Add new face 'header-line-highlight'
Date: Sun, 13 Aug 2017 17:27:04 +0300

> From: Alex <agrambot <at> gmail.com>
> Cc: 28033-done <at> debbugs.gnu.org
> Date: Sat, 12 Aug 2017 21:05:42 -0600
> 
> >  . The order of the references to various parts of the changes in the
> >    log message should assume the reading order of top to bottom, so
> >    the log message might need some minor reordering.  In this case,
> >    your original order:
> 
> I was going for a mostly alphabetical ordering. I take it that doesn't
> matter?
> 
> >> * doc/emacs/display.texi (Standard Faces):
> >> * etc/NEWS: Document the face.
> >> * lisp/emacs-lisp/tabulated-list.el (tabulated-list-init-header):
> >> * lisp/info.el (Info-fontify-node): Use the face.
> >> * lisp/faces.el: Define the face.
> >
> >    refers to "the face" before it was defined.  I've reordered it to
> >    put the reference to the lisp/faces.el change before all the rest.
> 
> I don't really understand this part (though I don't mind following it).
> Only the commit summary line references the face by name, and the
> summary is already at the top. Why does it matter that, in the actual
> program execution, the face has to be defined first?

Well, the logical order is: first you introduce the face, then you
use it, then you document it.  So it'd be nice to have the log message
read this way, top to bottom.  In your case, it was in the reverse
order, probably because "C-x 4 a" puts the entries in LIFO order.

Admittedly, this is a very minor aesthetic issue.

> P.S. I happened upon two more places to add this face to. Would you
> please push this as well?

Will do, thanks.

Message #31 received at 28033 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: agrambot <at> gmail.com
Cc: 28033 <at> debbugs.gnu.org
Subject: Re: bug#28033: [PATCH] Add new face 'header-line-highlight'
Date: Sun, 13 Aug 2017 17:59:08 +0300

> Date: Sun, 13 Aug 2017 17:27:04 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: 28033 <at> debbugs.gnu.org
> 
> > P.S. I happened upon two more places to add this face to. Would you
> > please push this as well?
> 
> Will do, thanks.

Done.

Once again, please always mention the bug number on the log message.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.