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

[Alex Gramiak <agrambot@gmail.com>]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex Gramiak <agrambot@gmail.com>
>> Cc: 35468@debbugs.gnu.org
>> Date: Mon, 29 Apr 2019 11:43:23 -0600
>> 
>> I'm in {x,w32}_draw_box_rect right now, trying to generalize both
>> versions. The issue is that the fill command in each accepts different
>> arguments; specifically the w32 version takes in the color explicitly
>> and uses s->hdc instead of s->gc. So I think there will have to be 2
>> different fill_rectangle interface procedures: one for glyph strings (so
>> that the w32 version can access s->hdc), and another for other
>> procedures like *_draw_bar_cursor.
>
> Would it be possible to generalize this into a single interface?  The
> GC vs HDC part can be generalized by passing both (HDC will be NULL in
> the X and NS cases), and the color will be passed as in the w32
> version, with the X version doing its thing with GCForeground
> internally.  Alternatively, we could have a set_foreground interface
> that will do nothing for w32 and for X call XGetGCValues and
> XSetForeground, to set up s->gc.  The second alternative will probably
> be more efficient.
>
> If this doesn't work, can you tell why?

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

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'm not sure how
one would make a no-op version of setting the context foreground work in
all fill calls.

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.

Alternatively, if you don't want another version, there could be a
mandatory color argument that one could supply with a pre-defined
invalid color to instead use the GC color.


I have some more questions regarding w32 incompatibilities that I ran
into:

1) Why does w32_set_mouse_face_gc use GCFont in its mask when the X
version doesn't?

2) Why does w32_draw_glyphless_glyph_string_foreground have the boolean
`with_background' instead of using false unconditionally like the X
version does? Should the X version be updated?

3) Why does w32_draw_glyph_string for FACE_UNDER_LINE use a thickness of
1 for w32_fill_area instead of using s->underline_thickness like X/NS
do? This seems like a bug.

4) The w32 versions of some procedures need to save the font around the
calls to font->driver->draw; is this necessary? Specifically, see
w32_draw_glyph_string_foreground. Assuming it's necessary, I generalized
it as:

  static void
  gui_draw_glyph_string_foreground (struct glyph_string *s)
  {
    int i, x;
    struct graphical_drawing_interface *gdif = FRAME_GDIF (s->f);

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

    /* Currently just used by the w32 port to update S->hdc.  */
    if (gsif->update_secondary_context)
      gsif->update_secondary_context (s, CON_BACK | CON_FORE | CON_ALIGN);

    /* Draw characters of S as rectangles if S's font could not be
       loaded.  */
    if (s->font_not_found_p)
      {
        for (i = 0; i < s->nchars; ++i)
          {
            struct glyph *g = s->first_glyph + i;
            gdif->draw_rectangle (s, x, s->y, g->pixel_width - 1, s->height - 1);
            x += g->pixel_width;
          }
      }
    else
      {
        struct font *font = s->font;
        int boff = font->baseline_offset;
        int y;
        void *old_font;


        if (gdif->save_secondary_context_font)
          old_font = gdif->save_secondary_context_font (s);

        if (font->vertical_centering)
          boff = VCENTER_BASELINE_OFFSET (font, s->f) - boff;

        y = s->ybase - boff;
        if (s->for_overlaps
            || (s->background_filled_p && s->hl != DRAW_CURSOR))
          font->driver->draw (s, 0, s->nchars, x, y, false);
        else
          font->driver->draw (s, 0, s->nchars, x, y, true);
        if (s->face->overstrike)
          font->driver->draw (s, 0, s->nchars, x + 1, y, false);

        if (gdif->set_secondary_context_font)
          gdif->set_secondary_context_font (s, old_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?