bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

[Leo]

[-- Attachment #1: Type: text/plain, Size: 240 bytes --]

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.


[-- Attachment #2: pycharm-wave.png --]
[-- Type: image/png, Size: 6656 bytes --]

[-- Attachment #3: emacs-wave.png --]
[-- Type: image/png, Size: 5556 bytes --]

[-- Attachment #4: Type: text/plain, Size: 156 bytes --]


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

bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

[Leo]

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;

bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

[Leo]

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;

bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

> 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


[-- Attachment #2: flybad1.png --]
[-- Type: image/png, Size: 456 bytes --]

[-- Attachment #3: Type: text/plain, Size: 59 bytes --]


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


[-- Attachment #4: flybad2.png --]
[-- Type: image/png, Size: 562 bytes --]

[-- Attachment #5: Type: text/plain, Size: 109 bytes --]


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

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


[-- Attachment #6: flybad3.png --]
[-- Type: image/png, Size: 660 bytes --]

[-- Attachment #7: Type: text/plain, Size: 200 bytes --]


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


[-- Attachment #8: flyok1.png --]
[-- Type: image/png, Size: 485 bytes --]

[-- Attachment #9: Type: text/plain, Size: 59 bytes --]


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


[-- Attachment #10: flyok2.png --]
[-- Type: image/png, Size: 598 bytes --]

[-- Attachment #11: Type: text/plain, Size: 61 bytes --]


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


[-- Attachment #12: flyok3.png --]
[-- Type: image/png, Size: 676 bytes --]

[-- Attachment #13: Type: text/plain, Size: 54 bytes --]


I believe this confirms the patch is an improvement.

bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

[Leo]

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

bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

[Juri Linkov]

> 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)

bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

[Leo]

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

bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Date: Sat, 29 Dec 2012 01:54:28 +0200
> Cc: 13000@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.

bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

[Juri Linkov]

>> 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)

bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

[Leo Liu]

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

bug#13000: 24.2.90; underwave doesn't look as good as other IDEs

[Juri Linkov]

> 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.