GNU bug report logs - #35468
[PATCH] Refactor draw_glyph_string on X and w32

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 35468 in the body.
You can then email your comments to 35468 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Alex Gramiak <agrambot <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Refactor draw_glyph_string on X and w32
Date: Sat, 27 Apr 2019 19:29:30 -0600

[Message part 1 (text/plain, inline)]

This merges x_draw_glyph_string and w32_draw_glyph_string together to
remove duplicated code.

I wanted to do it for NS as well, but it seems just a bit too different
to make it easily work.

[0001-Refactor-draw_glyph_string-on-X-and-w32.patch (text/x-patch, attachment)]

Message #8 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Sun, 28 Apr 2019 20:11:54 +0300

> From: Alex Gramiak <agrambot <at> gmail.com>
> Date: Sat, 27 Apr 2019 19:29:30 -0600
> 
> This merges x_draw_glyph_string and w32_draw_glyph_string together to
> remove duplicated code.
> 
> I wanted to do it for NS as well, but it seems just a bit too different
> to make it easily work.

If we don't include NS in this, we won't actually gain enough to
justify the reshuffle.  What prevented you from including NS, it looks
to me the code is very similar?  But maybe wait with the answer until
you read the rest of the comments below.

> +struct draw_glyph_string_interface

I'd prefer draw_glyphs_interface, it's shorter.  And given the
comments below, maybe the name should be gui_interface.

> -x_draw_glyph_string (struct glyph_string *s)
> +x_draw_underwave (struct glyph_string *s)
>  {
> -  bool relief_drawn_p = false;
> -
> -  /* If S draws into the background of its successors, draw the
> -     background of the successors first so that S can draw into it.
> -     This makes S->next use XDrawString instead of XDrawImageString.  */
> -  if (s->next && s->right_overhang && !s->for_overlaps)
> +  if (s->face->underline_defaulted_p)
> +    x_draw_underwave_1 (s);
> +  else
>      {
> -      int width;
> -      struct glyph_string *next;
> -
> -      for (width = 0, next = s->next;
> -	   next && width < s->right_overhang;
> -	   width += next->width, next = next->next)
> -	if (next->first_glyph->type != IMAGE_GLYPH)
> -	  {
> -	    x_set_glyph_string_gc (next);
> -	    x_set_glyph_string_clipping (next);
> -	    if (next->first_glyph->type == STRETCH_GLYPH)
> -	      x_draw_stretch_glyph_string (next);
> -	    else
> -	      x_draw_glyph_string_background (next, true);
> -	    next->num_clips = 0;
> -	  }
> +      XGCValues xgcv;
> +      XGetGCValues (s->display, s->gc, GCForeground, &xgcv);
> +      XSetForeground (s->display, s->gc, s->face->underline_color);
> +      x_draw_underwave_1 (s);
> +      XSetForeground (s->display, s->gc, xgcv.foreground);
>      }
> +}

I think this stops short of the goal.  The goal is to refactor the
code into terminal-independent code and terminal-dependent code, and
then arrange the latter in meaningful interfaces that will be useful
henceforth for writing display code.  By contrast, what you did leaves
in terminal-specific parts code that is completely independent of the
window-system, and OTOH doesn't try to identify terminal-specific
parts which are different implementations of the same general idea.
For example, all GUI terminals have code that sets foreground and
background colors, code that clears a rectangle, code that fills a
rectangle with a given color, code that draws a line between 2 points,
etc.  We should identify such pieces of code, give them names, and
make them the methods called via the redisplay interface pointers.  As
a rule, no terminal-independent code should ever appear in the
window-system specific part, not even logic that looks at
terminal-independent data structures, like this, for example:

  +      if (s->face->strike_through_color_defaulted_p)
  +        {
  +          w32_fill_area (s->f, s->hdc, s->gc->foreground, s->x,
  +                         y, s->width, height);
  +        }
  +      else
  +        {
  +          w32_fill_area (s->f, s->hdc, s->face->strike_through_color, s->x,
  +                         y, s->width, height);
  +        }

The meaning of s->face->strike_through_color_defaulted_p has no place
in w32term.c, it should be in the terminal-independent part, and
w32_fill_area should be a w32-specific implementation of a GUI drawing
primitive.  Likewise with stuff like XGetGCValues on X and DC on w32.

IOW, if we are refactoring this stuff, let's do it right, not just
ad-hoc because the current code by some chance lends itself to a
particular division into parts, not just because it's easy.  If we go
the easy way, chances are that tomorrow someone who'd like to add a
new GUI feature will again have to duplicate code because of lack of
necessary primitive building blocks, which were left lumped together
with terminal-independent code instead of being separated.

The result of this refactoring should be more low-level and more
primitive interfaces, and they should each one make sense, not be
ad-hoc.  It means the job becomes more complex, and you will probably
need to ask questions regarding the GUI systems you are less familiar
with.  But the result will IMO much better and future-proof.

And once again, please put all the new gui_* functions extracted from
*term.c on a new file gui_term.c, not in xdisp.c.

Thanks.

Message #11 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Alex Gramiak <agrambot <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Sun, 28 Apr 2019 13:46:46 -0600

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Alex Gramiak <agrambot <at> gmail.com>
>> Date: Sat, 27 Apr 2019 19:29:30 -0600
>> 
>> This merges x_draw_glyph_string and w32_draw_glyph_string together to
>> remove duplicated code.
>> 
>> I wanted to do it for NS as well, but it seems just a bit too different
>> to make it easily work.
>
> If we don't include NS in this, we won't actually gain enough to
> justify the reshuffle.

I still think that removing duplication between 2 out of 3 is worth it
considering the size of the removal, but I started to do this because
I'm working on a new backend (GTK without depending on X) that would
benefit from this refactoring.

> What prevented you from including NS, it looks to me the code is very
> similar?

Mostly the differences in clipping behaviour that seemed just slightly
incompatible with the way gui_draw_glyphs_string does it. I'll look at
it a bit harder.

It also doesn't have the two sections (the s->prev and s->next
conditionals) at the end, which I'm not sure is a bug or unnecessary
there. Those could just be surrounded by a (!FRAME_NS_P (s->f))
conditional though.

>> +struct draw_glyph_string_interface
>
> I'd prefer draw_glyphs_interface, it's shorter.  And given the
> comments below, maybe the name should be gui_interface.

draw_glyphs_interface doesn't seem bad, as long as glyph strings are the
only types of glyphs that are drawn (otherwise draw_glyphs would be too
broad).

I think gui_interface is too broad of a description, since many of the
terminal hooks and frame parameter handlers (setting GUI window
properties, accessing color structures, setting WM hints, etc.) could
also be considered as belonging to a GUI interface. Perhaps
gui_drawing_interface or graphical_drawing_interface?

> The result of this refactoring should be more low-level and more
> primitive interfaces, and they should each one make sense, not be
> ad-hoc.  It means the job becomes more complex, and you will probably
> need to ask questions regarding the GUI systems you are less familiar
> with.  But the result will IMO much better and future-proof.

I'll see what I can do.

Message #14 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Alex Gramiak <agrambot <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Mon, 29 Apr 2019 11:43:23 -0600

>> The result of this refactoring should be more low-level and more
>> primitive interfaces, and they should each one make sense, not be
>> ad-hoc.  It means the job becomes more complex, and you will probably
>> need to ask questions regarding the GUI systems you are less familiar
>> with.  But the result will IMO much better and future-proof.
>
> I'll see what I can do.

Alright, for the first question may I ask if you're okay with the
following abstraction (which may be the start of a pattern)?

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.

  void
  gui_draw_glyph_string_box (struct glyph_string *s)
  {
    int width, left_x, right_x, top_y, bottom_y, last_x;
    bool raised_p, left_p, right_p;
    struct glyph *last_glyph;

    last_x = ((s->row->full_width_p && !s->w->pseudo_window_p)
              ? WINDOW_RIGHT_EDGE_X (s->w)
              : window_box_right (s->w, s->area));

    /* The glyph that may have a right box line.  */
    last_glyph = (s->cmp || s->img
                  ? s->first_glyph
                  : s->first_glyph + s->nchars - 1);

    width = eabs (s->face->box_line_width);
    raised_p = s->face->box == FACE_RAISED_BOX;
    left_x = s->x;
    right_x = (s->row->full_width_p && s->extends_to_end_of_line_p
               ? last_x - 1
               : min (last_x, s->x + s->background_width) - 1);
    top_y = s->y;
    bottom_y = top_y + s->height - 1;

    left_p = (s->first_glyph->left_box_line_p
              || (s->hl == DRAW_MOUSE_FACE
                  && (s->prev == NULL
                      || s->prev->hl != s->hl)));
    right_p = (last_glyph->right_box_line_p
               || (s->hl == DRAW_MOUSE_FACE
                   && (s->next == NULL
                       || s->next->hl != s->hl)));

    if (s->face->box == FACE_SIMPLE_BOX)
      FRAME_GDIF (s->f)->draw_box_rect (s,
                                        left_x,
                                        top_y,
                                        right_x,
                                        bottom_y,
                                        width,
                                        left_p,
                                        right_p);
    else
      {
        FRAME_GDIF (s->f)->setup_relief_colors (s);
        FRAME_GDIF (s->f)->draw_relief_rect (s->f,
                                             left_x,
                                             top_y,
                                             right_x,
                                             bottom_y,
                                             width,
                                             raised_p,
                                             true,
                                             true,
                                             left_p,
                                             right_p);
      }
  }

  void
  gui_draw_box_rect (struct glyph_string *s,
                     int left_x, int top_y, int right_x, int bottom_y, int width,
                     bool left_p, bool right_p)
  {
    struct graphical_drawing_interface *gdif = FRAME_GDIF (s->f);

    gdif->save_context (s);

    gdif->set_context_clip (s);
    gdif->set_context_foreground (s, s->face->box_color);

    /* Top.  */
    gdif->fill_rectangle (s, s->gc, left_x, top_y, right_x - left_x + 1, width);

    /* Left.  */
    if (left_p)
      {
        gdif->fill_rectangle (s, s->gc,
                              left_x, top_y,
                              width, bottom_y - top_y + 1);
      }

    /* Bottom.  */
    gdif->fill_rectangle (s, s->gc,
                          left_x, bottom_y - width + 1,
                          right_x - left_x + 1, width);

    /* Right.  */
    if (right_p)
      {
        gdif->fill_rectangle (s, s->gc,
                              right_x - width + 1, top_y,
                              width, bottom_y - top_y + 1);
      }

    gdif->reset_context (s);
  }

It doesn't work for NS partially because of the following section only
present in the NS equivalent of gui_draw_glyph_string_box. Would you be
okay with putting this in the generic version inside a FRAME_NS_P (s->f)
check? I don't know why only NS has this, though...

  if (s->hl == DRAW_MOUSE_FACE)
    {
      face = FACE_FROM_ID_OR_NULL (s->f,
				   MOUSE_HL_INFO (s->f)->mouse_face_face_id);
      if (!face)
        face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
    }
  else
    face = s->face;

Message #17 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Tue, 30 Apr 2019 07:59:37 +0300

> From: Alex Gramiak <agrambot <at> gmail.com>
> Cc: 35468 <at> 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?

> It doesn't work for NS partially because of the following section only
> present in the NS equivalent of gui_draw_glyph_string_box. Would you be
> okay with putting this in the generic version inside a FRAME_NS_P (s->f)
> check? I don't know why only NS has this, though...
> 
>   if (s->hl == DRAW_MOUSE_FACE)
>     {
>       face = FACE_FROM_ID_OR_NULL (s->f,
> 				   MOUSE_HL_INFO (s->f)->mouse_face_face_id);
>       if (!face)
>         face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
>     }
>   else
>     face = s->face;

I don't know why this is TRT, either.  We could ask Alan to look into
this, or we could simply remove that, as other terminals don't use
box_line_width from the mouse face, they use s->face instead.

Thanks.

Message #20 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Alex Gramiak <agrambot <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Tue, 30 Apr 2019 12:00:52 -0600

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Alex Gramiak <agrambot <at> gmail.com>
>> Cc: 35468 <at> 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?

Message #23 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Alan Third <alan <at> idiocy.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35468 <at> debbugs.gnu.org, Alex Gramiak <agrambot <at> gmail.com>
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Tue, 30 Apr 2019 21:11:29 +0100

On Tue, Apr 30, 2019 at 07:59:37AM +0300, Eli Zaretskii wrote:
> > From: Alex Gramiak <agrambot <at> gmail.com>
> > Cc: 35468 <at> debbugs.gnu.org
> > Date: Mon, 29 Apr 2019 11:43:23 -0600
> > 
> > It doesn't work for NS partially because of the following section only
> > present in the NS equivalent of gui_draw_glyph_string_box. Would you be
> > okay with putting this in the generic version inside a FRAME_NS_P (s->f)
> > check? I don't know why only NS has this, though...
> > 
> >   if (s->hl == DRAW_MOUSE_FACE)
> >     {
> >       face = FACE_FROM_ID_OR_NULL (s->f,
> > 				   MOUSE_HL_INFO (s->f)->mouse_face_face_id);
> >       if (!face)
> >         face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
> >     }
> >   else
> >     face = s->face;
> 
> I don't know why this is TRT, either.  We could ask Alan to look into
> this, or we could simply remove that, as other terminals don't use
> box_line_width from the mouse face, they use s->face instead.

A quick test doesn’t turn up any immediate issues with removing this,
but I’m unsure in what circumstances DRAW_MOUSE_FACE would be in use.
-- 
Alan Third

Message #26 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: mituharu <at> math.s.chiba-u.ac.jp
To: "Alex Gramiak" <agrambot <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Wed, 1 May 2019 09:14:50 +0900

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

Unlike the NS port, the terminal code in the Mac port is much like the X11
version.
But there is one notable difference in the above respect.
It defines mac_erase_rectangle, which is like mac_fill_rectangle (the
XFillRectangle counterpart)
but uses the background color of GC and also handles stipples:

/* Mac replacement for XFillRectangle.  */

