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

[Ben Key]

Chong Yidong <address@hidden> 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.

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

</patch>