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

[Eli Zaretskii]

> From: Alex Gramiak <agrambot@gmail.com>
> Cc: 35468@debbugs.gnu.org
> Date: Thu, 02 May 2019 13:41:56 -0600

Sorry, it took slightly more than "a few hours".

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

First, it should be possible to have just 1 interface for getting both
colors, with the semantics that if one of the pointers is NULL, the
corresponding color is not required.  I'd call this interface
select_colors.  If w32 doesn't need that, we could either (1) omit
implementing such an interface for w32 (if it's _never_ needed there),
or (2) not call it when the frame is a w32 GUI frame.  So I see no
problems in this part.

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

I think so, yes.  And it sounds like Mituharu-san prefers something
like that as well.

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

The way I wrote it was confusing: by the 'draw' method I actually
meant the external APIs called by the 'draw' method, like
XftDrawGlyphs.  Compare that with w32's ExtTextOutW in w32font_draw.

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

The draw method doesn't know if it's the last call with a given font.
Only the caller knows that.  So the caller knows when it needs to
restore the previous font.

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

I don't think I understand the difficulties, sorry.  Why is
s->img->mask a problem?

In any case, it's not 100% "verboten" for platform-specific code to
look at the internals of 'struct glyph_string', if the interface needs
many different members of that struct.  Avoiding this is just a
general rule, which makes it easier to implement generic interfaces
that will fit future uses.  However draw_image (which, btw, I'd call
draw_image_foreground) looks specialized enough to be exempt of that
rule.

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

It doesn't look like the call to w32_draw_rectangle needs that, as the
code doesn't do that elsewhere.

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

What details does glyph_has_image hide?  Is that just to test
s->img->pixmap?

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

And what does glyph_image_uses_mask hide?  AFAIU, the current code
simply looks at s->img->mask, and if so, why do we need an interface
for that?

Thanks.