static void
mac_fill_rectangle (struct frame *f, GC gc, int x, int y, int width, int
height)
{
  MAC_BEGIN_DRAW_TO_FRAME (f, gc, context);
  CGContextSetFillColorWithColor (context, gc->cg_fore_color);
  {
    CGRect rect = mac_rect_make (f, x, y, width, height);

    CGContextFillRects (context, &rect, 1);
  }
  MAC_END_DRAW_TO_FRAME (f);
}

static void
mac_erase_rectangle (struct frame *f, GC gc, int x, int y,
		     int width, int height)
{
  MAC_BEGIN_DRAW_TO_FRAME (f, gc, context);
  {
    CGRect rect = mac_rect_make (f, x, y, width, height);

    CG_CONTEXT_FILL_RECT_WITH_GC_BACKGROUND (f, context, rect, gc);
    if (gc->xgcv.fill_style == FillOpaqueStippled && gc->xgcv.stipple)
      {
	CGContextClipToRects (context, &rect, 1);
	CGContextSetFillColorWithColor (context, gc->cg_fore_color);
	int scale = CFArrayGetCount (gc->xgcv.stipple);
	if (FRAME_BACKING_SCALE_FACTOR (f) < scale)
	  scale = FRAME_BACKING_SCALE_FACTOR (f);
	CGImageRef image_mask =
	  (CGImageRef) CFArrayGetValueAtIndex (gc->xgcv.stipple, scale - 1);
	rect = CGRectMake (0, 0, CGImageGetWidth (image_mask) / (CGFloat) scale,
			   CGImageGetHeight (image_mask) / (CGFloat) scale);
	CGContextScaleCTM (context, 1, -1);
	CGContextSetInterpolationQuality (context, kCGInterpolationNone);
	CGContextDrawTiledImage (context, rect, image_mask);
      }
  }
  MAC_END_DRAW_TO_FRAME (f);
}

BTW, the code between MAC_BEGIN_DRAW_TO_FRAME and MAC_END_DRAW_TO_FRAME
is executed in the dedicated drawing thread with the help of Grand Central
Dispatch.
See https://bitbucket.org/mituharu/emacs-mac/src/master/src/macterm.c for
the full source code.

I guess introducing the erase_rectagle handler makes things simpler and
efficient.

					YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

Message #29 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alan Third <alan <at> idiocy.org>
Cc: 35468 <at> debbugs.gnu.org, agrambot <at> gmail.com
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Wed, 01 May 2019 20:38:17 +0300

> Date: Tue, 30 Apr 2019 21:11:29 +0100
> From: Alan Third <alan <at> idiocy.org>
> Cc: Alex Gramiak <agrambot <at> gmail.com>, 35468 <at> debbugs.gnu.org
> 
> > >   if (s->hl == DRAW_MOUSE_FACE)
> > >     {
> > >       face = FACE_FROM_ID_OR_NULL (s->f,
> > > 				   MOUSE_HL_INFO (s->f)->mouse_face_face_id);
> > >       if (!face)
> > >         face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
> > >     }
> > >   else
> > >     face = s->face;
> > 
> > I don't know why this is TRT, either.  We could ask Alan to look into
> > this, or we could simply remove that, as other terminals don't use
> > box_line_width from the mouse face, they use s->face instead.
> 
> A quick test doesn’t turn up any immediate issues with removing this,
> but I’m unsure in what circumstances DRAW_MOUSE_FACE would be in use.

E.g., when you hover the mouse pointer above the mouse-sensitive
portions of the mode line.

Message #32 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Wed, 01 May 2019 21:19:09 +0300

> From: Alex Gramiak <agrambot <at> gmail.com>
> Cc: 35468 <at> 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.

Message #35 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Alan Third <alan <at> idiocy.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35468 <at> debbugs.gnu.org, agrambot <at> gmail.com
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Wed, 1 May 2019 22:08:47 +0100

On Wed, May 01, 2019 at 08:38:17PM +0300, Eli Zaretskii wrote:
> > Date: Tue, 30 Apr 2019 21:11:29 +0100
> > From: Alan Third <alan <at> idiocy.org>
> > Cc: Alex Gramiak <agrambot <at> gmail.com>, 35468 <at> debbugs.gnu.org
> > 
> > > >   if (s->hl == DRAW_MOUSE_FACE)
> > > >     {
> > > >       face = FACE_FROM_ID_OR_NULL (s->f,
> > > > 				   MOUSE_HL_INFO (s->f)->mouse_face_face_id);
> > > >       if (!face)
> > > >         face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
> > > >     }
> > > >   else
> > > >     face = s->face;
> > > 
> > > I don't know why this is TRT, either.  We could ask Alan to look into
> > > this, or we could simply remove that, as other terminals don't use
> > > box_line_width from the mouse face, they use s->face instead.
> > 
> > A quick test doesn’t turn up any immediate issues with removing this,
> > but I’m unsure in what circumstances DRAW_MOUSE_FACE would be in use.
> 
> E.g., when you hover the mouse pointer above the mouse-sensitive
> portions of the mode line.

