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

["Jan D." <jan.h.d@swipnet.se>]

Hello.

Stefan Monnier skrev 2014-04-29 07:39:
>> 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?

The discussion was in 2012, here is the previous explanation:

http://lists.gnu.org/archive/html/emacs-devel/2012-04/msg00370.html

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

As I don't use CJK, I'm not qualified to comment more than on code 
style, and you already did that.

I would however rather see that frame was not part of the struct 
xftfont_info.  It is only used in xftfont_get_cjk_padding.  You can call 
that function at font open time, and store the value in a cjk_padding 
member in xftfont_info.

	Jan D.

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