[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
From: |
Eli Zaretskii |
Subject: |
bug#35468: [PATCH] Refactor draw_glyph_string on X and w32 |
Date: |
Sat, 04 May 2019 11:17:21 +0300 |
> 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.
bug#35468: [PATCH] Refactor draw_glyph_string on X and w32, Alex Gramiak, 2019/05/03