It looks exactly the same to me with or without the code. Perhaps I’d
see something different if I was using some theme which defined mouse
face differently.

I really don’t see any reason why NS should behave differently from
the other terms here.
-- 
Alan Third

Message #38 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Alex Gramiak <agrambot <at> gmail.com>
To: Alan Third <alan <at> idiocy.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Thu, 02 May 2019 12:14:10 -0600

Alan Third <alan <at> idiocy.org> writes:

> It looks exactly the same to me with or without the code. Perhaps I’d
> see something different if I was using some theme which defined mouse
> face differently.
>
> I really don’t see any reason why NS should behave differently from
> the other terms here.

There's a similar block in ns_maybe_dumpglyphs_background. Could you
test with that block removed as well? Also, there's a conditional check
(s->hl != DRAW_CURSOR) there that's not present in the other versions;
could you see if that's necessary?

Message #41 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Alex Gramiak <agrambot <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Thu, 02 May 2019 13:41:56 -0600

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Alex Gramiak <agrambot <at> gmail.com>
>> Cc: 35468 <at> 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.

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.

> What is needed in fill_rectangle_glyph from the glyph_string except
> the frame pointer?

It depends, sometimes it's just the frame and GC (and HDC), and
sometimes it's other attributes like x/y/width/height. AFAICT, all
arguments except the frame (it's always s->f) are needed for the glyph
string filling.

> (The name is also sub-optimal: what is a "rectangle glyph"?)

I meant it as "fill rectangle from glyph", but I guess I was too terse.

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

I'm not sure how it would affect the w32 side, but I just mentioned
combining the two since it would make "more sense" from the generic POV.

> 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?
The X/Cairo side at least need the GC (since it's not always s->gc), and
outside of the {get,set}_context_* way I posted, I don't see how I could
leave out the color argument.

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

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

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

Okay, I did that and removed all usages of GCFont in the W32 version
(and an unused #define GCFont in nsgui.h).

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

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?
It seems like it would simplify the code on the w32 side and make it
more in line with the other backends.

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

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

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

Okay, I was thinking about other backends, but as long as the only
backends using that interface just pass pointers void * works just fine.



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.

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?

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))
      {
        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))
          {
            /* When the image has a mask, we can expect that at
               least part of a mouse highlight or a block cursor will
               be visible.  If the image doesn't have a mask, make
               a block cursor visible by drawing a rectangle around
               the image.  I believe it's looking better if we do
               nothing here for mouse-face.  */
            if (s->hl == DRAW_CURSOR)
              {
                const int relief = eabs (s->img->relief);
                gdif->draw_rectangle (s,
                                      x - relief,
                                      y - relief,
                                      s->slice.width + relief*2 - 1,
                                      s->slice.height + relief*2 - 1);
              }
          }
      }
    else
      /* Draw a rectangle if image could not be loaded.  */
      gdif->draw_rectangle (s, x, y, s->slice.width - 1, s->slice.height - 1);

    if (gdif->restore_secondary_context)
      gdif->restore_secondary_context (s); // RestoreDC (s->hdc, -1);
  }

Message #44 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Thu, 02 May 2019 23:14:55 +0300

> From: Alex Gramiak <agrambot <at> gmail.com>
> Cc: 35468 <at> 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.

Message #47 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35468 <at> debbugs.gnu.org, Alex Gramiak <agrambot <at> gmail.com>
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Fri, 03 May 2019 16:26:08 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

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

I hope things look up sooner than later, and thank you for your efforts
regardless.

-- 
Basil

Message #50 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Alex Gramiak <agrambot <at> gmail.com>
To: mituharu <at> math.s.chiba-u.ac.jp
Cc: Eli Zaretskii <eliz <at> gnu.org>, 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Fri, 03 May 2019 13:01:18 -0600

mituharu <at> math.s.chiba-u.ac.jp writes:

> Unlike the NS port, the terminal code in the Mac port is much like the X11
> version.

I looked at your code and fortunately it indeed looks like the same
abstractions for glyph drawing that would work for the X11 version would
work for the Mac port.

