GNU bug report logs - #13000
24.2.90; underwave doesn't look as good as other IDEs

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 13000 in the body.
You can then email your comments to 13000 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: Leo <sdl.web <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.2.90; underwave doesn't look as good as other IDEs
Date: Mon, 26 Nov 2012 11:21:21 +0800

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

Thanks to Aurelien Aptel for implementing this feature. I have started
using it in flymake to indicate errors. However, I have noticed that it
doesn't look as good as that of, for example, the pycharm IDE. Here is a
screenshot comparison.

[pycharm-wave.png (image/png, attachment)]

[emacs-wave.png (image/png, attachment)]

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

In the IDE the wave line is thinner and the ripple bigger. So it appears
clearly wave-like. In emacs, it looks too similar to a blurry straight
line.

Leo

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

From: Leo <sdl.web <at> gmail.com>
To: 13000 <at> debbugs.gnu.org
Cc: aurelien.aptel <at> gmail.com
Subject: Re: bug#13000: 24.2.90; underwave doesn't look as good as other IDEs
Date: Wed, 26 Dec 2012 14:31:03 +0800

The problem I am seeing seems to be specific to Mac Port that I am
using. I'll contact its author for more details.

BTW, the code and comment doesn't match. Could someone review and fix
it?

xterm.c around line 2645

/*
   Draw a wavy line under S. The wave fills wave_height pixels from y0.

                    x0         wave_length = 2
                                 --
                y0   *   *   *   *   *
                     |* * * * * * * * *
    wave_height = 3  | *   *   *   *

*/

static void
x_draw_underwave (struct glyph_string *s)
{
  int wave_height = 2, wave_length = 3;

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

From: Leo <sdl.web <at> gmail.com>
To: 13000 <at> debbugs.gnu.org
Cc: Chong Yidong <cyd <at> stupidchicken.com>, aurelien.aptel <at> gmail.com
Subject: Re: bug#13000: 24.2.90; underwave doesn't look as good as other IDEs
Date: Thu, 27 Dec 2012 16:33:19 +0800

The following patch by YAMAMOTO Mitsuharu brings the code to be in line
with the comment. I tested it with an X11 build and the wave looked
better.


=== modified file 'src/xterm.c'
*** src/xterm.c 2012-12-12 15:33:30 +0000
--- src/xterm.c 2012-12-27 01:20:52 +0000
***************
*** 2633,2646 ****
  static void
  x_draw_underwave (struct glyph_string *s)
  {
!   int wave_height = 2, wave_length = 3;
    int dx, dy, x0, y0, width, x1, y1, x2, y2, odd, xmax;
    XRectangle wave_clip, string_clip, final_clip;

    dx = wave_length;
    dy = wave_height - 1;
    x0 = s->x;
!   y0 = s->ybase + 1;
    width = s->width;
    xmax = x0 + width;

--- 2633,2646 ----
  static void
  x_draw_underwave (struct glyph_string *s)
  {
!   int wave_height = 3, wave_length = 2;
    int dx, dy, x0, y0, width, x1, y1, x2, y2, odd, xmax;
    XRectangle wave_clip, string_clip, final_clip;

    dx = wave_length;
    dy = wave_height - 1;
    x0 = s->x;
!   y0 = s->ybase - wave_height + 3;
    width = s->width;
    xmax = x0 + width;

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

From: Juri Linkov <juri <at> jurta.org>
To: Leo <sdl.web <at> gmail.com>
Cc: aurelien.aptel <at> gmail.com, 13000 <at> debbugs.gnu.org
Subject: Re: bug#13000: 24.2.90; underwave doesn't look as good as other IDEs
Date: Thu, 27 Dec 2012 21:33:18 +0200

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

> The problem I am seeing seems to be specific to Mac Port
> that I am using.

The problem occurs in X11 builds on GNU/Linux too.

With a small font the underwave line turns into a dashed line.
For example, with the following small fonts it looks like

-misc-fixed-medium-r-normal--10-100-75-75-c-60-iso8859-1

[flybad1.png (image/png, inline)]

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

-misc-fixed-medium-r-normal--14-130-75-75-c-70-iso8859-1

[flybad2.png (image/png, inline)]

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

Only with a bigger font, it looks like a wave:

-misc-fixed-medium-r-normal--18-120-100-100-c-90-iso8859-1

[flybad3.png (image/png, inline)]

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

However, the proposed patch provides much better graceful degradation,
where even with small fonts the line looks like a wave, for example:

-misc-fixed-medium-r-normal--10-100-75-75-c-60-iso8859-1

[flyok1.png (image/png, inline)]

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

-misc-fixed-medium-r-normal--14-130-75-75-c-70-iso8859-1

[flyok2.png (image/png, inline)]

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

-misc-fixed-medium-r-normal--18-120-100-100-c-90-iso8859-1

[flyok3.png (image/png, inline)]

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

I believe this confirms the patch is an improvement.

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

From: Leo <sdl.web <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#13000: 24.2.90; underwave doesn't look as good as other IDEs
Date: Fri, 28 Dec 2012 17:06:35 +0800

On 2012-12-28 03:33 +0800, Juri Linkov wrote:
> The problem occurs in X11 builds on GNU/Linux too.
>
> With a small font the underwave line turns into a dashed line.
> For example, with the following small fonts it looks like

Many thanks for the amazing and detailed tests. I hope this new feature
is done properly before users see it.

Leo

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

From: Juri Linkov <juri <at> jurta.org>
To: Leo <sdl.web <at> gmail.com>
Cc: 13000 <at> debbugs.gnu.org
Subject: Re: bug#13000: 24.2.90; underwave doesn't look as good as other IDEs
Date: Sat, 29 Dec 2012 01:54:28 +0200

> I hope this new feature is done properly before users see it.

For users to take advantage of this new feature we could modify
the faces where it would be most useful like the patch below does.

One problem is how to handle the tty case.  On a tty frame
underwave underlines and doesn't use the color attribute.
So the question is what face condition to use to filter out
displays where underwave is not supported.

The already supported condition ((supports :underline t))
doesn't take into account the underwave attribute,
and a similar condition ((supports :underline wave))
is not implemented.

Thus the patch uses the condition ((class color) (min-colors 88))
to find the displays that should use the old face definitions
without underwave:

=== modified file 'lisp/progmodes/flymake.el'
--- lisp/progmodes/flymake.el	2012-11-12 08:42:27 +0000
+++ lisp/progmodes/flymake.el	2012-12-28 23:50:57 +0000
@@ -844,12 +844,18 @@ (defun flymake-region-has-flymake-overla
     has-flymake-overlays))
 
 (defface flymake-errline
-  '((t :inherit error))
+  '((((class color) (min-colors 88))
+     :underline (:color "Red1" :style wave))
+    (t
+     :inherit error))
   "Face used for marking error lines."
   :group 'flymake)
 
 (defface flymake-warnline
-  '((t :inherit warning))
+  '((((class color) (min-colors 88))
+     :underline (:color "DarkOrange" :style wave))
+    (t
+     :inherit warning))
   "Face used for marking warning lines."
   :group 'flymake)

