From mboxrd@z Thu Jan  1 00:00:00 1970
Path: news.gmane.org!not-for-mail
From: Stefan Monnier <monnier@iro.umontreal.ca>
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: <jwvwqe8r9w2.fsf-monnier+emacs@gnu.org>
References: <CA+Gegu_J9EFD6WhiXvA8C+5EA2BwvpRD0Y_HLe_Lgb35kBkM0g@mail.gmail.com>
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 <nanjunjie@gmail.com>
Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Apr 29 07:39:56 2014
Return-path: <emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org>
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 <emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org>)
	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 <emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org>)
	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 <monnier@iro.umontreal.ca>) 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 <monnier@iro.umontreal.ca>) 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 <monnier@iro.umontreal.ca>) 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: <CA+Gegu_J9EFD6WhiXvA8C+5EA2BwvpRD0Y_HLe_Lgb35kBkM0g@mail.gmail.com>
	(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." <emacs-devel.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/emacs-devel>,
	<mailto:emacs-devel-request@gnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/archive/html/emacs-devel>
List-Post: <mailto:emacs-devel@gnu.org>
List-Help: <mailto:emacs-devel-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/emacs-devel>,
	<mailto:emacs-devel-request@gnu.org?subject=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: <http://permalink.gmane.org/gmane.emacs.devel/171649>

> 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