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

What is needed in fill_rectangle_glyph from the glyph_string except
the frame pointer?  (The name is also sub-optimal: what is a
"rectangle glyph"?)

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

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

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

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

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

I don't know, but the w32 code was added in a dedicated commit.
Unfortunately, I cannot find any trace of discussing this change or
any bug reports that led to it.  We could leave this code under the
FRAME_W32_P condition.

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

It's a bug of sorts, but you will never see any buggy behavior as
result of it, because there are no Windows fonts that specify
thickness other than zero or 1.  I'm okay with using the X code for
w32 as well, it cannot do any harm.

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

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

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

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

Thanks.