> But there is one notable difference in the above respect.
> It defines mac_erase_rectangle, which is like mac_fill_rectangle (the
> XFillRectangle counterpart)
> but uses the background color of GC and also handles stipples:
>
> /* Mac replacement for XFillRectangle.  */
>
> static void
> mac_fill_rectangle (struct frame *f, GC gc, int x, int y, int width, int
> height)
> {
>   MAC_BEGIN_DRAW_TO_FRAME (f, gc, context);
>   CGContextSetFillColorWithColor (context, gc->cg_fore_color);
>   {
>     CGRect rect = mac_rect_make (f, x, y, width, height);
>
>     CGContextFillRects (context, &rect, 1);
>   }
>   MAC_END_DRAW_TO_FRAME (f);
> }
>
> static void
> mac_erase_rectangle (struct frame *f, GC gc, int x, int y,
> 		     int width, int height)
> {
>   MAC_BEGIN_DRAW_TO_FRAME (f, gc, context);
>   {
>     CGRect rect = mac_rect_make (f, x, y, width, height);
>
>     CG_CONTEXT_FILL_RECT_WITH_GC_BACKGROUND (f, context, rect, gc);
>     if (gc->xgcv.fill_style == FillOpaqueStippled && gc->xgcv.stipple)
>       {
> 	CGContextClipToRects (context, &rect, 1);
> 	CGContextSetFillColorWithColor (context, gc->cg_fore_color);
> 	int scale = CFArrayGetCount (gc->xgcv.stipple);
> 	if (FRAME_BACKING_SCALE_FACTOR (f) < scale)
> 	  scale = FRAME_BACKING_SCALE_FACTOR (f);
> 	CGImageRef image_mask =
> 	  (CGImageRef) CFArrayGetValueAtIndex (gc->xgcv.stipple, scale - 1);
> 	rect = CGRectMake (0, 0, CGImageGetWidth (image_mask) / (CGFloat) scale,
> 			   CGImageGetHeight (image_mask) / (CGFloat) scale);
> 	CGContextScaleCTM (context, 1, -1);
> 	CGContextSetInterpolationQuality (context, kCGInterpolationNone);
> 	CGContextDrawTiledImage (context, rect, image_mask);
>       }
>   }
>   MAC_END_DRAW_TO_FRAME (f);
> }

Why does it use the background color of GC? It appears that the
Cairo version uses the foreground color of GC; is that because the Cairo
version doesn't handle stippling?

> I guess introducing the erase_rectagle handler makes things simpler and
> efficient.

Looking over the code, and considering that stippling is quite uncommon
nowadays (GTK 3 removed it, AFAIK), I think the best approach would be
to define 3 separate interface procedures:

1) fill_rectangle: handles x_fill_rectangle/mac_fill_rectangle

2) fill_rectangle_with_color: handles (1), but also with the temporary
overriding of the GC that's done frequently for mac_erase_rectangle.

3) fill_rectangle_with_stipple: handles the temporary XFillStyle
overriding of the GC.

A third procedure wouldn't be inelegant in comparison to the
alternatives, since otherwise I would need a set_context_stipple
procedure to indicate stippling.

Then mac_fill_rectangle can be assigned to fill_rectangle, and
mac_erase_rectangle can be assigned to fill_rectangle_with_stipple.

Message #53 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Alan Third <alan <at> idiocy.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Fri, 3 May 2019 22:12:36 +0100

On Thu, May 02, 2019 at 12:14:10PM -0600, Alex Gramiak wrote:
> Alan Third <alan <at> idiocy.org> writes:
> 
> > It looks exactly the same to me with or without the code. Perhaps I’d
> > see something different if I was using some theme which defined mouse
> > face differently.
> >
> > I really don’t see any reason why NS should behave differently from
> > the other terms here.
> 
> There's a similar block in ns_maybe_dumpglyphs_background. Could you
> test with that block removed as well? Also, there's a conditional check
> (s->hl != DRAW_CURSOR) there that's not present in the other versions;
> could you see if that's necessary?

Removing the block in ns_maybe_dumpglyphs_background seems to make no
difference here. Removing the DRAW_CURSOR check results in the cursor
being drawn as a blank (background colour) rectangle on occasion.

I’m not sure what the difference is, but there are a couple of
comments in nsterm.m which reference NS drawing the cursor differently
to other terms, i.e.

  /* We draw the cursor (with NSRectFill), then draw the glyph on top
     (other terminals do it the other way round).  We must set
     w->phys_cursor_width to the cursor width.  For bar cursors, that
     is CURSOR_WIDTH; for box cursors, it is the glyph width.  */

and 

  /* NOTE: under NS this is NOT used to draw cursors, but we must avoid
     overwriting cursor (usually when cursor on a tab).  */

and I suspect this check is related.
-- 
Alan Third

Message #56 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: mituharu <at> math.s.chiba-u.ac.jp
To: "Alex Gramiak" <agrambot <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Sat, 4 May 2019 06:33:05 +0900

> mituharu <at> math.s.chiba-u.ac.jp writes:
>
>> Unlike the NS port, the terminal code in the Mac port is much like the
>> X11
>> version.
>
> I looked at your code and fortunately it indeed looks like the same
> abstractions for glyph drawing that would work for the X11 version would
> work for the Mac port.

I deliberately made the Mac port code aligned to the X11 version
for the consistency in behavior as well as the ease of catching
up upstream changes.

