Re: Status of MAC/W32/X consolidation -- first major patch committed.

[Kim F. Storm]

Jason Rumney <address@hidden> writes:

> address@hidden (Kim F. Storm) writes:
> 
> > I swapped the names of "w32_get_glyph_overhangs" and
> > "x_get_glyph_overhangs" to make the X and W32 code less different...
> 
> That could be dangerous. 

I agree in general, but not in this case... read on.


>                          Usually if I changed the x_ prefix to w32_,
> I did so because the interface or effect of the function was
> different. In this case, the interface was as follows.
> 
> static void w32_get_glyph_overhangs P_ ((HDC hdc, struct glyph *,
>                                           struct frame *,
>                                           int *, int *));



Yes, I understand that.  However, in this specific case, I really
didn't see any reason for a w32-specific version, as the change to
the interface (adding `hdc' arg) seemed unnecessary -- as the function
didn't reference `hdc' at all.  Here is the original version:

static void
w32_get_glyph_overhangs (hdc, glyph, f, left, right)
     HDC hdc;
     struct glyph *glyph;
     struct frame *f;
     int *left, *right;
{
  *left = *right = 0;

  if (glyph->type == CHAR_GLYPH)
    {
      XFontStruct *font;
      struct face *face;
      wchar_t char2b;
      XCharStruct *pcm;

      face = x_get_glyph_face_and_encoding (f, glyph, &char2b, NULL);
      font = face->font;

      if (font
          && (pcm = w32_per_char_metric (font, &char2b,
                                         glyph->w32_font_type)))
        {
          if (pcm->rbearing > pcm->width)
            *right = pcm->rbearing - pcm->width;
          if (pcm->lbearing < 0)
            *left = -pcm->lbearing;
        }
    }
}

> 
>  x_get_glyph_overhangs was probably a wrapper to present the same
>  interface as the x equivalent, but less efficient due to needing to
>  get the hdc on every call (it is called for every glyph displayed, so
>  that makes a big difference).
> 

Again, since the original w32_get_glyph_overhangs didn't actually
reference the hdc argument, but the original x_get_glyph_overhangs
(and the new w32_get_glyph_overhangs) still did the extra work of
getting the hdc, I thought that it might be required to do so
due to side-effects of the get_frame_dc.

So I believe my changes are safe!

We may further simplify the code by getting rid of the new
w32_get_glyph_overhangs (if the assumption about required side-effects
is false).  Then just discard w32_get_glyph_overhangs and use
x_get_glyph_overhangs directly in its place.


BTW, this leads me to another strange thing about the merged version
of this code (based on the code from xterm.c):

void
x_get_glyph_overhangs (glyph, f, left, right)
     struct glyph *glyph;
     struct frame *f;
     int *left, *right;
{
  *left = *right = 0;

  if (glyph->type == CHAR_GLYPH)
    {
      XFontStruct *font;
      struct face *face;
      struct font_info *font_info;
      XChar2b char2b;
      XCharStruct *pcm;

      face = get_glyph_face_and_encoding (f, glyph, &char2b, NULL);
      font = face->font;
      font_info = FONT_INFO_FROM_ID (f, face->font_info_id);
      if (font  /* ++KFS: Should this be font_info ?  */
          && (pcm = rif->per_char_metric (font, &char2b, glyph->font_type)))
        {
          if (pcm->rbearing > pcm->width)
            *right = pcm->rbearing - pcm->width;
          if (pcm->lbearing < 0)
            *left = -pcm->lbearing;
        }
    }
}


Notice that the code finds the `font_info' for the font, but doesn't actually
use that for anything.  I have a suspicion that the test that reads
        if (font && ...)
was really meant to read
        if (font_info && ...)

I see three indications why this may be true:
1) Why find the font_info (and not using it) ?
2) Is font (== face->font) ever NULL ?  (sounds strange to me)
3) Other similar parts of the code which call per_char_metric
   actually check the font_info rather than the font.

If anyone can sched some light on this, it would be appreciated!

-- 
Kim F. Storm <address@hidden> http://www.cua.dk