[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#38796: 26.3; `view-lossage': Use a variable for the lossage limit
From: |
Eli Zaretskii |
Subject: |
bug#38796: 26.3; `view-lossage': Use a variable for the lossage limit |
Date: |
Fri, 04 Sep 2020 15:07:50 +0300 |
> 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.