From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Ben Key Newsgroups: gmane.emacs.devel Subject: Re: Patch to fix the Bar Cursor Too Narrow problem on Mac OS X Date: Mon, 21 Feb 2011 15:15:46 -0600 Message-ID: References: <87d3mnj0o1.fsf@stupidchicken.com> <87wrktxo9t.fsf@stupidchicken.com> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=001485f85bb8333d84049cd15fa6 X-Trace: dough.gmane.org 1298323324 8345 80.91.229.12 (21 Feb 2011 21:22:04 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Mon, 21 Feb 2011 21:22:04 +0000 (UTC) To: Chong Yidong , Emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Feb 21 22:22:00 2011 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1PrdCi-0000gZ-4p for ged-emacs-devel@m.gmane.org; Mon, 21 Feb 2011 22:22:00 +0100 Original-Received: from localhost ([127.0.0.1]:54765 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PrdCh-0004Vz-DO for ged-emacs-devel@m.gmane.org; Mon, 21 Feb 2011 16:21:59 -0500 Original-Received: from [140.186.70.92] (port=43396 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Prd74-00013D-3H for Emacs-devel@gnu.org; Mon, 21 Feb 2011 16:16:11 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Prd73-0005Gg-1S for Emacs-devel@gnu.org; Mon, 21 Feb 2011 16:16:10 -0500 Original-Received: from mail-bw0-f51.google.com ([209.85.214.51]:47332) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Prd72-0005GX-PV for Emacs-devel@gnu.org; Mon, 21 Feb 2011 16:16:08 -0500 Original-Received: by bwz10 with SMTP id 10so2673107bwz.38 for ; Mon, 21 Feb 2011 13:16:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:from:date :message-id:subject:to:content-type; bh=gjNPA8ywa4CoufzhPUrmsLu8K3YA+WG++GlviPHPiLY=; b=YzWc1NnQ+KxmlSxHSRSmVS1aQ4OjE4krrdvWN+Z84Bqzq0Z7iUckMFzgdYntGKfqwN hjGIELVJyYVQhMnASIw4qbhkgWqw8P1GV6Kl6yx+gm52QOqTccy6Gj15UQ4BD1s/L6Rn OQ+p/UzApS/o4MrOXnFaxTv29UbH2MrlV2gQw= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :content-type; b=w5HiR5Uh2ELmnnQoVKG4RDQJ0E8zzNngawY904AI9cfNYwhdHud1WNtp1j2ES65gGD qWh2N3Ry6uYccoLuWxKWIDvPTN0T69MJYimD3jj9VRXG7OWRTdBnPz4ETbKEgS0hrae/ gZUdbnKPpdVBL8B8eSfk8jcH4cesj5OvizZ3Q= Original-Received: by 10.204.98.130 with SMTP id q2mr1767610bkn.31.1298322966133; Mon, 21 Feb 2011 13:16:06 -0800 (PST) Original-Received: by 10.204.25.81 with HTTP; Mon, 21 Feb 2011 13:15:46 -0800 (PST) In-Reply-To: <87wrktxo9t.fsf@stupidchicken.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 209.85.214.51 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:136345 Archived-At: --001485f85bb8333d84049cd15fa6 Content-Type: text/plain; charset=ISO-8859-1 Chong Yidong writes: > nsterm.m's usage of get_specified_cursor_type should follow the same > logic as the other terminals. If the NS terminal alone requires a > change to get_specified_cursor_type, that is a bug in nsterm.m; changing > get_specified_cursor_type only papers over this bug. In my opinion the real bug is get_specified_cursor_type was ever written so that there are code paths where the function returns without initializing an output parameter that is used by other functions. It is this bug that the original author of nsterm.m noticed and decided to work around by ignoring the user specified value for cursor width with the line s.size.width = min (cursor_width, 2); //FIXME(see above) The problem is not that "the NS terminal alone requires a change to get_specified_cursor_type." The problem is that get_specified_cursor_type has a bug that I am attempting to fix. My change to xdisp.c does not "paper over" anything. Instead, it makes an attempt to fix a bug that should have never been introduced in the first place. I do not know why you are so opposed to my attempt to fix this bug in get_specified_cursor_type. If it is the use of the somewhat arbitrary value of 2 for the default value of width, then perhaps you will find this patch more to your liking. === modified file 'src/nsterm.m' --- src/nsterm.m 2011-02-17 10:19:29 +0000 +++ src/nsterm.m 2011-02-21 19:22:41 +0000 @@ -2232,9 +2232,6 @@ /* -------------------------------------------------------------------------- External call (RIF): draw cursor (modeled after x_draw_window_cursor - FIXME: cursor_width is effectively bogus -- it sometimes gets set - in xdisp.c set_frame_cursor_types, sometimes left uninitialized; - DON'T USE IT (no other terms do) -------------------------------------------------------------------------- */ { NSRect r, s; @@ -2278,6 +2275,9 @@ get_phys_cursor_geometry (w, glyph_row, phys_cursor_glyph, &fx, &fy, &h); + if (cursor_type == BAR_CURSOR) + w->phys_cursor_width = cursor_width; + r.origin.x = fx, r.origin.y = fy; r.size.height = h; r.size.width = w->phys_cursor_width; @@ -2335,7 +2335,6 @@ break; case BAR_CURSOR: s = r; - s.size.width = min (cursor_width, 2); //FIXME(see above) /* If the character under cursor is R2L, draw the bar cursor on the right of its glyph, rather than on the left. */ === modified file 'src/xdisp.c' --- src/xdisp.c 2011-02-18 15:11:10 +0000 +++ src/xdisp.c 2011-02-21 20:37:20 +0000 @@ -23200,6 +23200,20 @@ get_specified_cursor_type (Lisp_Object arg, int *width) { enum text_cursor_kinds type; + int default_width = 2; + struct frame *f; + + /* + Initialize width to a reasonable default value here to ensure that there can + never be a case in which the function exits without width being initialized. + */ + if (NULL != selected_frame) + { + f = XFRAME (selected_frame); + if (FRAME_WINDOW_P(f)) + default_width = FRAME_COLUMN_WIDTH (f); + } + *width = default_width; if (NILP (arg)) return NO_CURSOR; This new patch sets the default value of width to the frame column width. I got the idea from get_phys_cursor_geometry which also uses the frame column width as part of the process of initializing w->phys_cursor_width in some cases. > Does it do the right thing even if you leave out the change to xdisp.c? In all the cases I have tested, my patch does work correctly without the change to xdisp.c. But this does not mean that there will not be cases in which leaving out my fix to xdisp.c will result in undesirable results for some users. The fact is that without my fix to xdisp.c, there are cases in which ns_draw_window_cursor is called with a bogus value for cursor_width due to the get_specified_cursor_type not initializing width bug. This bug should be fixed! Otherwise, ns_draw_window_cursor can not depend on cursor_width always being properly initialized and there is no truly reliable way to fix the Bar Cursor Too Narrow problem. If you have a valid reason for not fixing this bug in get_specified_cursor_type please let me know. --001485f85bb8333d84049cd15fa6 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Chong Yidong writes:

> nsterm.m's usage of g= et_specified_cursor_type should follow the same
> logic as the other terminals. =A0If the NS terminal alone requires a > change to get_specified_cursor_type, that is a bug in nsterm.m; changi= ng
> get_specified_cursor_type only papers over this bug.

In my opinion the real bug is get_specified_cursor_type was e= ver written so that there are code paths where the function returns without= initializing an output parameter that is used by other functions.=A0 It is= this bug that the original author of nsterm.m noticed and decided to work = around by ignoring the user specified value for cursor width with the line<= br> =A0=A0=A0=A0=A0 s.size.width =3D min (cursor_width, 2); //FIXME(see above)<= br>
The problem is not that "the NS termina= l alone requires a change to get_specified_cursor_type."=A0 The proble= m is that get_specified_cursor_type has a bug that = I am attempting to fix.=A0 My change to xdisp.c doe= s not "paper over" anything.=A0 Instead, it makes an attempt to f= ix a bug that should have never been introduced in the first place.<= font size=3D"4">

I do not know why you are so opposed to my attempt to fix this bug in <= /font>get_specified_cursor_type.=A0 If it is the use of th= e somewhat arbitrary value of 2 for the default value of width, then perhap= s you will find this patch more to your liking.

<patch>
=3D=3D=3D modified file 'src/nsterm.m'
--- = src/nsterm.m=A0=A0=A0 2011-02-17 10:19:29 +0000
+++ src/nsterm.m=A0=A0= =A0 2011-02-21 19:22:41 +0000
@@ -2232,9 +2232,6 @@
=A0/* -----------= ---------------------------------------------------------------
=A0=A0=A0=A0=A0 External call (RIF): draw cursor
=A0=A0=A0=A0=A0 (modele= d after x_draw_window_cursor
-=A0=A0=A0=A0 FIXME: cursor_width is effect= ively bogus -- it sometimes gets set
-=A0=A0=A0=A0 in xdisp.c set_frame_= cursor_types, sometimes left uninitialized;
-=A0=A0=A0=A0 DON'T USE IT (no other terms do)
=A0=A0=A0 -----------= --------------------------------------------------------------- */
=A0{<= br>=A0=A0 NSRect r, s;
@@ -2278,6 +2275,9 @@
=A0
=A0=A0 get_phys_c= ursor_geometry (w, glyph_row, phys_cursor_glyph, &fx, &fy, &h);=
=A0
+=A0 if (cursor_type =3D=3D BAR_CURSOR)
+=A0=A0=A0 w->phys_cur= sor_width =3D cursor_width;
+
=A0=A0 r.origin.x =3D fx, r.origin.y = =3D fy;
=A0=A0 r.size.height =3D h;
=A0=A0 r.size.width =3D w->phy= s_cursor_width;
@@ -2335,7 +2335,6 @@
=A0=A0=A0=A0=A0=A0 break;
=A0=A0=A0=A0 case BAR_CURSOR:
=A0=A0=A0=A0= =A0=A0 s =3D r;
-=A0=A0=A0=A0=A0 s.size.width =3D min (cursor_width, 2);= //FIXME(see above)
=A0
=A0=A0=A0=A0=A0=A0 /* If the character under = cursor is R2L, draw the bar cursor
=A0=A0=A0=A0=A0=A0=A0=A0=A0 on the ri= ght of its glyph, rather than on the left.=A0 */

=3D=3D=3D modified file 'src/xdisp.c'
--- src/xdisp.c=A0=A0= =A0 2011-02-18 15:11:10 +0000
+++ src/xdisp.c=A0=A0=A0 2011-02-21 20:37:= 20 +0000
@@ -23200,6 +23200,20 @@
=A0get_specified_cursor_type (Lisp_= Object arg, int *width)
=A0{
=A0=A0 enum text_cursor_kinds type;
+=A0 int default_width =3D 2= ;
+=A0 struct frame *f;
+
+=A0 /*
+=A0 Initialize width to a r= easonable default value here to ensure that there can
+=A0 never be a ca= se in which the function exits without width being initialized.
+=A0 */
+=A0 if (NULL !=3D selected_frame)
+=A0 {
+=A0=A0=A0 f =3D= XFRAME (selected_frame);
+=A0=A0=A0 if (FRAME_WINDOW_P(f))
+=A0=A0= =A0=A0=A0 default_width =3D FRAME_COLUMN_WIDTH (f);
+=A0 }
+=A0 *widt= h =3D default_width;
=A0
=A0=A0 if (NILP (arg))
=A0=A0=A0=A0 return NO_CURSOR;

</patch>

This new patch = sets the default value of width to the frame column width.=A0 I got the ide= a from get_phys_cursor_geometry which also uses the frame column width as p= art of the process of initializing w->phys_cursor_width in some cases.
> Does it do the right thing even if you leave out the change to xdi= sp.c?

In all the cases I have tested, my patch does work correctly w= ithout the change to xdisp.c.=A0 But this does not mean that there will not= be cases in which leaving out my fix to xdisp.c will result in undesirable= results for some users.=A0 The fact is that without my fix to xdisp.c, the= re are cases in which ns_draw_window_cursor is called with a bogus value fo= r cursor_width due to the g= et_specified_cursor_type not initializing width bug.=A0 This bug should be = fixed!=A0 Otherwise, ns_draw_window_cursor can not = depend on cursor_width always being properly initia= lized and there is no truly reliable way to fix the Bar Cursor Too Narrow p= roblem.

If you have a valid reason for not fixing this bug in get_specified_cursor_type please let me know.

 --001485f85bb8333d84049cd15fa6--