> Why does it use the background color of GC? It appears that the
> Cairo version uses the foreground color of GC; is that because the Cairo
> version doesn't handle stippling?

I wrote both Mac and cairo versions, and I used the foreground
color for cairo (although I already used the background color on
the Mac port) to minimize the changes from the Xlib version
because it was incorporated into xterm.c.

Actually the stipple bitmap support for the Mac port was added
very recently.  It had been missing in the Mac port for more than
a decade because I also thought it is an outdated thing and not
worth implemented.  But I changed my mind by looking at an
unexpected use of stipple bitmaps:

  https://lists.gnu.org/r/emacs-devel/2019-03/msg00502.html

So the use of the background color of GC on the Mac port is not
for stipple bitmaps (at least historically) but mainly for the
following reasons:

  1. It can avoid XGetGCValues dance you mentioned without the
     drastic changes in the code that may make catching up
     mainstream difficult.
  2. There are multiple cases that "filling with the background
     color" is useful.
  3. The Mac port supports "vibrancy" (a frosted-glass-like
     translucency effect) only for the background color.  So
     filling with the background color is implemented differently
     from doing that with the foreground color on the Mac
     port (see the macro CG_CONTEXT_FILL_RECT_WITH_GC_BACKGROUND
     in macterm.h).  It is regarded as "special stipples" and
     actually controlled by the stipple attibute of faces:
     https://bitbucket.org/mituharu/emacs-mac/src/df827786d7a7fb0a0e2f27577af67e32d9a888a9/doc/emacs/macport.texi#lines-519
     In retrospect, it is natural to associate "stipple"
     operations with "erasing" (filling with the background
     color) in this sense.

>> I guess introducing the erase_rectagle handler makes things simpler and
>> efficient.
>
> Looking over the code, and considering that stippling is quite uncommon
> nowadays (GTK 3 removed it, AFAIK), I think the best approach would be
> to define 3 separate interface procedures:
>
> 1) fill_rectangle: handles x_fill_rectangle/mac_fill_rectangle
>
> 2) fill_rectangle_with_color: handles (1), but also with the temporary
> overriding of the GC that's done frequently for mac_erase_rectangle.
>
> 3) fill_rectangle_with_stipple: handles the temporary XFillStyle
> overriding of the GC.
>
> A third procedure wouldn't be inelegant in comparison to the
> alternatives, since otherwise I would need a set_context_stipple
> procedure to indicate stippling.
>
> Then mac_fill_rectangle can be assigned to fill_rectangle, and
> mac_erase_rectangle can be assigned to fill_rectangle_with_stipple.

Because of the reasons I mentioned above, I'd prefer
erase_rectangle to fill_rectangle_with_color/stipple.  Also, many
use cases in the Mac port involve erasure of multiple rectangles,
so erase_rectangles (multi-rectangle version like
XFillRectangles) would eliminate some overhead of setting up
colors and the clipping area.

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

Message #59 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: mituharu <at> math.s.chiba-u.ac.jp
To: "Alex Gramiak" <agrambot <at> gmail.com>,
 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Sat, 4 May 2019 13:00:14 +0900

> Because of the reasons I mentioned above, I'd prefer
> erase_rectangle to fill_rectangle_with_color/stipple.  Also, many
> use cases in the Mac port involve erasure of multiple rectangles,
> so erase_rectangles (multi-rectangle version like
> XFillRectangles) would eliminate some overhead of setting up
> colors and the clipping area.

Sorry, there weren't so many occurrence of multi-rectangle
filling/erasing.  Maybe I counted them wrong previously.

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

Message #62 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Sat, 04 May 2019 11:17:21 +0300

> From: Alex Gramiak <agrambot <at> gmail.com>
> Cc: 35468 <at> 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.

Message #65 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Alex Gramiak <agrambot <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35468 <at> debbugs.gnu.org, mituharu <at> math.s.chiba-u.ac.jp
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Sat, 04 May 2019 13:29:34 -0600

Eli Zaretskii <eliz <at> gnu.org> writes:

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

Ah, I see. I'll keep the setter and rename it to, say,
set_device_context_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?

I meant problem as in that it's "leaking" the internals a bit.

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

That's good; it will be easier to work with the image procedures in that
case.

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

On most platforms, yes, but the Cairo drawing uses s->img->cr_data
instead.

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

I was thinking that since AFAIU the Cairo drawing doesn't set
s->img->mask it wouldn't make sense, from an interface POV, to check it
directly. I suppose it doesn't really matter in that case, and it would
be faster to just check s->img->mask even if the backend doesn't use it.

Message #68 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: mituharu <at> math.s.chiba-u.ac.jp
To: "Alex Gramiak" <agrambot <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Sun, 5 May 2019 09:10:38 +0900

