bug#38796: 26.3; `view-lossage': Use a variable for the lossage limit

[Eli Zaretskii <eliz@gnu.org>]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: larsi@gnus.org,  stefankangas@gmail.com,  monnier@iro.umontreal.ca,
>   38796@debbugs.gnu.org,  uyennhi.qm@gmail.com
> Date: Fri, 04 Sep 2020 11:31:33 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> FWIW I have been running Emacs almost 3 months using this branch
> >> with no issues.
> >> If Eli is OK with it, I can merge it to master next week.
> >
> > Sorry, OK with what? where's the patch which I should agree with?
> 
> Branch origin/bug#38796-lossage-limit
> 
> The following is the difference with master branch when I updated
> the branch (last August 27th):

Not sure what that means.  Is this the patch you suggest installing,
or does it need more work to adapt it to the current master?

>    If something surprising happens, and you are not sure what you typed,
>  use @kbd{C-h l} (@code{view-lossage}).  @kbd{C-h l} displays your last
> -300 input keystrokes and the commands they invoked.  If you see
> -commands that you are not familiar with, you can use @kbd{C-h k} or
> +input keystrokes and the commands they invoked.  By default, Emacs
> +stores the last 300 events; if you wish, you can change this number with
              ^^^^^^^^^^^^^^^
The first sentence talks about keystrokes, but the last sentence talks
about "events".  The reader might become confused whether these two
terms refer to the same entity.

> ++++
> +** The new command 'lossage-limit' controls the maximum number
> +of keystrokes and commands recorded.
> +
>  +++
>  ** New variables that hold default buffer names for shell output.
>  The new constants 'shell-command-buffer-name' and
> @@ -92,6 +96,9 @@ The new constants 'shell-command-buffer-name' and
>  for the output of, respectively, synchronous and async shell
>  commands.
>  
> +** The new command lossage-size' allow users to set the maximum

Missing opening quote for lossage-size.

> +number of keystrokes and commands recorded.

NEWS mentions 2 separate commands, but I see only one in the
implementation.

> --- a/lisp/edmacro.el
> +++ b/lisp/edmacro.el
> @@ -35,8 +35,8 @@
>  ;;  * `M-x' followed by a command name, to edit a named command
>  ;;    whose definition is a keyboard macro.
>  ;;
> -;;  * `C-h l' (view-lossage), to edit the 300 most recent keystrokes
> -;;    and install them as the "current" macro.
> +;;  * `C-h l' (view-lossage), to edit the 300 most recent
> +;;    keystrokes and install them as the "current" macro.

This change is a no-op; why is it needed?

>  (defun view-lossage ()
> -  "Display last few input keystrokes and the commands run.
> +  "Display last input keystrokes and the commands run.

Why this change?

> -/* This vector holds the last NUM_RECENT_KEYS keystrokes.  */
> +/* Size of the recent_keys vector */
                                    ^^^
A comment should end in a period and 2 spaces before "*/".

>  /* Pointer to next place to store character in kbd_buffer.  */
>  static union buffered_input_event *kbd_store_ptr;
>  
> +

Do we really need to add an empty line here?

> +DEFUN ("lossage-size", Flossage_size, Slossage_size, 0, 1,
> +       "(list (read-number \"new-size: \" (lossage-size)))",
> +       doc: /* Return the maximum number of saved keystrokes.

The first line describes only one of the two functionalities of this
command; it should describe both.

> +Called with ARG, then set this number to ARG.

"ARG non-nil means set the maximum number of keystrokes to that number."

> +The saved keystrokes are the records shown by `view-lossage'.
> +If you want to disable the lossage records, then set this maximum to a
> +small number, e.g. 0.

The "small number, e.g." part is inaccurate: it _must_ be zero, right?

> +usage: (lossage-size &optional ARG) */)

Is this "usage" needed? what happens if you don't use it?

> +  if (NILP(arg))
> +    return make_fixnum(lossage_limit == 1 ? 0 : lossage_limit);
                         ^
Space here.

So if the user sets the limit to 1, the next call to lossage-size will
return zero?  Isn't that confusing?

> +  /* Internally, the minimum lossage_limit is 1; users will likely use
> +     0 to disable the lossage, thus here we change 0 -> 1.  */
> +  if (new_size == 0)
> +    new_size = 1;

I still don't like this.  I think it will cause confusion and errors.

> +  return Qnil;

Why return nil when setting the limit? why not the previous limit?

> +(ert-deftest keyboard-lossage-size ()
> +  "Test `lossage-size'."
> +  (dolist (val (list 100 300 400 400 500 1000 700 300))
> +    (lossage-size val)
> +    (should (= val (lossage-size))))

This doesn't test the actual recording of VAL events.

Thanks.