bug#35468: [PATCH] Refactor draw_glyph_string on X and w32

[Eli Zaretskii]

> From: Alex Gramiak <agrambot@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.