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

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

[Ben Key]

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

Hello,

The first thing I noticed when I began using Emacs on Mac OS X was that when
Emacs is configured to use a bar cursor the cursor is difficult to see
because it is too narrow, even when the width of the bar cursor is specified
by using the (bar . WIDTH) syntax when setting the value of the cursor-type
variable.  The following patch fixes this problem.

There are three changes in this patch.

   1. The function get_specified_cursor_type in src/xdisp.c has been
   modified so that the width output parameter is always initialized.
   2. FIXME comments that are no longer valid have been removed from
the function
   ns_draw_window_cursor in nsterm.m.  The comments are no longer valid because
   they discuss the cursor_width may not be initialized problem that is
   fixed by change 1.
   3. In the BAR_CURSOR case, s.size.width is set equal to "min
   (cursor_width, w->phys_cursor_width)."  Previously it was set equal to "min
   (cursor_width, 2)" which effectively caused the user specified value for the
   cursor width to be ignored.


<patch>
=== modified file 'src/nsterm.m'
--- src/nsterm.m    2011-02-17 10:19:29 +0000
+++ src/nsterm.m    2011-02-18 03:43:28 +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;
@@ -2335,7 +2332,7 @@
       break;
     case BAR_CURSOR:
       s = r;
-      s.size.width = min (cursor_width, 2); //FIXME(see above)
+      s.size.width = min (cursor_width, w->phys_cursor_width);

       /* 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-16 18:39:46 +0000
+++ src/xdisp.c    2011-02-18 03:45:19 +0000
@@ -23201,6 +23201,12 @@
 {
   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>

I tested these changes against revision 103325 on Mac OS X.

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

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

[Chong Yidong]

Ben Key <bkey76@gmail.com> writes:

> The first thing I noticed when I began using Emacs on Mac OS X was
> that when Emacs is configured to use a bar cursor the cursor is
> difficult to see because it is too narrow, even when the width of the
> bar cursor is specified by using the (bar . WIDTH) syntax when setting
> the value of the cursor-type variable.  The following patch fixes this
> problem.

Thanks.  I'll commit your fix to the comments for ns_draw_window_cursor
shortly, but I don't think the other changes are quite right.

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)
      {

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

[Ben Key]

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

Chong Yidong <cyd@stupidchicken.com> 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>

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

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

[Chong Yidong]

Ben Key <bkey76@gmail.com> writes:

> 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.

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.

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

[Chong Yidong]

Ben Key <bkey76@gmail.com> writes:

> 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.

Does it do the right thing even if you leave out the change to xdisp.c?

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

[Ben Key]

[-- 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 --]

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

[Ben Key]

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

My changes to xdisp.c are based on one simple premise that applies to all
programming languages.  A function parameter or variable should not be used
unless you can guarantee that it has been properly initialized to some sane
value 100% of the time.  As the get_specified_cursor_type function is right
now, the cursor_width parameter of ns_draw_window_cursor is only properly
initialized some of the time.

My changes to get_specified_cursor_type satisfy that requirement by
initializing width to some sane value at the very beginning of the function
that is used by the many possible code paths where the function previously
returned without first bothering to initialize width.  The changes do not
break any of the code paths where width is initialized to some other value
before the function returns.

The simple fact is that my changes to get_specified_cursor_type are
absolutely required to guarantee that the cursor_width parameter of
ns_draw_window_cursor is properly initialized to some sane value 100% of the
time, especially if you use the latest version of my patch that uses the
frame column width as the default value for width.

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

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

[Chong Yidong]

Ben Key <bkey76@gmail.com> writes:

> 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
>
> I do not know why you are so opposed to my attempt to fix this bug in
> get_specified_cursor_type.

Because the width argument to get_specified_cursor_type, as written and
documented, should have no effect *unless* the cursor is a bar cursor.
So, whether or not get_specified_cursor_type sets width in the
non-bar-cursor case, should make no difference in behavior.

Indeed, this is the case with X.

If changing the behavior of get_specified_cursor_type (e.g. making it
set a default width) does make a difference on the NS terminal, there is
a problem with the logic somewhere in the NS terminal, or maybe
somewhere else in Emacs.  If so, we need a fix there.  Changing
get_specified_cursor_type would not address this problem.

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

[Chong Yidong]

Chong Yidong <cyd@stupidchicken.com> writes:

> If changing the behavior of get_specified_cursor_type (e.g. making it
> set a default width) does make a difference on the NS terminal, there is
> a problem with the logic somewhere in the NS terminal

By the way, ns_draw_window_cursor really ought to be re-written to
follow x_draw_window_cursor in xterm.c.  The current version is a mess.

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

[Adrian Robert]

Chong Yidong <cyd <at> stupidchicken.com> writes:

> By the way, ns_draw_window_cursor really ought to be re-written to
> follow x_draw_window_cursor in xterm.c.  The current version is a mess.


OK, I'll bite on this since it's my code originally.  :-)  I deliberately chose
not to break into subfunctions like X since the clarity of having the whole
implementation in one block seemed greater than that gained by splitting out. 
Also a number of redundancies in x_draw_bar_cursor() and x_draw_hollow_cursor()
are saved.  Opinions can vary, but calling one approach a "mess" while holding
the other up as a model seems a bit much, especially when long-function style is
used frequently throughout emacs.  I would say both could be improved.

As far as the width/cursor_width, maybe the parameter name could be changed to
"bar_cursor_width" or otherwise documented to indicate its intended use?


-Adrian

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

[Chong Yidong]

Adrian Robert <Adrian.B.Robert@gmail.com> writes:

> OK, I'll bite on this since it's my code originally.  :-) I
> deliberately chose not to break into subfunctions like X since the
> clarity of having the whole implementation in one block seemed greater
> than that gained by splitting out.  Also a number of redundancies in
> x_draw_bar_cursor() and x_draw_hollow_cursor() are saved.

Nothing wrong with choosing not to break into subfunctions, but there
some differences in the basic logic that make it hard to maintain.

Consider the xterm behavior.  For FILLED_BOX_CURSOR, we call
draw_phys_cursor_glyph to redraw that glyph with the cursor face
(DRAW_CURSOR).  No additional repainting is necessary.  For bar cursors,
we paint an extra thin bar on top of the screen area for the existing
glyph; there is no need to call draw_phys_cursor_glyph.

In contrast, ns_draw_window_cursor repaints via NSRectFill (for both box
and bar cursors), and then calls draw_phys_cursor_glyph (for both box
and bar cursors).  The NS terminal seems not to use DRAW_CURSOR for
drawing filled box cursors.  So the basic handling for box cursors seems
to be completely different, and I don't understand the rationale.

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

[Jan Djärv]

2011-02-23 21:06, Chong Yidong skrev:
> Adrian Robert<Adrian.B.Robert@gmail.com>  writes:
>
>> OK, I'll bite on this since it's my code originally.  :-) I
>> deliberately chose not to break into subfunctions like X since the
>> clarity of having the whole implementation in one block seemed greater
>> than that gained by splitting out.  Also a number of redundancies in
>> x_draw_bar_cursor() and x_draw_hollow_cursor() are saved.
>
> Nothing wrong with choosing not to break into subfunctions, but there
> some differences in the basic logic that make it hard to maintain.
>
> Consider the xterm behavior.  For FILLED_BOX_CURSOR, we call
> draw_phys_cursor_glyph to redraw that glyph with the cursor face
> (DRAW_CURSOR).  No additional repainting is necessary.  For bar cursors,
> we paint an extra thin bar on top of the screen area for the existing
> glyph; there is no need to call draw_phys_cursor_glyph.
 > In contrast, ns_draw_window_cursor repaints via NSRectFill (for both box
 > and bar cursors), and then calls draw_phys_cursor_glyph (for both box
 > and bar cursors).

You are simplifying things.  For X a different GC with different background 
and foreground from normal text is used when drawing the glyph (the cursor 
face).  What X in fact does is filling with the background and then paint the 
glyph with the foreground.
There is no such thing in NS.  You have to set the background of the cursor 
face with NSRectFill, and then paint the foreground of the cursor face by 
painting the glyph. It is basically the same thing, but NS does not have one 
call for it.

> The NS terminal seems not to use DRAW_CURSOR for
> drawing filled box cursors.  So the basic handling for box cursors seems
> to be completely different, and I don't understand the rationale.

I don't know what you mean, NS do use DRAW_CURSOR, just search for it in nsterm.m

	Jan D.

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

[Chong Yidong]

Ben Key <bkey76@gmail.com> writes:

>> 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.

Thanks.  I've committed that part.

I don't agree with the change to get_specified_cursor_type, and have not
applied it.  If you can find other instances where bogus cursor width
values are used, we should fix those instances, because it means the
logic in the callers is faulty.