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

[Alex Gramiak <agrambot@gmail.com>]

Eli Zaretskii <eliz@gnu.org> writes:

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

Ah, I see. I'll keep the setter and rename it to, say,
set_device_context_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?

I meant problem as in that it's "leaking" the internals a bit.

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

That's good; it will be easier to work with the image procedures in that
case.

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

On most platforms, yes, but the Cairo drawing uses s->img->cr_data
instead.

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

I was thinking that since AFAIU the Cairo drawing doesn't set
s->img->mask it wouldn't make sense, from an interface POV, to check it
directly. I suppose it doesn't really matter in that case, and it would
be faster to just check s->img->mask even if the backend doesn't use it.