bug#17115: 24.3.50; possible bug in drawing images with box on W32

bug#17115: 24.3.50; possible bug in drawing images with box on W32

[YAMAMOTO Mitsuharu]

Recently I found a bug (which is different from Bug#17114) in drawing
images with box on the Mac port, and I suspect it also happens on W32
because their code are similar (though I can't test it).  Could
someone try if it is reproducible on W32 and if so, please try the
patch at the end?

Steps to reproduce:

  1. $ emacs -Q -D &
  2. (insert-image (create-image "splash.png")
	      (propertize " " 'face '(:box (:line-width 5)))) C-j
  3. C-p C-p

Result:

  The left half of the character `t' just after the image gets erased.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

=== modified file 'src/w32term.c'
*** src/w32term.c	2014-03-26 15:57:13 +0000
--- src/w32term.c	2014-03-27 02:32:46 +0000
***************
*** 2082,2094 ****
  static void
  x_draw_image_glyph_string (struct glyph_string *s)
  {
-   int x, y;
    int box_line_hwidth = eabs (s->face->box_line_width);
    int box_line_vwidth = max (s->face->box_line_width, 0);
    int height;
    HBITMAP pixmap = 0;
  
!   height = s->height - 2 * box_line_vwidth;
  
    /* Fill background with face under the image.  Do it only if row is
       taller than image or if image has a clip mask to reduce
--- 2082,2097 ----
  static void
  x_draw_image_glyph_string (struct glyph_string *s)
  {
    int box_line_hwidth = eabs (s->face->box_line_width);
    int box_line_vwidth = max (s->face->box_line_width, 0);
    int height;
    HBITMAP pixmap = 0;
  
!   height = s->height;
!   if (s->slice.y == 0)
!     height -= box_line_vwidth;
!   if (s->slice.y + s->slice.height >= s->img->height)
!     height -= box_line_vwidth;
  
    /* Fill background with face under the image.  Do it only if row is
       taller than image or if image has a clip mask to reduce
***************
*** 2101,2112 ****
        || s->img->pixmap == 0
        || s->width != s->background_width)
      {
!       x = s->x;
        if (s->first_glyph->left_box_line_p
  	  && s->slice.x == 0)
! 	x += box_line_hwidth;
  
-       y = s->y;
        if (s->slice.y == 0)
  	y += box_line_vwidth;
  
--- 2104,2120 ----
        || s->img->pixmap == 0
        || s->width != s->background_width)
      {
!       int x = s->x;
!       int y = s->y;
!       int width = s->background_width;
! 
        if (s->first_glyph->left_box_line_p
  	  && s->slice.x == 0)
! 	{
! 	  x += box_line_hwidth;
! 	  width -= box_line_hwidth;
! 	}
  
        if (s->slice.y == 0)
  	y += box_line_vwidth;
  
***************
*** 2150,2156 ****
  	}
        else
  #endif
! 	x_draw_glyph_string_bg_rect (s, x, y, s->background_width, height);
  
        s->background_filled_p = 1;
      }
--- 2158,2164 ----
  	}
        else
  #endif
! 	x_draw_glyph_string_bg_rect (s, x, y, width, height);
  
        s->background_filled_p = 1;
      }

bug#17115: 24.3.50; possible bug in drawing images with box on W32

[YAMAMOTO Mitsuharu]

>>>>> On Thu, 27 Mar 2014 11:42:09 +0900, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> said:

> Recently I found a bug (which is different from Bug#17114) in drawing
> images with box on the Mac port, and I suspect it also happens on W32
> because their code are similar (though I can't test it).  Could
> someone try if it is reproducible on W32 and if so, please try the
> patch at the end?

> Steps to reproduce:

>   1. $ emacs -Q -D &
>   2. (insert-image (create-image "splash.png")
> 	      (propertize " " 'face '(:box (:line-width 5)))) C-j
>   3. C-p C-p

> Result:

>   The left half of the character `t' just after the image gets erased.

Probably "splash.png" in Step 2 should have been "splash.xpm" on W32
so that s->img->mask != NULL holds.

And I managed to reproduce the similar problem also on X11 by
replacing Step 2 with:

  2'. (insert-image (create-image "splash.png" nil nil :margin 10)
	      (propertize " " 'face '(:box (:line-width 5)))) C-j

Below is a patch for X11.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

=== modified file 'src/xterm.c'
*** src/xterm.c	2014-03-21 19:04:57 +0000
--- src/xterm.c	2014-03-27 04:34:57 +0000
***************
*** 2402,2416 ****
  	{
  	  int x = s->x;
  	  int y = s->y;
  
  	  if (s->first_glyph->left_box_line_p
  	      && s->slice.x == 0)
! 	    x += box_line_hwidth;
  
  	  if (s->slice.y == 0)
  	    y += box_line_vwidth;
  
! 	  x_draw_glyph_string_bg_rect (s, x, y, s->background_width, height);
  	}
  
        s->background_filled_p = 1;
--- 2402,2420 ----
  	{
  	  int x = s->x;
  	  int y = s->y;
+ 	  int width = s->background_width;
  
  	  if (s->first_glyph->left_box_line_p
  	      && s->slice.x == 0)
! 	    {
! 	      x += box_line_hwidth;
! 	      width -= box_line_hwidth;
! 	    }
  
  	  if (s->slice.y == 0)
  	    y += box_line_vwidth;
  
! 	  x_draw_glyph_string_bg_rect (s, x, y, width, height);
  	}
  
        s->background_filled_p = 1;

bug#17115: 24.3.50; possible bug in drawing images with box on W32

[Eli Zaretskii]

> Date: Thu, 27 Mar 2014 11:42:09 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> 
> Recently I found a bug (which is different from Bug#17114) in drawing
> images with box on the Mac port, and I suspect it also happens on W32
> because their code are similar (though I can't test it).  Could
> someone try if it is reproducible on W32 and if so, please try the
> patch at the end?
> 
> Steps to reproduce:
> 
>   1. $ emacs -Q -D &
>   2. (insert-image (create-image "splash.png")
> 	      (propertize " " 'face '(:box (:line-width 5)))) C-j
>   3. C-p C-p
> 
> Result:
> 
>   The left half of the character `t' just after the image gets erased.

Confirmed.  (And yes, you need to use the XPM image to reproduce the
problem; PNG doesn't.)

> === modified file 'src/w32term.c'
> *** src/w32term.c	2014-03-26 15:57:13 +0000
> --- src/w32term.c	2014-03-27 02:32:46 +0000

Thanks, I installed this with minor changes (to keep the original
code style intact).

bug#17115: 24.3.50; possible bug in drawing images with box on W32

[YAMAMOTO Mitsuharu]

Version: 24.4

>>>>> On Thu, 27 Mar 2014 18:29:29 +0200, Eli Zaretskii <eliz@gnu.org> said:

>> Steps to reproduce:
>> 
>> 1. $ emacs -Q -D &
>> 2. (insert-image (create-image "splash.png")
>> (propertize " " 'face '(:box (:line-width 5)))) C-j
>> 3. C-p C-p
>> 
>> Result:
>> 
>> The left half of the character `t' just after the image gets erased.

> Confirmed.  (And yes, you need to use the XPM image to reproduce the
> problem; PNG doesn't.)

Thanks for confirming this.

>> === modified file 'src/w32term.c'
>> *** src/w32term.c	2014-03-26 15:57:13 +0000
>> --- src/w32term.c	2014-03-27 02:32:46 +0000

> Thanks, I installed this with minor changes (to keep the original
> code style intact).

I moved the variables `x' and `y' to the inner block because that
makes the code look more like the corresponding one in xterm.c.

Anyway, I've just installed the patch for X11.  Closing.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#17115: 24.3.50; possible bug in drawing images with box on W32

[Eli Zaretskii]

> Date: Fri, 28 Mar 2014 09:54:53 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: 17115-done@debbugs.gnu.org
> 
> > Thanks, I installed this with minor changes (to keep the original
> > code style intact).
> 
> I moved the variables `x' and `y' to the inner block because that
> makes the code look more like the corresponding one in xterm.c.

I don't object that in principle, I just think that style-related
changes like this should be separate from the other changes, because
otherwise they make it harder to understand what exactly was changed,
and why.