=== modified file 'lisp/textmodes/flyspell.el'
--- lisp/textmodes/flyspell.el	2012-09-17 05:41:04 +0000
+++ lisp/textmodes/flyspell.el	2012-12-28 23:53:51 +0000
@@ -445,11 +445,19 @@ (make-variable-buffer-local 'flyspell-da
 ;;*---------------------------------------------------------------------*/
 ;;*    Highlighting                                                     */
 ;;*---------------------------------------------------------------------*/
-(defface flyspell-incorrect '((t :underline t :inherit error))
+(defface flyspell-incorrect
+  '((((class color) (min-colors 88))
+     :underline (:color "Red1" :style wave))
+    (t
+     :underline t :inherit error))
   "Flyspell face for misspelled words."
   :group 'flyspell)
 
-(defface flyspell-duplicate '((t :underline t :inherit warning))
+(defface flyspell-duplicate
+  '((((class color) (min-colors 88))
+     :underline (:color "DarkOrange" :style wave))
+    (t
+     :underline t :inherit warning))
   "Flyspell face for words that appear twice in a row.
 See also `flyspell-duplicate-distance'."
   :group 'flyspell)

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

From: Leo <sdl.web <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#13000: 24.2.90; underwave doesn't look as good as other IDEs
Date: Sat, 29 Dec 2012 14:45:47 +0800

On 2012-12-29 07:54 +0800, Juri Linkov wrote:
> For users to take advantage of this new feature we could modify
> the faces where it would be most useful like the patch below does.

I meant the need for underwave to actually look like a wave. I don't
know why this is tagged minor though. 

I agree there are a few places underwave is best suited than anything
else (the reason I requested this feature).

Cheers,
Leo

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> jurta.org>
Cc: sdl.web <at> gmail.com, 13000 <at> debbugs.gnu.org
Subject: Re: bug#13000: 24.2.90; underwave doesn't look as good as other IDEs
Date: Sat, 29 Dec 2012 09:19:20 +0200

> From: Juri Linkov <juri <at> jurta.org>
> Date: Sat, 29 Dec 2012 01:54:28 +0200
> Cc: 13000 <at> debbugs.gnu.org
> 
> One problem is how to handle the tty case.  On a tty frame
> underwave underlines and doesn't use the color attribute.
> So the question is what face condition to use to filter out
> displays where underwave is not supported.
> 
> The already supported condition ((supports :underline t))
> doesn't take into account the underwave attribute,
> and a similar condition ((supports :underline wave))
> is not implemented.

Implementing the missing condition should be easy.

> Thus the patch uses the condition ((class color) (min-colors 88))
> to find the displays that should use the old face definitions
> without underwave:

This will do the wrong thing on 256-color xterm, I think.

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

From: Juri Linkov <juri <at> jurta.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: sdl.web <at> gmail.com, 13000 <at> debbugs.gnu.org
Subject: Re: bug#13000: 24.2.90; underwave doesn't look as good as other IDEs
Date: Sat, 29 Dec 2012 23:54:30 +0200

>> The already supported condition ((supports :underline t))
>> doesn't take into account the underwave attribute,
>> and a similar condition ((supports :underline wave))
>> is not implemented.
>
> Implementing the missing condition should be easy.

Yes, pretty easy:

=== modified file 'src/xfaces.c'
--- src/xfaces.c	2012-11-16 17:20:23 +0000
+++ src/xfaces.c	2012-12-29 21:50:36 +0000
@@ -4877,6 +4877,8 @@ tty_supports_face_attributes_p (struct frame *f,
     {
       if (STRINGP (val))
 	return 0;		/* ttys can't use colored underlines */
+      else if (EQ (CAR_SAFE (val), QCstyle) && EQ (CAR_SAFE (CDR_SAFE (val)), Qwave))
+	return 0;		/* ttys can't use wave underlines */
       else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
 	return 0;		/* same as default */
       else


Then its support could checked with ((supports :underline (:style wave)))

=== modified file 'lisp/progmodes/flymake.el'
--- lisp/progmodes/flymake.el	2012-11-12 08:42:27 +0000
+++ lisp/progmodes/flymake.el	2012-12-29 21:54:08 +0000
@@ -844,12 +844,18 @@ (defun flymake-region-has-flymake-overla
     has-flymake-overlays))
 
 (defface flymake-errline
-  '((t :inherit error))
+  '((((supports :underline (:style wave)))
+     :underline (:style wave :color "Red1"))
+    (t
+     :inherit error))
   "Face used for marking error lines."
   :group 'flymake)
 
 (defface flymake-warnline
-  '((t :inherit warning))
+  '((((supports :underline (:style wave)))
+     :underline (:style wave :color "DarkOrange"))
+    (t
+     :inherit warning))
   "Face used for marking warning lines."
   :group 'flymake)
 

=== modified file 'lisp/textmodes/flyspell.el'
--- lisp/textmodes/flyspell.el	2012-09-17 05:41:04 +0000
+++ lisp/textmodes/flyspell.el	2012-12-29 21:54:14 +0000
@@ -445,11 +445,19 @@ (make-variable-buffer-local 'flyspell-da
 ;;*---------------------------------------------------------------------*/
 ;;*    Highlighting                                                     */
 ;;*---------------------------------------------------------------------*/
-(defface flyspell-incorrect '((t :underline t :inherit error))
+(defface flyspell-incorrect
+  '((((supports :underline (:style wave)))
+     :underline (:style wave :color "Red1"))
+    (t
+     :underline t :inherit error))
   "Flyspell face for misspelled words."
   :group 'flyspell)
 
-(defface flyspell-duplicate '((t :underline t :inherit warning))
+(defface flyspell-duplicate
+  '((((supports :underline (:style wave)))
+     :underline (:style wave :color "DarkOrange"))
+    (t
+     :underline t :inherit warning))
   "Flyspell face for words that appear twice in a row.
 See also `flyspell-duplicate-distance'."
   :group 'flyspell)

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

From: Leo Liu <sdl.web <at> gmail.com>
To: Juri Linkov <juri <at> jurta.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 13000 <at> debbugs.gnu.org
Subject: Re: bug#13000: 24.2.90; underwave doesn't look as good as other IDEs
Date: Fri, 04 Jan 2013 09:47:36 +0800

On 2012-12-30 05:54 +0800, Juri Linkov wrote:
>> Implementing the missing condition should be easy.
>
> Yes, pretty easy:

Hello Juri and Eli,

These are good use cases for underwave. I wonder if they are ready to
install in trunk.

Leo

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

From: Juri Linkov <juri <at> jurta.org>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 13000-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#13000: 24.2.90; underwave doesn't look as good as other IDEs
Date: Wed, 09 Jan 2013 01:51:10 +0200

> These are good use cases for underwave. I wonder if they are ready to
> install in trunk.

Thanks for the reminder.  They are now installed in trunk.

Previous Next

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