From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: "Jan D." Newsgroups: gmane.emacs.devel Subject: Re: A patch for enforcing double-width CJK character display Date: Tue, 29 Apr 2014 08:36:57 +0200 Message-ID: <535F4889.3070302@swipnet.se> References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1398753439 11281 80.91.229.3 (29 Apr 2014 06:37:19 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 29 Apr 2014 06:37:19 +0000 (UTC) Cc: JunJie Nan , emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Apr 29 08:37:13 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 1Wf1fD-0005ia-Vj for ged-emacs-devel@m.gmane.org; Tue, 29 Apr 2014 08:37:12 +0200 Original-Received: from localhost ([::1]:47426 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wf1fD-00006t-9H for ged-emacs-devel@m.gmane.org; Tue, 29 Apr 2014 02:37:11 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:48883) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wf1f3-00005f-AZ for emacs-devel@gnu.org; Tue, 29 Apr 2014 02:37:08 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wf1ew-0000Mg-La for emacs-devel@gnu.org; Tue, 29 Apr 2014 02:37:01 -0400 Original-Received: from mailfe08.swip.net ([212.247.154.225]:40723 helo=swip.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wf1ew-0000Lp-AT for emacs-devel@gnu.org; Tue, 29 Apr 2014 02:36:54 -0400 X-T2-Spam-Status: No, hits=0.8 required=5.0 tests=BAYES_50 Original-Received: from hosdjarv.se (account mj138573@tele2.se [46.59.42.57] verified) by mailfe08.swip.net (CommuniGate Pro SMTP 5.4.4) with ESMTPA id 497960423; Tue, 29 Apr 2014 08:36:49 +0200 Original-Received: from jdvpro.hq.ismobile.com (unknown [176.57.193.190]) (Authenticated sender: jhd) by hosdjarv.se (Postfix) with ESMTPSA id ED95A1A039C; Tue, 29 Apr 2014 06:36:48 +0000 (UTC) User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:17.0) Gecko/20130328 Thunderbird/17.0.5 In-Reply-To: X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 212.247.154.225 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:171650 Archived-At: 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 >