From: "Jan D." <jan.h.d@swipnet.se>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: JunJie Nan <nanjunjie@gmail.com>, emacs-devel@gnu.org
Subject: Re: A patch for enforcing double-width CJK character display
Date: Tue, 29 Apr 2014 08:36:57 +0200 [thread overview]
Message-ID: <535F4889.3070302@swipnet.se> (raw)
In-Reply-To: <jwvwqe8r9w2.fsf-monnier+emacs@gnu.org>
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
>
next prev parent reply other threads:[~2014-04-29 6:36 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-04-28 5:35 A patch for enforcing double-width CJK character display JunJie Nan
2014-04-29 5:39 ` Stefan Monnier
2014-04-29 6:36 ` Jan D. [this message]
2014-04-29 8:16 ` Thien-Thi Nguyen
2014-04-29 20:41 ` Liang Wang
-- strict thread matches above, loose matches on Subject: below --
2014-10-04 3:26 Feng Shu
2014-04-30 2:00 Hui Liu
2014-04-30 17:08 ` Liang Wang
[not found] <4F85A138.6090900@i-soft.com.cn>
2012-04-11 15:48 ` Kan-Ru Chen
2012-04-11 16:16 ` 黄建忠
2012-04-12 8:56 ` 黄建忠
2012-04-12 9:53 ` Eli Zaretskii
2012-04-12 11:18 ` 黄建忠
2012-04-12 14:27 ` Eli Zaretskii
2012-04-12 17:56 ` 黄建忠
2012-04-12 20:33 ` Stefan Monnier
[not found] ` <4F8782C8.2030005@i-soft.com.cn>
2012-04-13 11:42 ` 黄建忠
2012-04-13 12:03 ` 黄建忠
2012-04-13 13:27 ` Stefan Monnier
2012-04-15 5:10 ` Miles Bader
2012-04-15 13:27 ` 黄建忠
2012-04-15 16:08 ` William Xu
2012-04-15 22:19 ` Miles Bader
2012-04-16 0:51 ` 黄建忠
2012-04-16 5:27 ` Miles Bader
2012-04-16 5:40 ` 黄建忠
2012-04-16 6:37 ` 黄建忠
2012-04-16 9:21 ` 黄建忠
2012-04-17 2:16 ` 黄建忠
2012-04-17 0:13 ` Miles Bader
2012-04-17 0:39 ` Miles Bader
2012-04-17 2:00 ` 黄建忠
2012-04-17 2:30 ` Miles Bader
2012-04-17 3:00 ` 黄建忠
2012-04-17 4:08 ` Miles Bader
2012-04-17 4:56 ` Werner LEMBERG
2012-04-17 5:02 ` 黄建忠
2012-04-17 6:33 ` Miles Bader
2012-04-17 7:03 ` Werner LEMBERG
2012-04-17 5:52 ` Miles Bader
2012-04-17 6:10 ` 黄建忠
2012-04-17 7:02 ` Miles Bader
2012-04-17 8:06 ` Werner LEMBERG
2012-04-17 8:25 ` Miles Bader
2012-04-17 9:06 ` Werner LEMBERG
2012-04-17 8:51 ` 黄建忠
2012-04-17 6:45 ` Werner LEMBERG
2012-04-17 9:07 ` James Cloos
2012-04-17 9:27 ` 黄建忠
2012-04-17 1:47 ` 黄建忠
2012-04-18 6:54 ` Kenichi Handa
2012-04-18 8:13 ` 黄建忠
2012-04-18 13:58 ` Miles Bader
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=535F4889.3070302@swipnet.se \
--to=jan.h.d@swipnet.se \
--cc=emacs-devel@gnu.org \
--cc=monnier@iro.umontreal.ca \
--cc=nanjunjie@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.