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: Sat, 19 Feb 2011 19:23:55 -0600 Message-ID: References: <87d3mnj0o1.fsf@stupidchicken.com> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=001636c5b5aef872fe049cac9aec X-Trace: dough.gmane.org 1298165155 2975 80.91.229.12 (20 Feb 2011 01:25:55 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Sun, 20 Feb 2011 01:25:55 +0000 (UTC) To: Chong Yidong , Emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Feb 20 02:25:49 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 1Pqy2D-0007ca-0J for ged-emacs-devel@m.gmane.org; Sun, 20 Feb 2011 02:25:49 +0100 Original-Received: from localhost ([127.0.0.1]:40090 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pqy2B-0002w9-H1 for ged-emacs-devel@m.gmane.org; Sat, 19 Feb 2011 20:24:23 -0500 Original-Received: from [140.186.70.92] (port=60531 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pqy26-0002vu-Is for Emacs-devel@gnu.org; Sat, 19 Feb 2011 20:24:20 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pqy25-0007Vx-1e for Emacs-devel@gnu.org; Sat, 19 Feb 2011 20:24:18 -0500 Original-Received: from mail-bw0-f41.google.com ([209.85.214.41]:39924) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pqy24-0007Vs-Mw for Emacs-devel@gnu.org; Sat, 19 Feb 2011 20:24:17 -0500 Original-Received: by bwz17 with SMTP id 17so497388bwz.0 for ; Sat, 19 Feb 2011 17:24:15 -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=plVEFAQLLtaFz13vTuY+Dew4mxImzcyYji/LxrxFlZI=; b=STc1rJwv/+N4iZl28b1FuVnKRYlMHNLPRLKNF52nD/KqfBvnm31Z6tJxLZW4MWAmKv 7EIypX9Zj9UR1H1Q7+kyxbX9UXu6TQYPf0KuiD8dDonEE6jlk8hKGKi2luZxsiY9b/NY 6W3OgYtrfTWqDf/g/VyoE4K+BOYmB4BJFq/kg= 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=LxhGmTIg+LHFsirHbacen07OnKsd69ep6jnKiUPSn6kLB84l7t1D6IVJBOoutDCmAe NxvfybqRjCjAd58GPN0CUFta2NCumi7Nkk7/ZI8Kv5XIyPfSyjR94kJpM8yL3285N3Dz W8/wCi08P26sGn5AFTJ5n5UmITT3PQJMmZOd0= Original-Received: by 10.204.65.135 with SMTP id j7mr2197273bki.85.1298165055123; Sat, 19 Feb 2011 17:24:15 -0800 (PST) Original-Received: by 10.204.25.81 with HTTP; Sat, 19 Feb 2011 17:23:55 -0800 (PST) In-Reply-To: <87d3mnj0o1.fsf@stupidchicken.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 209.85.214.41 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:136255 Archived-At: --001636c5b5aef872fe049cac9aec Content-Type: text/plain; charset=ISO-8859-1 Chong Yidong writes: > Could you try the following patch instead? > > *** src/nsterm.m 2011-02-17 10:19:29 +0000 > --- src/nsterm.m 2011-02-19 19:13:36 +0000 > *************** > *** 2264,2269 **** > --- 2264,2271 ---- > w->phys_cursor_width = 0; > return; > } > + else if (cursor_type == BAR_CURSOR) > + w->phys_cursor_width = width; > > if ((phys_cursor_glyph = get_phys_cursor_glyph (w)) == NULL) > { > For one, the change to src/xdisp.c is critical. Otherwise there would be places in get_specified_cursor_type where width is not initialized before the function exits and the FIXME comments in ns_draw_window_cursor would still be valid. For example, just two lines below where I added the line "*width = 2;" is a return statement. If the line initializing width were not added and the arg parameter was equal to nil, the function get_specified_cursor_type would exit without width being initialized. There are many other examples of return without first initializing width in this function. Granted, as far as I know, the code paths where width is not properly initialized are for cursor types other than the bar cursor, but I am not absolutely certain of that. Even if the code paths where get_specified_cursor_type exits without initializing width are for cursor types other than the bar cursor, the line initializing width should still be added. In general, it is a very bad idea for a function that has an output parameter to have any code paths that cause the function to exit without first initializing that output parameter, even if that output parameter is not currently used in any currently possible scenarios. Code changes over time and a scenario where that output parameter is used could easily be added in the future. If that new scenario just assumes the output parameter has properly been initialized, a new bug is introduced. Secondly, I am not certain what you mean by using the following line. + w->phys_cursor_width = width; There is no variable named 'width' in the function ns_draw_window_cursor. If you actually mean the following + w->phys_cursor_width = cursor_width; then this would have no result unless you also removed the line s.size.width = min (cursor_width, 2); //FIXME(see above) that occurs latter in the function. It is this line that causes the problem of the user specified cursor width being ignored. However, your suggested change would not work even if you also removed that line. This is because the function get_phys_cursor_geometry is called after your proposed new line. Unfortunately, get_phys_cursor_geometry, which is found in src/xdisp.c, modifies this value as follows. w->phys_cursor_width = wd; In my tests with your proposed changes and the removal of the "s.size.width = min (cursor_width, 2); //FIXME(see above)" line, cursor_width was equal to 3 but w->phys_cursor_width ended up being set to 11 or higher due to the line I pointed out in get_phys_cursor_geometry. The end result is that the cursor ended up being much wider than the user specified width. My change of simply changing the line s.size.width = min (cursor_width, 2); //FIXME(see above) to s.size.width = min (cursor_width, w->phys_cursor_width); does not have this problem. If you placed the lines if (cursor_type == BAR_CURSOR) w->phys_cursor_width = width; after the call to get_phys_cursor_geometry and removed the line s.size.width = min (cursor_width, 2); //FIXME(see above) it would have the same effect as my original proposed change and cause the user specified cursor width to be honored. For example, the following patch would also work. === modified file 'src/nsterm.m' --- src/nsterm.m 2011-02-17 10:19:29 +0000 +++ src/nsterm.m 2011-02-20 01:03:34 +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-20 01:04:09 +0000 @@ -23200,6 +23200,12 @@ get_specified_cursor_type (Lisp_Object arg, int *width) { enum text_cursor_kinds type; + + /* + 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. + */ + *width = 2; if (NILP (arg)) return NO_CURSOR; --001636c5b5aef872fe049cac9aec Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Chong Yidong <cyd@stupidchicken.com> writes:


Could you try the following patch instead?

*** src/nsterm.m =A0 =A0 =A0 =A02011-02-17 10:19:29 +0000
--- src/nsterm.m =A0 =A0 =A0 =A02011-02-19 19:13:36 +0000
***************
*** 2264,2269 ****
--- 2264,2271 ----
=A0 =A0 =A0 =A0w->phys_cursor_width =3D 0;
=A0 =A0 =A0 =A0return;
=A0 =A0 =A0}
+ =A0 else if (cursor_type =3D=3D BAR_CURSOR)
+ =A0 =A0 w->phys_cursor_width =3D width;

=A0 =A0if ((phys_cursor_glyph =3D get_phys_cursor_glyph (w)) =3D=3D NULL)<= br> =A0 =A0 =A0{

For one, the change to src/xdisp.c is critical.=A0 Otherwise there would be places in = get_specified_cursor_type where width is not initia= lized before the function exits and the FIXME comments in ns_draw_window_cursor would still be valid.=A0 For example, just two= lines below where I added the line "*width = =3D 2;" is a return statement.=A0 If the line initializing width were = not added and the arg parameter was equal to nil, the function get_specified_cursor_type would exit without width being initia= lized.=A0 There are many other examples of return without first initializin= g width in this function.=A0 Granted, as far as I k= now, the code paths where width is not properly initialized are for cursor = types other than the bar cursor, but I am not absolutely certain of that.= =A0 Even if the code paths where get_specified_curs= or_type exits without initializing width are for cu= rsor types other than the bar cursor, the line initializing width should st= ill be added.=A0 In general, it is a very bad idea for a function that has = an output parameter to have any code paths that cause the function to exit = without first initializing that output parameter, even if that output param= eter is not currently used in any currently possible scenarios.=A0 Code cha= nges over time and a scenario where that output parameter is used could eas= ily be added in the future.=A0 If that new scenario just assumes the output= parameter has properly been initialized, a new bug is introduced.

Secondly, I am not certain what you mean by using the following line.+ =A0 =A0 w->phys_cursor_width =3D width;
There is no variable name= d 'width' in the function ns_draw_window_cursor.=A0 If you actually= mean the following
+ =A0 =A0 w->phys_cursor_width =3D cursor_width;=
then this would have no result unless you also removed the line
=A0= =A0=A0=A0=A0 s.size.width =3D min (cursor_width, 2); //FIXME(see above)
= that occurs latter in the function.=A0 It is this line that causes the prob= lem of the user specified cursor width being ignored.

However, your suggested change would not work even if you also removed = that line.=A0 This is because the function get_phys_cursor_geometry is call= ed after your proposed new line.=A0 Unfortunately, get_phys_cursor_geometry= , which is found in src/xdisp.c, modifies this value as follows.
=A0 w->phys_cursor_width =3D wd;

In my tests with your proposed c= hanges and the removal of the "s.size.width = =3D min (cursor_width, 2); //FIXME(see above)" line, cursor_width was equal to 3 but w->phys= _cursor_width ended up being set to 11 or higher due to the line I pointed = out in get_phys_cursor_geometry.=A0 The end result = is that the cursor ended up being much wider than the user specified width.=

My change of simply changing the line
=A0=A0= =A0=A0=A0 s.size.width =3D min (cursor_width, 2); //FIXME(see above)
to<= br>=A0=A0=A0=A0=A0 s.size.width =3D min (cursor_wid= th, w->phys_cursor_width);
does not have this problem.

If you placed the lines
=A0 if (curso= r_type =3D=3D BAR_CURSOR)
=A0=A0=A0 w->phys_cursor_width =3D width;after the call to get_phys_cursor_geometry and re= moved the line
=A0=A0=A0=A0=A0 s.size.width =3D min (cursor_width, 2); //= FIXME(see above)
it would have the same effect as my original proposed c= hange and cause the user specified cursor width to be honored.=A0 For examp= le, the following patch would also work.

<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-20 01:03:34 +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-20 01:04:= 09 +0000
@@ -23200,6 +23200,12 @@
=A0get_specified_cursor_type (Lisp_= Object arg, int *width)
=A0{
=A0=A0 enum text_cursor_kinds type;
+=A0
+=A0 /*
+=A0 Ini= tialize width to a reasonable default value here to ensure that there can+=A0 never be a case in which the function exits without width being init= ialized.
+=A0 */
+=A0 *width =3D 2;
=A0
=A0=A0 if (NILP (arg))
=A0=A0=A0= =A0 return NO_CURSOR;

</patch>


--001636c5b5aef872fe049cac9aec--