Re: A patch for enforcing double-width CJK character display

[Stefan Monnier <monnier@iro.umontreal.ca>]

> Do not know why the patch is still not installed, although from the
> discussion thread nobody oppose it indeed.

Sorry, I can't remember seeing your patch before, I must have overlooked
it.  Is it related to http://debbugs.gnu.org/10179?

This said, I don't know much of anything about the font drivers, so
I can't really comment on the substance of the code, but while waiting
for someone like Jan or Handa to look at it, I can give you some trivial
cosmetic recommendations (most of them documented in the "GNU Coding
Standards").

If you resubmit a new patch, I recommend you send it to
bug-gnu-emacs@gnu.org (so that it gets a tracking number) or directly to
10179@debbugs.gnu.org if it's related to that bug.

> +  struct frame *frame; /* hold frame ptr, cjk double width fix need it */

Please capitalize and punctuate your comments.

> +  int is_cjk; /* Flag to tell if it is CJK font or not. */

Thanks for capitalizing and punctuating this comment, this one is good.
We usually prefer to two spaces after the final ".", in case you feel
like polishing it yet a bit more.

> +   because Korean fonts may not have any Chinese characters at all.
> +   codes from xterm.*/

Here we do need spacing (ideally two spaces) between "." and "*/"
I don't understand exactly what is meant by "codes from xterm".
Maybe that should be "Code inspired by similar logic in XTerm"?

> +static int
> +xftfont_is_cjk_font(struct xftfont_info *xftfont_info)

Always put a space before the open parenthesis.

> +{
> +  if(XftCharExists(xftfont_info->display, xftfont_info->xftfont, 0x4E00) ||
> +      XftCharExists(xftfont_info->display, xftfont_info->xftfont, 0xAC00))

Same here.  Additionally, please but the line before "||" rather than after.

> +    return 1;
> +  return 0; 

Please make the return type "bool", then.  And use "true" and "false"
rather than 1 and 0.  Also, you can apply eta-reduction to the above
code and just write

    return (XftCharExists (xftfont_info->display, xftfont_info->xftfont, 0x4E00)
            || XftCharExists (xftfont_info->display, xftfont_info->xftfont, 0xAC00));

[ Tho that would step over the 80 columns limit, so you may then want to
  introduce a local var to hold xftfont_info->display, maybe.  ]

> +  if(half_width_cjk)
> +    *half_width_cjk = 0;

I think half_width_cjk should be a pointer to "bool" and we should use
"false" here.

> +  if( default_width == 0 || /* something wrong */

Please don't put a space after an open paren (but do put one before).

> +  if( char_width < default_width) { 

The "{" should be on a line of its own.

> +  } else /* get the padding, all cjk symbols is DOUBLE width */

And the "}" should also be on its own line.

> +  xftfont_info->is_cjk = xftfont_is_cjk_font(xftfont_info);

`is_cjk' should be a `bool' field.


        Stefan