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

It's late down here, and I had a bad week and an even worse day.  So
just a few quick answers for now, and the rest will follow in a few
hours.

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

Why can't you pass HDC itself?  It's just a pointer.

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

Yes, that was my impression from looking at the code.  We will just
need to pack the color into a GC in some cases.

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

Look at the driver's 'draw' method itself, e.g. xftfont_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)?

It could, but that would be less efficient, since we many times call
the draw method several times in a row.

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

I would leave this for another rainy day, and for now just have an
interface which just w32 will implement.

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

Not besides, instead.

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

There's no context here, either.  That call selects a different font,
and returns a handle of the previous font so that it could be restored
later.

Stay tuned for the rest.

Thanks.