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

[Ben Key]

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.