Subject: Re: bug#70622: [PATCH] New window parameter 'cursor-type'

From: Eshel Yaron <[email-masked]>
Date: Sun, 28 Apr 2024 21:05:59 +0200

Eli Zaretskii <[email-masked]> writes:

>> From: Eshel Yaron <[email-masked]>
>> Cc: [email-masked]
>> Date: Sun, 28 Apr 2024 17:00:33 +0200
>>
>> > Why only cons cells are supported?
>>
>> We need a convenient way to specify both "window parameter not set" and
>> "window parameter set and says not to show a cursor".  Naturally, we
>> want to use nil for "window parameter not set", but then nil is what the
>> variable cursor-type uses for "don't to show a cursor".
>
> We have the same problem in the frame parameter by this name, and we
> solve it there without these complications.  Why should the window
> parameter be different?

This is a little different, though.  Indeed, the frame parameter also
uses nil to denote "no cursor".  The variable, which overrides the frame
parameter, uses t to delegate to the frame parameter, so it supports all
values of the frame parameter (including nil), plus t.  But unlike a
variable, we can't make the window parameter default to t (or any other
non-nil value) as easily.

> It's confusing and hard to remember.

I went with this format because it lets you use all values of the
cursor-type variable in a uniform manner (just wrap it in a list).
Alternatively, we can use the exact same format for the window
parameter as we do for the variable, except we interpret a window
parameter nil value as "window parameter unset", not "no cursor", and
instead add another value for the window parameter, 'none, that'll
correspond to the nil value of the variable (so window parameter 'none
would say not to show the cursor).  This is in line with what we have
for the mode-line-format variable and window parameter, for example.
Does this alternative sound better?

>> > And I have a question: is this supposed to work for non-selected
>> > windows as well?  The documentation you added says nothing about that,
>> > but I wonder what was the intent?
>>
>> Yes, it's supposed to work for non-selected windows as well.  Do you
>> think that's worth mentioning explicitly in the documentation?
>
> Not just that, but it also means one cannot specify a different cursor
> type for a window when it's selected and when not selected, unlike
> with buffers.  Is this anomaly justified?

See below.

>> > The reason I ask is that we have two buffer-local variables, not one,
>> > for both selected and non-selected windows, whereas your patch
>> > provides just one window parameter.  How will it interact with the
>> > buffer-local variables in both cases?
>>
>> That's a really good point.  WRT cursor-in-non-selected-windows, I think
>> there are two viable options:
>>
>> 1. Give cursor-in-non-selected-windows precedence over the new window
>>    parameter, and add another window parameter to override
>>    cursor-in-non-selected-windows.
>> 2. Give the new window parameter precedence also over
>>    cursor-in-non-selected-windows.
>>
>> In the updated patch, I went with option 2, so if you set the
>> cursor-type window parameter, that overrides any buffer-local variable,
>> whether or not the window is selected.  I think that's sensible enough,
>> WDYT?
>
> I tend to think option 1 is better, but I'm curious what others think.

All right.  I can implement that option instead, if that's deemed
preferable.  The way I see it, option 1 makes it easy to set the
cursor for a window and have it change based on whether or not the
window is selected, but it requires a bit more work to set the cursor
for that window once and for all if you don't want it to change based
on window selection.  Option 2 is the other way around: it's easier in
case you want to set the cursor type for a window regardless of
whether it is selected or not.  So there's a small tradeoff here.

> Martin, WDYT?

[...]

> I think it would be better to make sure the parameter takes effect
> immediately.  Documenting the force-window-update thing should be
> fallback, if the immediate effect is impossible.

OK, I'll look into it.

>>        if (w == XWINDOW (echo_area_window))
>>      {
>> -	  if (EQ (BVAR (b, cursor_type), Qt) || NILP (BVAR (b, cursor_type)))
>> +	  win_cursor = window_parameter (w, Qcursor_type);
>> +	  if (CONSP (win_cursor))
>> +	    {
>> +	      return get_specified_cursor_type (XCAR (win_cursor), width);
>> +	    }
>
> We don't use braces for a single-line block.

Thanks, I'll fix that in the next iteration of this patch.

Best,

Eshel