From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: A patch for enforcing double-width CJK character display Date: Tue, 29 Apr 2014 01:39:39 -0400 Message-ID: References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1398750002 31256 80.91.229.3 (29 Apr 2014 05:40:02 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 29 Apr 2014 05:40:02 +0000 (UTC) Cc: emacs-devel@gnu.org To: JunJie Nan Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Apr 29 07:39:56 2014 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Wf0lo-0003Kj-IZ for ged-emacs-devel@m.gmane.org; Tue, 29 Apr 2014 07:39:56 +0200 Original-Received: from localhost ([::1]:47222 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wf0lo-0007ov-73 for ged-emacs-devel@m.gmane.org; Tue, 29 Apr 2014 01:39:56 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:38707) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wf0lf-0007oZ-I9 for emacs-devel@gnu.org; Tue, 29 Apr 2014 01:39:53 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wf0lZ-0006YU-Jr for emacs-devel@gnu.org; Tue, 29 Apr 2014 01:39:47 -0400 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.181]:8228) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wf0lZ-0006YO-FO for emacs-devel@gnu.org; Tue, 29 Apr 2014 01:39:41 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ArYGAIDvNVNMCqOU/2dsb2JhbABZgwY7gw/APYEXF3SCJQEBAQECAScvIwULCzQSFBgNDRcTh3EI0hkXjiNXB4Q4BJoBizuDXYFqg0whgS0 X-IPAS-Result: ArYGAIDvNVNMCqOU/2dsb2JhbABZgwY7gw/APYEXF3SCJQEBAQECAScvIwULCzQSFBgNDRcTh3EI0hkXjiNXB4Q4BJoBizuDXYFqg0whgS0 X-IronPort-AV: E=Sophos;i="4.97,753,1389762000"; d="scan'208";a="59341341" Original-Received: from 76-10-163-148.dsl.teksavvy.com (HELO ceviche.home) ([76.10.163.148]) by ironport2-out.teksavvy.com with ESMTP/TLS/ADH-AES256-SHA; 29 Apr 2014 01:39:40 -0400 Original-Received: by ceviche.home (Postfix, from userid 20848) id DE5B466090; Tue, 29 Apr 2014 01:39:39 -0400 (EDT) In-Reply-To: (JunJie Nan's message of "Mon, 28 Apr 2014 13:35:03 +0800") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.4.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 206.248.154.181 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:171649 Archived-At: > 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