From: Eli Zaretskii <eliz@gnu.org>
To: Alex Gramiak <agrambot@gmail.com>
Cc: 35468@debbugs.gnu.org
Subject: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Sat, 04 May 2019 11:17:21 +0300 [thread overview]
Message-ID: <838svmmy5q.fsf@gnu.org> (raw)
In-Reply-To: <878svomynv.fsf@gmail.com> (message from Alex Gramiak on Thu, 02 May 2019 13:41:56 -0600)
> 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.
next prev parent reply other threads:[~2019-05-04 8:17 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-28 1:29 bug#35468: [PATCH] Refactor draw_glyph_string on X and w32 Alex Gramiak
2019-04-28 17:11 ` Eli Zaretskii
2019-04-28 19:46 ` Alex Gramiak
2019-04-29 17:43 ` Alex Gramiak
2019-04-30 4:59 ` Eli Zaretskii
2019-04-30 18:00 ` Alex Gramiak
2019-05-01 0:14 ` mituharu
2019-05-03 19:01 ` Alex Gramiak
2019-05-03 21:33 ` mituharu
2019-05-04 4:00 ` mituharu
2019-05-01 18:19 ` Eli Zaretskii
2019-05-02 19:41 ` Alex Gramiak
2019-05-02 20:14 ` Eli Zaretskii
2019-05-03 15:26 ` Basil L. Contovounesios
2019-05-04 8:17 ` Eli Zaretskii [this message]
2019-05-04 19:29 ` Alex Gramiak
2019-05-05 0:10 ` mituharu
2019-05-05 16:00 ` Eli Zaretskii
2019-05-05 2:34 ` Eli Zaretskii
2019-04-30 20:11 ` Alan Third
2019-05-01 17:38 ` Eli Zaretskii
2019-05-01 21:08 ` Alan Third
2019-05-02 18:14 ` Alex Gramiak
2019-05-03 21:12 ` Alan Third
2021-05-12 14:43 ` Lars Ingebrigtsen
2021-07-22 12:55 ` Lars Ingebrigtsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=838svmmy5q.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=35468@debbugs.gnu.org \
--cc=agrambot@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.