Re: Patch to fix the Bar Cursor Too Narrow problem on Mac OS X

[Ben Key <bkey76@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4185 bytes --]

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.

<patch>
=== 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;

</patch>

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.

[-- Attachment #2: Type: text/html, Size: 4879 bytes --]