GNU bug report logs - #10736
Add "underwave" face attribute

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 10736 in the body.
You can then email your comments to 10736 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: Aurélien Aptel <aurelien.aptel <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Add "underwave" face attribute
Date: Mon, 6 Feb 2012 13:00:54 +0100

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

Changelog
=========

2012-02-06  Aurélien Aptel  <aurelien.aptel <at> gmail.com>

	Add underwave face attribute for X11, W32 and NextStep.


Summary of the changes
======================

The :underline attribute has now a new way to be set:

(:color color :style style)
   If `color' is a string, underline in it.
   If `color' is `foreground-color', underline with the
   foreground color of the face.

   If `style' is `wave' underline with a wave.
   If `style' is `line' underline with a line.

   If the attribute :color is omited, underline with the
   foreground color of the face.
   If the attribute :style is omited, underline with a line.

All the previous ways to set :underline behave the same.

Customize menu
--------------

The customize menu now looks like this:

 Underline: [Off/On]
   Color: [Foreground color/...]
   Style: [Line/Wave]

Implementation
--------------

-- src/dispextern.h --

Add a new enum for the underlining type.

 enum face_underline_type {
      FACE_UNDER_LINE,
      FACE_UNDER_WAVE,
 };

And a new field in struct face.

 struct face {
        ...
        enum face_underline_type underline_type;
        ...
 };

-- src/xterm.c --

Update X11 backend.

* Add x_draw_underwave().
* The wave is computed from the distance to the left margin so that
  there is no artifact when drawing 2 consecutive waves.
* The drawing is clipped in order to not overlap.

  static void
  x_draw_underwave (struct glyph_string *s)

