Re: PATCH: changing terminal cursor form

[Oliver Kiddle <opk@xxxxxxx>]

Mikael Magnusson wrote:
> This feels a bit confusing to me, the name suggests it is an array
> containing enabled extensions, but the above suggests that an empty
> element can imply an extension is enabled while an entry with a - in
> front disables it. To me it would make more sense if the value started

The array is intended to be a list of extensions that your terminal
supports. Each entry typically corresponds to an escape sequence that is
not covered by termcap. So arguably you are asserting that your terminal
doesn't support cursor changes when you do:
  .term.extensions=( -cursor )
To disable the new zsh feature you might instead use:
  zle_cursorform=( pending:none overwrite:none insert:none )

The first command is a lie but for disabling a feature it can be simpler
and won't need adapting if new contexts are added with a default cursor
shape.

> out with "query" as an element, such that setting .term.extensions=()
> would disable querying on startup. The documentation also says some

For each extension, there are three possible states: supported,
unsupported and unknown. Absence from the list communicates the
"unknown" state and in that case a default applies. For the most part
terminals simply ignore unrecognised sequences that are nevertheless a
valid CSI, OSC, etc sequence so it is harmless to enable the use of
most extensions by default.

If the array started out with all enabled extensions, that'd have to be
set before the zle module is loaded and users would need to know how to
filter out individual values. So instead of:
  .term.extensions+=( -cursor-shape )
They would need to know how to do:
  .term.extensions=( ${.term.extensions:#cursor-shape} )

> things that seem a bit conflicting, the entry for .extensions claims
>
>   When ZLE starts, it will add entries for
>   features that were auto-detected.  This auto-detection uses extensions itself,
>   all named with a `tt(query)' prefix.
>
> but the entries for the query-foo values only state that they query
> the foo value, not that foo feature is "detected" and enabled in any
> way, for example,

While all (both currently) auto-detection extensions are named with a
query prefix the converse is not the case: i.e. not all extensions named
with a `query' prefix are for auto-detecting terminal support for an
extension.

I've tried to use common prefixes to classify extensions and to allow
them to be enabled or disabled together. `query' is used for all
extensions that entail reading a response back from the terminal.

> +item(tt(query-cursor) <E>)(

> Perhaps a user would like to query the cursor color and have access to
> its values, without enabling the other automagic cursor color/shape
> changes? And analogously for the other stuff.

This doesn't enable the cursor-color extension. It just sets
.term.cursor and a separate internal variable. That internal variable
would only be used to reset the colour if a colour that is configured in
zle_cursorform got applied.

> I also don't understand (from the documentation) if setting -cursor
> will disable the querying, or if it would still query but then not
> enable the cursor extension (or some other result)? Would setting
> -query-cursor have a different result? (assuming it is done before ZLE
> init)

There's no point in doing auto-detection of an extension that the user
has explicitly declared to be supported or unsupported (as opposed to
unknown). query-color exists to populate .term.cursor and not as an
auto-detection mechanism.

> Actually after rereading the documentation I am even more confused,
> there is "query-cursor", but no "cursor", only "cursor-color" and
> "cursor-shape", but query-cursor only queries the color, and enables
> both of these features? Meanwhile the color is stored in .term.cursor,
> not .term.cursorcolor, and is customized via zle_cursorform. Maybe
> some consistency with the naming would be helpful here.

Would it be better if renamed to query-cursorcolor? The lack of a second
hyphen being intentional to separate it from cursor-color which it
doesn't enable or disable.

If you think .term.cursorcolor is a better variable name, that's fine.

Naming is always one of the trickiest parts and suggestions are very
welcome.

> There's a few more bits than this (precmd/exec callsites etc), but I
> think this shows the general idea,

You'll be unaffected other than getting an underline cursor for
overwrite mode. The fact that the defaults don't set any colour means
that any setup that only entails colours will be unaffected. Your
completion active configuration would also be unaffected even if it was
applying shapes. But if you were setting, a bar cursor for overwrite
mode, that might be replaced.

> I suppose that your new system won't really interfere with this too
> much, but historically we have opted to not enable new features by
> default when they can conflict with existing configurations. I'm not
> vetoing it or anything, but we could also consider just adding it to
> the newuser wizard thingy?

What to enable is always a balance and involves making a judgement.
Leaving everything disabled means most users will never enable them and
I've often seen criticism of zsh for the fact that you need huge amounts
of configuration. Very little is being enabled by default here - just
the cursor shape in three contexts, two of which are only relevant to vi
mode users.

> I've never seen the word cursor form before, I assume it is supposed
> to entail the combination of shape and color? I couldn't see the term
> explicitly introduced anywhere.

That's exactly how I'm using it. I'm aware that the word "form" does
not normally encompass colour. I did consult a thesaurus in search of a
better term without success. If you can think of anything better, please
say. Otherwise, I'll look to define it explicitly in the documentation.

Thanks for the comments. It's always useful to get the perspective of
someone else because when you know the fresh code some things seem
obvious when they really aren't. I'll try to improve the documentation.
I think it's outgrown params.yo anyway and should be moved to zle.yo
with just a reference in the parameter list. Do my attempts at
clarification here make sense because I might propagate any lack of
clarity to the documentation.

Should I perhaps apply the patch series despite the outstanding SIGINT
issue to at least make it easier for people to test the changes?

Oliver