[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
From: |
Alex Gramiak |
Subject: |
bug#35468: [PATCH] Refactor draw_glyph_string on X and w32 |
Date: |
Tue, 30 Apr 2019 12:00:52 -0600 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) |
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?