>> 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?
>
> I was thinking that since AFAIU the Cairo drawing doesn't set
> s->img->mask it wouldn't make sense, from an interface POV, to check it
> directly. I suppose it doesn't really matter in that case, and it would
> be faster to just check s->img->mask even if the backend doesn't use it.

IMO, image support for cairo is still premature and needs some
restructuring.  It does not support postprocessing (:conversion
ALGORITHM), mask removal (:mask nil), or image-mask-p.  Bitmaps
for some image format are not stored in the premultiplied alpha
format that cairo expects.

All of them are supported in the Mac port and I set dummy
s->img->mask there (not containing the actual mask bitmap data)
if the image has an alpha channel.  Probably setting a bitfield
for the existence of mask (alpha channel) when creating image
data would work as an alternative way.

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

Message #71 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: 35468 <at> debbugs.gnu.org, mituharu <at> math.s.chiba-u.ac.jp
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Sun, 05 May 2019 05:34:56 +0300

> From: Alex Gramiak <agrambot <at> gmail.com>
> Cc: 35468 <at> debbugs.gnu.org, mituharu <at> math.s.chiba-u.ac.jp
> Date: Sat, 04 May 2019 13:29:34 -0600
> 
> > 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.
> 
> Ah, I see. I'll keep the setter and rename it to, say,
> set_device_context_font.

"Device context" is a w32-specific concept, I wouldn't propagate it
into an abstraction.  set_font sounds much better to me.

> > I don't think I understand the difficulties, sorry.  Why is
> > s->img->mask a problem?
> 
> I meant problem as in that it's "leaking" the internals a bit.

Yes, but where and why do you need to leak it?

> > What details does glyph_has_image hide?  Is that just to test
> > s->img->pixmap?
> 
> On most platforms, yes, but the Cairo drawing uses s->img->cr_data
> instead.

A macro should do here, there's no need for an interface, IMO.

> >>         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?
> 
> I was thinking that since AFAIU the Cairo drawing doesn't set
> s->img->mask it wouldn't make sense, from an interface POV, to check it
> directly. I suppose it doesn't really matter in that case, and it would
> be faster to just check s->img->mask even if the backend doesn't use it.

Yes, I think so.

Message #74 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: mituharu <at> math.s.chiba-u.ac.jp
Cc: 35468 <at> debbugs.gnu.org, agrambot <at> gmail.com
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Sun, 05 May 2019 19:00:09 +0300

> Date: Sun, 5 May 2019 09:10:38 +0900
> From: mituharu <at> math.s.chiba-u.ac.jp
> Cc: "Eli Zaretskii" <eliz <at> gnu.org>,
>  35468 <at> debbugs.gnu.org
> 
> >> 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?
> >
> > I was thinking that since AFAIU the Cairo drawing doesn't set
> > s->img->mask it wouldn't make sense, from an interface POV, to check it
> > directly. I suppose it doesn't really matter in that case, and it would
> > be faster to just check s->img->mask even if the backend doesn't use it.
> 
> IMO, image support for cairo is still premature and needs some
> restructuring.  It does not support postprocessing (:conversion
> ALGORITHM), mask removal (:mask nil), or image-mask-p.  Bitmaps
> for some image format are not stored in the premultiplied alpha
> format that cairo expects.
> 
> All of them are supported in the Mac port and I set dummy
> s->img->mask there (not containing the actual mask bitmap data)
> if the image has an alpha channel.  Probably setting a bitfield
> for the existence of mask (alpha channel) when creating image
> data would work as an alternative way.

Would you like to propose the abstractions for this that are supposed
to be future-proof (if what Alex shows isn't already sufficiently so)?

TIA

Message #77 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Wed, 12 May 2021 16:43:17 +0200

Alex Gramiak <agrambot <at> gmail.com> writes:

> This merges x_draw_glyph_string and w32_draw_glyph_string together to
> remove duplicated code.
>
> I wanted to do it for NS as well, but it seems just a bit too different
> to make it easily work.

Skimming this thread, it seems like the consensus was that this patch is
an improvement, but that it'd be nice to cover NS as well.  But it seems
like the discussion mostly dissipated without reaching any conclusion.

Alex, have you done any further work on this, or is this abandoned?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #80 received at 35468 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: 35468 <at> debbugs.gnu.org
Subject: Re: bug#35468: [PATCH] Refactor draw_glyph_string on X and w32
Date: Thu, 22 Jul 2021 14:55:11 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Skimming this thread, it seems like the consensus was that this patch is
> an improvement, but that it'd be nice to cover NS as well.  But it seems
> like the discussion mostly dissipated without reaching any conclusion.
>
> Alex, have you done any further work on this, or is this abandoned?

This was two months ago, so I guess there's not going to be any further
progress here, and I'm closing this bug report.  If further progress can
be made, please respond to the debbugs address and we'll reopen.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.