bug#28033: [PATCH] Add new face 'header-line-highlight'

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28033: [PATCH] Add new face 'header-line-highlight'

Alex-2
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 (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28033: [PATCH] Add new face 'header-line-highlight'

Eli Zaretskii
> From: Alex <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28033: [PATCH] Add new face 'header-line-highlight'

Alex-2
Eli Zaretskii <[hidden email]> writes:

>> From: Alex <[hidden email]>
>> 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 (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28033: [PATCH] Add new face 'header-line-highlight'

Eli Zaretskii
> From: Alex <[hidden email]>
> Cc: [hidden email]
> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28033: [PATCH] Add new face 'header-line-highlight'

Alex-2
Eli Zaretskii <[hidden email]> 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 (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28033: [PATCH] Add new face 'header-line-highlight'

Eli Zaretskii
> From: Alex <[hidden email]>
> Cc: [hidden email]
> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28033: [PATCH] Add new face 'header-line-highlight'

Alex-2
Eli Zaretskii <[hidden email]> 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 (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28033: [PATCH] Add new face 'header-line-highlight'

Eli Zaretskii
> From: Alex <[hidden email]>
> Cc: [hidden email]
> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28033: [PATCH] Add new face 'header-line-highlight'

Eli Zaretskii
> Date: Sun, 13 Aug 2017 17:27:04 +0300
> From: Eli Zaretskii <[hidden email]>
> Cc: [hidden email]
>
> > 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.



Loading...