* Add a new codepath in x_draw_glyph_string() to handle the new style.

 /* Draw underline.  */
 if (s->face->underline_p)
   {
     if (s->face->underline_type == FACE_UNDER_WAVE)
       {
          ...new code...
       }
     else if (s->face->underline_type == FACE_UNDER_LINE)
       {
           ...re-indented old code...
       }

-- src/w32term.m --
-- src/nsterm.c --

Same change made to src/xterm.c, basically.
Just replaced line drawing primitive by the system one.

* Juanma Barranquero helped on the W32 port.
* Alp Aker helped on the NextStep port.

-- src/xfaces.c --

Add List_Object for symbol `line' and `wave'.
Reuse Qforeground_color, QCstyle, QCcolor.
Change checks made to :underline value to accept CONS construct.
Handle CONS construct when updating struct face attribute.

-- lisp/faces.el --
-- doc/lispref/display.texi --

Update :underline documentation

-- lisp/cus-face.el --

Update customize menu for :underline.

(:underline
 (choice :tag "Underline"
        :help-echo "Control text underlining."
        (const :tag "Off" nil)
        (list :tag "On"
          (const :format "" :value :color)
          (choice :tag "Color" (const :tag "Foreground Color"
foreground-color) color)
              (const :format "" :value :style)
              (choice :tag "Style"
                      (const :tag "Line" line)
                      (const :tag "Wave" wave)))))

[underwave-clip-3.patch (text/x-patch, attachment)]

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

From: Lars Magne Ingebrigtsen <larsi <at> gnus.org>
To: Aurélien Aptel <aurelien.aptel <at> gmail.com>
Cc: 10736 <at> debbugs.gnu.org
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Thu, 12 Apr 2012 22:16:11 +0200

Aurélien Aptel <aurelien.aptel <at> gmail.com> writes:

> 2012-02-06  Aurélien Aptel  <aurelien.aptel <at> gmail.com>
>
> 	Add underwave face attribute for X11, W32 and NextStep.
>
> Summary of the changes
> ======================
>
> The :underline attribute has now a new way to be set:
>
> (:color color :style style)
>    If `color' is a string, underline in it.
>    If `color' is `foreground-color', underline with the
>    foreground color of the face.
>
>    If `style' is `wave' underline with a wave.
>    If `style' is `line' underline with a line.

The screen shots look really good, but I'm not familiar enough with the
C level face stuff to judge that.

Looking at the mailing list, it seems like it was agreed that this is
useful, and that this interface is correct.  Since we're in Emacs 24.2
territory now, should this go in?

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

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

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Lars Magne Ingebrigtsen <larsi <at> gnus.org>
Cc: 10736 <at> debbugs.gnu.org,
	Aurélien Aptel <aurelien.aptel <at> gmail.com>
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Thu, 12 Apr 2012 17:57:07 -0400

> Looking at the mailing list, it seems like it was agreed that this is
> useful, and that this interface is correct.  Since we're in Emacs 24.2
> territory now, should this go in?

Assuming the code is OK, yes.   I'd like to hear someone's opinion on
the code, tho (Jan, maybe?).


        Stefan

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

From: Alp Aker <alptekin.aker <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Magne Ingebrigtsen <larsi <at> gnus.org>, 10736 <at> debbugs.gnu.org,
	Aurélien Aptel <aurelien.aptel <at> gmail.com>
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Thu, 12 Apr 2012 20:49:17 -0400

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

On Thu, Apr 12, 2012 at 5:57 PM, Stefan Monnier
<monnier <at> iro.umontreal.ca> wrote:
> Assuming the code is OK, yes.   I'd like to hear someone's opinion on
> the code, tho (Jan, maybe?).

Looking afresh at the small bit I contributed for ns_draw_underwave,
it seems to me that that function does some unnecessary work with
clipping rectangles, so I'd like to offer a slightly simpler version
(revised patch file attached).

(In explanation of this change:  If I understand the NS port's drawing
routine correctly, at the time this function is called the current
clipping path has already been restricted to the glyph string's clip
rects.  Since NSRectClip can only further restrict the current
clipping path, there's no need explicitly to invoke the glyph string's
clip rects at this point.  Indeed, I suspect the NS version could get
away without worrying about clipping at all here, but I'm not as
certain of this latter point.)

[underwave-clip-4.patch (application/octet-stream, attachment)]

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

From: Aurélien Aptel <aurelien.aptel <at> gmail.com>
To: Alp Aker <alptekin.aker <at> gmail.com>
Cc: 10736 <at> debbugs.gnu.org, Lars Magne Ingebrigtsen <larsi <at> gnus.org>,
	Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Fri, 13 Apr 2012 12:05:29 +0200

On Fri, Apr 13, 2012 at 2:49 AM, Alp Aker <alptekin.aker <at> gmail.com> wrote:
> Looking afresh at the small bit I contributed for ns_draw_underwave,
> it seems to me that that function does some unnecessary work with
> clipping rectangles, so I'd like to offer a slightly simpler version
> (revised patch file attached).

I agree. Most of this is necessary on X11 because the current clipping
region cannot be retrieved. If I just override it for underwaving,
non-visible text can be underwaved. My solution was to compute the
current clipping region again via get_glyph_string_clip_rect() (which
can be empty if the string is not visible) and intersect it with the
wave clipping region. Then draw the wave and set the glyph string
clipping region again.

> (In explanation of this change:  If I understand the NS port's drawing
> routine correctly, at the time this function is called the current
> clipping path has already been restricted to the glyph string's clip
> rects.  Since NSRectClip can only further restrict the current
> clipping path, there's no need explicitly to invoke the glyph string's

This is correct.

> clip rects at this point.  Indeed, I suspect the NS version could get
> away without worrying about clipping at all here, but I'm not as
> certain of this latter point.)

The underwave is computed from the left margin (every wave starts at
x=0) so that two consecutive underwaved strings appear to be
underwaved by one wave.

At first I did not use the clipping region and computed each time the
next line to draw; but for very small lines (1-2 px) the X11 drawline
primitive doesn't work well. Specifically, unneeded pixels were drawn
at both ends of the wave therefore making the juncture between two
waves visible.

My solution was to clip the region and draw a wavelet before/after when needed.

Concerning the NS port, as I understand drawing primitives work on a
non-discrete plane where integer coordinates falls "between" pixels
resulting in two blurred pixels instead of a filled one. Since I can't
experiment on this platform I was wondering if you could try shifting
every coordinate by .5 to see if it looks better.

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

From: Jan Djärv <jan.h.d <at> swipnet.se>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: Lars Magne Ingebrigtsen <larsi <at> gnus.org>, 10736 <at> debbugs.gnu.org,
	Aurélien Aptel <aurelien.aptel <at> gmail.com>
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Fri, 13 Apr 2012 19:49:24 +0200

Hello.

12 apr 2012 kl. 23:57 skrev Stefan Monnier:

>> Looking at the mailing list, it seems like it was agreed that this is
>> useful, and that this interface is correct.  Since we're in Emacs 24.2
>> territory now, should this go in?
> 
> Assuming the code is OK, yes.   I'd like to hear someone's opinion on
> the code, tho (Jan, maybe?).
> 

Looks OK.  In general I don't like the "Get GC values, change some GC value, draw, restore changed GC value" technique. It is better to create more GC:s.  But the new code just mimics the old, so it is nothing particular for this patch.

	Jan D.

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

From: Alp Aker <alptekin.aker <at> gmail.com>
To: Aurélien Aptel <aurelien.aptel <at> gmail.com>
Cc: 10736 <at> debbugs.gnu.org, Lars Magne Ingebrigtsen <larsi <at> gnus.org>,
	Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Fri, 13 Apr 2012 19:35:28 -0400

On Fri, Apr 13, 2012 at 6:05 AM, Aurélien Aptel
<aurelien.aptel <at> gmail.com> wrote:
> Concerning the NS port, as I understand drawing primitives work on a
> non-discrete plane where integer coordinates falls "between" pixels
> resulting in two blurred pixels instead of a filled one. Since I can't
> experiment on this platform I was wondering if you could try shifting
> every coordinate by .5 to see if it looks better.

It's not a matter of the coordinate system.  The Cocoa line drawing
functions draw each line so that it's centered on its path.  If the
width of the line is an odd number of pixels, anti-aliasing can then
make it look wider than expected.

In my (limited) experience, this effect is only noticeable when
drawing straight vertical or horizontal line segments.  In the present
case, shifting by 0.5 pixels has no discernable affect on the
appearance of the underwave.

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

From: Aurélien Aptel <aurelien.aptel <at> gmail.com>
To: Alp Aker <alptekin.aker <at> gmail.com>
Cc: 10736 <at> debbugs.gnu.org, Lars Magne Ingebrigtsen <larsi <at> gnus.org>,
	Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Tue, 5 Jun 2012 21:05:50 +0200

Just getting some news.

In case it wasn't clear, the last patch (underwave-clip-4.patch) sent
by Alp Aker is correct and can be applied. Are you waiting for more
people to judge the code or Is there something else I/we can do?

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Aurélien Aptel <aurelien.aptel <at> gmail.com>
Cc: Alp Aker <alptekin.aker <at> gmail.com>,
	Lars Magne Ingebrigtsen <larsi <at> gnus.org>,
	Jan Djärv <jan.h.d <at> swipnet.se>, 10736 <at> debbugs.gnu.org
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Tue, 05 Jun 2012 18:54:41 -0400

> In case it wasn't clear, the last patch (underwave-clip-4.patch) sent
> by Alp Aker is correct and can be applied. Are you waiting for more
> people to judge the code or Is there something else I/we can do?

Alp, Jan, or someone, could you install that code?


        Stefan

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

From: Alp Aker <alptekin.aker <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Magne Ingebrigtsen <larsi <at> gnus.org>,
	Jan Djärv <jan.h.d <at> swipnet.se>, 10736 <at> debbugs.gnu.org,
	Aurélien Aptel <aurelien.aptel <at> gmail.com>
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Wed, 6 Jun 2012 15:30:00 +0300

On Wed, Jun 6, 2012 at 1:54 AM, Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:
>
> Alp, Jan, or someone, could you install that code?

I'll do so next week if it hasn't been done by then (I'm on the road
at the moment).

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

From: Alp Aker <alptekin.aker <at> gmail.com>
To: 10736 <at> debbugs.gnu.org
Cc: Aurélien Aptel <aurelien.aptel <at> gmail.com>
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Sat, 16 Jun 2012 20:40:57 -0400

On Wed, Jun 6, 2012 at 8:30 AM, Alp Aker <alptekin.aker <at> gmail.com> wrote:
> On Wed, Jun 6, 2012 at 1:54 AM, Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:
>>
>> Alp, Jan, or someone, could you install that code?
>
> I'll do so next week if it hasn't been done by then (I'm on the road
> at the moment).

Patch applied as revno 108632.

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

From: Glenn Morris <rgm <at> gnu.org>
To: Alp Aker <alptekin.aker <at> gmail.com>
Cc: 10736 <at> debbugs.gnu.org,
	Aurélien Aptel <aurelien.aptel <at> gmail.com>
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Sat, 16 Jun 2012 21:47:06 -0400

I'm not seeing a copyright assignment for this. Unless one was completed
very recently?

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

From: Alp Aker <alptekin.aker <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 10736 <at> debbugs.gnu.org,
	Aurélien Aptel <aurelien.aptel <at> gmail.com>
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Sun, 17 Jun 2012 12:16:11 -0400

 On Sat, Jun 16, 2012 at 9:47 PM, Glenn Morris <rgm <at> gnu.org> wrote:
>
> I'm not seeing a copyright assignment for this. Unless one was completed
> very recently?

I didn't check, to be honest.  (Since the issue hadn't been raised in
either of the relevant threads and Stefan gave the go ahead to install
the code, I'd assumed the  copyright was in order.)

Aurélien, have you completed an assignment of copyright to the FSF?

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

From: Aurélien Aptel <aurelien.aptel <at> gmail.com>
To: Alp Aker <alptekin.aker <at> gmail.com>
Cc: Glenn Morris <rgm <at> gnu.org>, 10736 <at> debbugs.gnu.org
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Mon, 18 Jun 2012 02:55:28 +0200

On Sun, Jun 17, 2012 at 6:16 PM, Alp Aker <alptekin.aker <at> gmail.com> wrote:
> Aurélien, have you completed an assignment of copyright to the FSF?

I'm afraid I have not.
Which template should I use from this list [1]?

1: http://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright

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

From: Glenn Morris <rgm <at> gnu.org>
To: Aurélien Aptel <aurelien.aptel <at> gmail.com>
Cc: Alp Aker <alptekin.aker <at> gmail.com>, 10736 <at> debbugs.gnu.org
Subject: Re: bug#10736: Add "underwave" face attribute
Date: Mon, 18 Jun 2012 14:30:51 -0400

Aurélien Aptel wrote:

> On Sun, Jun 17, 2012 at 6:16 PM, Alp Aker <alptekin.aker <at> gmail.com> wrote:
>> Aurélien, have you completed an assignment of copyright to the FSF?
>
> I'm afraid I have not.
> Which template should I use from this list [1]?

I will send you the correct form off-list.
Thanks for the patch and for being willing to assign copyright.

Previous Next

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