bug#35468: [PATCH] Refactor draw_glyph_string on X and w32

[Alex Gramiak]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex Gramiak <agrambot@gmail.com>
>> Cc: 35468@debbugs.gnu.org
>> Date: Tue, 30 Apr 2019 12:00:52 -0600
>> 
>> I think that there can be a single interface for both glyph_string
>> filling (e.g., in *_clear_glyph_string_rect), and for non-glyph_string
>> filling (e.g., in *_draw_bar_cursor), but I'm not sure it would be worth
>> it. I would think that having two separate interfaces, one taking in a
>> glyph string and the other taking in a frame, would be cleaner:
>> 
>>  fill_rectangle_glyph (struct glyph_string *, GC, int, int, int, int);
>>  fill_rectangle (struct frame *, GC, int, int, int, int);
>> 
>> There needs to be a glyph_string argument in one version so that the w32
>> version can use s->HDC. If there was only one interface, then one would
>> have to supply NULL to the glyph_string argument whenever not dealing
>> with glyph strings, which IMO is unclean.
>
> It is slightly inelegant, but it certainly isn't a catastrophe.
> Unused arguments that are there for the sake of a function signature
> compatibility is not an uncommon technique in C.

I suppose. It's only because of the w32 version that uses s->HDC that
`s' needs to be passed in at all for the glyph string version.

> What is needed in fill_rectangle_glyph from the glyph_string except
> the frame pointer?

It depends, sometimes it's just the frame and GC (and HDC), and
sometimes it's other attributes like x/y/width/height. AFAICT, all
arguments except the frame (it's always s->f) are needed for the glyph
string filling.

> (The name is also sub-optimal: what is a "rectangle glyph"?)

I meant it as "fill rectangle from glyph", but I guess I was too terse.

>> So is having to have two versions, but I think it's the best option
>> unless you know of a way to embed HDC in w32's version of GC.
>
> I see no reason to add HDC to the w32 GC, they can remain separate at
> this point.

I'm not sure how it would affect the w32 side, but I just mentioned
combining the two since it would make "more sense" from the generic POV.

> But please note that gc->foreground and background used in some cases
> are exactly the color numbers used directly in other cases, so I think
> you could always pass colors or always pass a GC.

Do you mean that I could leave out either the color or the GC argument?
The X/Cairo side at least need the GC (since it's not always s->gc), and
outside of the {get,set}_context_* way I posted, I don't see how I could
leave out the color argument.

>> As for the color manipulation, I went low-level as you said before:
>> 
>>   unsigned long foreground, background;
>>   gdif->get_context_foreground (gc, &foreground);
>>   gdif->get_context_background (gc, &background);
>>   gdif->set_context_foreground (gc, background);
>>   gdif->fill_rectangle (s, gc, x, y, w, h);
>>   gdif->set_context_foreground (gc, foreground);
>> 
>> which replaces the XGetGCValues section in x_draw_stretch_glyph_string.
>> This unfortunately is more work in the w32 case as it manipulates s->gc
>> instead of just using the calculated gc->background.
>
> I don't think I understand what you mean by "manipulates s->gc".  Can
> you show the code which does that?

I just meant that it does something like:

  unsigned long foreground = s->gc->foreground;
  unsigned long background = s->gc->background;
  s->gc->foreground = background;
  fill_rectangle (...)
  s->gc->foreground = foreground;

Where previously the foreground didn't need to be saved/restored in the
w32 implementation.

>> If that is unsatisfactory), then instead I could do something like:
>> 
>>   gdif->fill_rectangle_with_color (s, gc->background, gc, x, y, w, h);
>> 
>> Which wouldn't touch s->gc for the w32 version and would do the whole
>> XGetGCValues dance for the X version.
>
> So fill_rectangle will fill with the current background and
> fill_rectangle_with_color will fill with the specified color?  It's
> possible, yes.

Possible, yes, but would you prefer that over the above?

>> 1) Why does w32_set_mouse_face_gc use GCFont in its mask when the X
>> version doesn't?
>
> It's a remnant of old code which was deleted from the X version.  We
> should remove that.

Okay, I did that and removed all usages of GCFont in the W32 version
(and an unused #define GCFont in nsgui.h).

>> 4) The w32 versions of some procedures need to save the font around the
>> calls to font->driver->draw; is this necessary?
>
> Yes.  The X 'draw' methods accept the font as an argument, but the w32
> implementation relies on setting the font outside of the 'draw' method
> because the 'draw' method draws using the "currently selected font".
> Then we need to restore the original font.

Where do the X 'draw' methods accept the font as an argument? Looking
at, e.g., *_draw_glyph_string_foreground, font->driver->draw takes the
same arguments.

Since font->driver->draw takes in the glyph string, why can't the 'draw'
methods use SelectObject (s->hdc, FONT_HANDLER (s->font)) and
SelectObject (s->hdc, old_font)?

If they can't, is there any other way to do it inside the draw methods?
It seems like it would simplify the code on the w32 side and make it
more in line with the other backends.

> You could define a set_font interface, with only a w32 implementation.

Did you have something else in mind besides the
save/restore_secondary_context_font I posted?

>>         if (gdif->save_secondary_context_font)
>
> This name is misleading: this is not a "secondary" font.  We are
> selecting the font to be used for drawing by the font driver's 'draw'
> method.

The "secondary" here applies to "context", not "context font". I used
the name since the code changes s->hdc's (what I dubbed to be the
"secondary context") font. Would you prefer just
{save,restore}_context_font?

>> Currently this requires save_secondary_context_font to allocate memory,
>> which is unideal. One could avoid this by introducing a new union type
>> (backend_font or somesuch) and using that instead of a void*. WDYT?
>
> There's no need: the HFONT data type is just a pointer, so you can
> store it in a 'void *' without allocating anything.  If you want to be
> "more Catholic than the Pope", make that variable a unitptr_t.

Okay, I was thinking about other backends, but as long as the only
backends using that interface just pass pointers void * works just fine.



I'm having trouble with *_draw_image_foreground -- they just seem too
different to nicely abstract. Would it be okay if some generic
constructs leak into it (namely: s->img->mask)? Otherwise the common
setup that w32 does would be problematic.

Does the RestoreDC/SaveDC call need to be around both toplevel branches
(i.e., also around the w32_draw_rectangle), or just the s->img->pixmap
branch?

For reference, this is what I have right now for
gui_draw_image_foreground:

  static void
  gui_draw_image_foreground (struct glyph_string *s)
  {
    struct graphical_drawing_interface *gdif = FRAME_GDIF (s->f);
    int x = s->x;
    int y = s->ybase - image_ascent (s->img, s->face, &s->slice);

    /* If first glyph of S has a left box line, start drawing it to the
       right of that line.  */
    if (s->face->box != FACE_NO_BOX
        && s->first_glyph->left_box_line_p
        && s->slice.x == 0)
      x += eabs (s->face->box_line_width);

    /* If there is a margin around the image, adjust x- and y-position
       by that margin.  */
    if (s->slice.x == 0)
      x += s->img->hmargin;
    if (s->slice.y == 0)
      y += s->img->vmargin;

    if (gdif->save_secondary_context)
      gdif->save_secondary_context (s, CON_ALL); // SaveDC (s->hdc);

    if (gdif->glyph_has_image (s))
      {
        gdif->draw_image (s, s->img->width, s->img->height,
                          s->slice.x, s->slice.y, s->slice.width, 
s->slice.height,
                          x, y, true);
        if (!gdif->glyph_image_uses_mask (s))
          {
            /* When the image has a mask, we can expect that at
               least part of a mouse highlight or a block cursor will
               be visible.  If the image doesn't have a mask, make
               a block cursor visible by drawing a rectangle around
               the image.  I believe it's looking better if we do
               nothing here for mouse-face.  */
            if (s->hl == DRAW_CURSOR)
              {
                const int relief = eabs (s->img->relief);
                gdif->draw_rectangle (s,
                                      x - relief,
                                      y - relief,
                                      s->slice.width + relief*2 - 1,
                                      s->slice.height + relief*2 - 1);
              }
          }
      }
    else
      /* Draw a rectangle if image could not be loaded.  */
      gdif->draw_rectangle (s, x, y, s->slice.width - 1, s->slice.height - 1);

    if (gdif->restore_secondary_context)
      gdif->restore_secondary_context (s); // RestoreDC (s->hdc, -1);
  }