Subject: Re: bug#77224: [PATCH] New minor mode 'cursor-indicators-mode'

From: Eli Zaretskii <[email-masked]>
Date: Tue, 25 Mar 2025 14:11:58 +0200

> Cc: [email-masked], [email-masked], Juri Linkov <[email-masked]>
> From: Elijah Gabe Pérez <[email-masked]>
> Date: Mon, 24 Mar 2025 12:49:19 -0600
> 
> Ship Mints <[email-masked]> writes:
> 
>  I think you need to update your configuration to not use tabs in lisp code–your patch is filled with tabs.
>  Standard elisp is this (see emacs/.dir-locals.el for other settings):
>  (emacs-lisp-mode . ((indent-tabs-mode . nil) …
> 
> Yeah, i forgot to untabify the whole file.

Thanks.

> +(defcustom cursor-indicators-overwrite-indicator "red3"

Using literal color names might be problematic on terminals that don't
support enough colors, unless you are extra careful to select colors
that are supported everywhere.  Alternatively, the default value could
be different for different numbers of supported colors.

Also, what happens if an Emacs session has both GUI frames (with lots
of colors) and TTY frames with a small number of colors -- does each
frame get its own color?

> +  "Cursor indicator used when overwrite mode is set, if non-nil.

That "if non-nil" part can potentially confuse, because it isn't clear
whether it refers to the value of the variable or to "overwrite mode".
Better say this the other way around:

  If non-nil, the cursor indication to use when overwrite mode is set.

(Note that I also slightly reworded the rest of this sentence, for
clarity.)

Suggest to make the same changes for other user options in this file.

> +(defcustom cursor-indicators-repeat-indicator "blue"
> +  "Cursor indicator used when `repeat-in-progress' is non-nil.

repeat-in-progress is an obscure variable, which is also not visible
until repeat.el is loaded.  Please reword this to be in user-level
terms, like "when a command is repeated due to `repeat-mode'" or
something similar.

Please also add the :version tag to each and every new defcustom.

> +(defcustom cursor-indicators-input-method-indicator "dark magenta"
> +  "Cursor indicator used when input method is activated now.
                                                           ^^^
What do you mean by "now"?

> +(define-minor-mode cursor-indicators-mode
> +  "Change cursor type and/or face for indications.

  "Change cursor type and/or face to indicate various minor modes."

> +When enabled, this mode changes the cursor appearance to indicate
> +overwrite mode or read-only buffers."
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
That is not the full list of indications.