Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: rudalics@gmx.at,  70622@debbugs.gnu.org
>> Date: Thu, 09 May 2024 12:56:54 +0200
>>
>> > Eshel, could you please update your patch along these lines?
>>
>> Yes, how does the attached patch look?
>
> Looks good, with a few nits:

Thanks, I'm attaching another iteration of this patch below.

>>  @vindex cursor-type
>> -The @code{cursor-type} frame parameter may be overridden by the
>> -variables @code{cursor-type} and
>> -@code{cursor-in-non-selected-windows}:
>> +The @code{cursor-type} frame parameter may be overridden by
>> +@code{set-window-cursor-type}, and by the variables @code{cursor-type}
>> +and @code{cursor-in-non-selected-windows}:
>
> Please add here a cross-reference to where set-window-cursor-type is
> described.

Done.

>> --- a/doc/lispref/windows.texi
>> +++ b/doc/lispref/windows.texi
>> @@ -5142,6 +5142,18 @@ Window Point
>>  so @code{window-point} will stay behind text inserted there.
>>  @end defvar
>>
>> +@defun set-window-cursor-type window type
>> +This function sets the cursor shape for @var{window}.  This setting
>> +takes precedence over the @code{cursor-type} variable, and @var{type}
>> +has the same format as the value of that variable.
>
> And here, please add a cross-reference to where the values accepted by
> cursor-type are described.

Done.

>> ++++
>> +*** New functions 'set-window-cursor-type' and 'window-cursor-type'.
>> +'set-window-cursor-type' sets a per-window cursor type, and
>> +'window-cursor-type' queries this setting for a given window.
>
> Here, we should tell what is the default value and its meaning.

And done.

>> +DEFUN ("set-window-cursor-type", Fset_window_cursor_type,
>> +       Sset_window_cursor_type, 2, 2, 0,
>> +       doc: /* Set the `cursor-type' of WINDOW to TYPE.
>> +
>> +This setting takes precedence over the variable `cursor-type', and TYPE
>> +has the same format as the value of that variable.  WINDOW nil means use
>> +the selected window.  */)
>> +  (Lisp_Object window, Lisp_Object type)
>> +{
>> +  struct window *w = decode_live_window (window);
>> +
>> +  wset_cursor_type (w, type);
>
> Shouldn't we validate the value of TYPE before plugging it into the
> window?  I know we will validate it at display time, but maybe it's a
> good idea to do that here as well, and signal an error up front?

AFAICT there are no invalid values, since we take "any other value" to
mean the same as 'hollow' (see C-h v cursor-type), so I think not
validating anything should be perfectly valid :)

> Martin, WDYT?
>
>> +DEFUN ("window-cursor-type", Fwindow_cursor_type, Swindow_cursor_type,
>> +       0, 1, 0,
>> +       doc: /* Return the `cursor-type' of WINDOW.
>> +WINDOW must be a live window and defaults to the selected one.  */)
>> +  (Lisp_Object window)
>> +{
>> +  return decode_live_window (window)->cursor_type;
>> +}
>
> This will AFAIU return t if the value is Qt.  Should we return the
> value actually in effect in that case?

Maybe, but that's a bit tricky because we need to consider whether or
not the window is selected and what cursor-in-non-selected-windows has
to say to figure out what the cursor will actually look like.  Currently
this function just provides Lisp with access to the window slot, so we
have all the needed information available in Lisp.  I suggest keeping
'window-cursor-type' simple and, if the need arises, providing a Lisp
function that calculates the effective cursor type for a given window.
WDYT?

>> --- a/src/window.h
>> +++ b/src/window.h
>> @@ -323,6 +323,9 @@ #define WINDOW_H_INCLUDED
>>         as the normal may yield a matrix which is too small.  */
>>      int nrows_scale_factor, ncols_scale_factor;
>>
>> +    /* `cursor-type' to use in this window.  */
>> +    Lisp_Object cursor_type;
>> +
>
> The comment after the mode_line_help_echo member of 'struct window'
> says:
>
>     /* No Lisp data may follow this point; mode_line_help_echo must be
>        the last Lisp member.  */

Oh I missed that, thanks!

> So you must move this new Lisp_Object member to before
> mode_line_help_echo

Done, here's the updated patch: