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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Drew Adams
This report is a follow-up to this emacs-devel thread:

https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00678.html

and to the older thread that it references:

https://lists.gnu.org/archive/html/emacs-devel/2006-03/msg01012.html

Please use a variable for the limit of how many characters to use for
recording lossage.

Doing that will give users (and Lisp code) control over the limit.
Having a limit that is hard-coded in C is not user-friendly - it's even
unemacsy.

The latter thread mentioned above ended with RMS's decision to have this
taken care of after the release that was then incipient.  But this was
never done.

I'd prefer to see this as a user option, but an ordinary defvar would
already be a great improvement.

I don't think it's uncommon to have mistakenly typed some keys, gotten
in a strange or unfamiliar situation as a result of that, and then tried
to find out just what keys led to that.  Depending on what else you did
in the meantime, before hitting `C-h l' (e.g., to get out of the strange
situation), `view-lossage' might not reach far enough backward to show
you the mistake you made.

It shouldn't be a big deal to move this limit to a Lisp variable.  Can
this please be done now?

---

Not part of this bug report, but `view-lossage' could also be improved
by letting you (or Lisp code) filter the recorded "keystroke" history,
to remove stuff that you feel is really extraneous (e.g. not keyboard
keystrokes).  Such filtering could be just hiding that from view.

(This is all the more relevant if we can control the lossage limit.  The
really relevant user input is sometimes a relatively small subset of the
full list of recorded events you see now with `C-h l'.)


In GNU Emacs 26.3 (build 1, x86_64-w64-mingw32)
 of 2019-08-29
Repository revision: 96dd0196c28bc36779584e47fffcca433c9309cd
Windowing system distributor `Microsoft Corp.', version 10.0.17763
Configured using:
 `configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static -g3''



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
severity 38796 wishlist
thanks

> Date: Sun, 29 Dec 2019 09:04:54 -0800 (PST)
> From: Drew Adams <[hidden email]>
>
> This report is a follow-up to this emacs-devel thread:
>
> https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00678.html
>
> and to the older thread that it references:
>
> https://lists.gnu.org/archive/html/emacs-devel/2006-03/msg01012.html
>
> Please use a variable for the limit of how many characters to use for
> recording lossage.
>
> Doing that will give users (and Lisp code) control over the limit.
> Having a limit that is hard-coded in C is not user-friendly - it's even
> unemacsy.

It's a constant size of a vector that records the keys in a cyclical
fashion.  So it isn't enough to expose the value to Lisp; one must
also write code that reallocates the vector when the value changes,
and copies the keystrokes from the old to the new vector in the
correct order.  Not rocket science, but still.

Patches are welcome.



Reply | Threaded
Open this post in threaded view
|

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

Drew Adams
> It's a constant size of a vector that records the keys in a cyclical
> fashion.  So it isn't enough to expose the value to Lisp; one must
> also write code that reallocates the vector when the value changes,
> and copies the keystrokes from the old to the new vector in the
> correct order.  Not rocket science, but still.

I see.  I hope such an enhancement is made.

> Patches are welcome.

A C patch will have to come from someone other than me.



Reply | Threaded
Open this post in threaded view
|

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

Tino Calancha-2
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:

>> From: Drew Adams <[hidden email]>
>> This report is a follow-up to this emacs-devel thread:
>> https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00678.html
>> and to the older thread that it references:
>> https://lists.gnu.org/archive/html/emacs-devel/2006-03/msg01012.html
>> Please use a variable for the limit of how many characters to use for
>> recording lossage.

> it isn't enough to expose the value to Lisp; one must
> also write code that reallocates the vector when the value changes,
> and copies the keystrokes from the old to the new vector in the
> correct order.
>
> Patches are welcome.

I have uploaded the branch feature/bug#38796-lossage-limit
[I've been using it during last 10 days with no issues]

Initially I decided to hardcode a sensible minimum of 100 (keeping
the current default of 300).  That is the first commit.
Actually, the code works for N > 0, but as Drew suggested, a value < 100
(which gives only ~ 50 lines of past inputs) is too small to be useful
for the use case of checking typing mistakes.

After some time I thought that, for some people (anyone in this thread),
a very small value in fact can be seen as a security feature;
see for instance the docstrings of `term-read-noecho' and
`comint-send-invisible'.  That motivates the second commit.

--8<-----------------------------cut here---------------start------------->8---
commit 9723c2645c8f47e651a81a023e80f882d181cc3f
Author: Tino Calancha <[hidden email]>
Date:   Sun Jun 7 23:37:59 2020 +0200

    Give Lisp control on the lossage size
   
    Add an user option to control the maximum number of
    recorded keystrokes (a.k.a lossage limit) (Bug#38796).
   
    * src/keyboard.c (lossage-limit): Add new variable.
    (MIN_NUM_RECENT_KEYS): Renamed from NUM_RECENT_KEYS.
    Set it as 100 and use it as the minimum value for lossage-limit.
    Keep the same default for the vector size as before (300).
    (update-lossage-limit): New function.
    (update_recent_keys): Helper function.
    (command_loop_1)
    (record_char)
    (recent-keys)
    (syms_of_keyboard): Replace NUM_RECENT_KEYS with lossage_limit as
    the vector size.
    (clear-this-command-keys): Fix docstring.
   
    * lisp/help.el (view-lossage): Mention lossage-limit in the docstring.
   
    * lisp/cus-start.el (lossage-limit): Register it as an user option.
   
    * lisp/edmacro.el (edit-kbd-macro): Update docstring and commentary header.
   
    * etc/NEWS (Changes in Emacs 28.1): Announce the new option.
    * doc/emacs/help.texi (Misc Help): Document it.
    * test/src/keyboard-tests.el (keyboard-lossage-limit): Add test.

diff --git a/doc/emacs/help.texi b/doc/emacs/help.texi
index 167c32c4d2..e2c0dc6802 100644
--- a/doc/emacs/help.texi
+++ b/doc/emacs/help.texi
@@ -563,10 +563,13 @@ Misc Help
 
 @kindex C-h l
 @findex view-lossage
+@vindex lossage-limit
   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 option @code{lossage-limit}.
+If you see commands that you are not familiar with, you can use @kbd{C-h k} or
 @kbd{C-h f} to find out what they do.
 
 @kindex C-h e
diff --git a/etc/NEWS b/etc/NEWS
index d58a61be21..da204c5825 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -70,6 +70,10 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 
 * Changes in Emacs 28.1
 
++++
+** The new option 'lossage-limit' controls the maximum number
+of keystrokes and commands recorded.
+
 ** Support for '(box . SIZE)' 'cursor-type'.
 By default, 'box' cursor always has a filled box shape.  But if you
 specify 'cursor-type' to be '(box . SIZE)', the cursor becomes a hollow
diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index 6632687da4..1ebf554b2b 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -352,6 +352,8 @@ minibuffer-prompt-properties--setter
      ;; indent.c
      (indent-tabs-mode indent boolean)
      ;; keyboard.c
+             (lossage-limit keyboard integer "28.1"
+                            :set (lambda (_ val) (update-lossage-limit val)))
      (meta-prefix-char keyboard character)
      (auto-save-interval auto-save integer)
              (auto-save-no-message auto-save boolean "27.1")
diff --git a/lisp/edmacro.el b/lisp/edmacro.el
index 71474c0289..089fea17ef 100644
--- 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 `lossage-limit' most recent
+;;    keystrokes and install them as the "current" macro.
 ;;
 ;;  * any key sequence whose definition is a keyboard macro.
 ;;
@@ -88,7 +88,7 @@ edit-kbd-macro
   "Edit a keyboard macro.
 At the prompt, type any key sequence which is bound to a keyboard macro.
 Or, type `\\[kmacro-end-and-call-macro]' or RET to edit the last
-keyboard macro, `\\[view-lossage]' to edit the last 300
+keyboard macro, `\\[view-lossage]' to edit the last `lossage-limit'
 keystrokes as a keyboard macro, or `\\[execute-extended-command]'
 to edit a macro by its command name.
 With a prefix argument, format the macro in a more concise way."
diff --git a/lisp/help.el b/lisp/help.el
index b7d867eb70..9e3d0922d0 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -456,9 +456,10 @@ view-external-packages
   (info "(efaq)Packages that do not come with Emacs"))
 
 (defun view-lossage ()
-  "Display last few input keystrokes and the commands run.
+  "Display last input keystrokes and the commands run.
 For convenience this uses the same format as
 `edit-last-kbd-macro'.
+See `lossage-limit' to update the number of recorded keystrokes.
 
 To record all your input, use `open-dribble-file'."
   (interactive)
diff --git a/src/keyboard.c b/src/keyboard.c
index f9b9399d50..a4d27c94fb 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -103,7 +103,7 @@ static KBOARD *all_kboards;
 /* True in the single-kboard state, false in the any-kboard state.  */
 static bool single_kboard;
 
-#define NUM_RECENT_KEYS (300)
+#define MIN_NUM_RECENT_KEYS (100)
 
 /* Index for storing next element into recent_keys.  */
 static int recent_keys_index;
@@ -111,7 +111,7 @@ static int recent_keys_index;
 /* Total number of elements stored into recent_keys.  */
 static int total_keys;
 
-/* This vector holds the last NUM_RECENT_KEYS keystrokes.  */
+/* This vector holds the last lossage_limit keystrokes.  */
 static Lisp_Object recent_keys;
 
 /* Vector holding the key sequence that invoked the current command.
@@ -294,6 +294,7 @@ static union buffered_input_event *kbd_fetch_ptr;
 /* Pointer to next place to store character in kbd_buffer.  */
 static union buffered_input_event *kbd_store_ptr;
 
+
 /* The above pair of variables forms a "queue empty" flag.  When we
    enqueue a non-hook event, we increment kbd_store_ptr.  When we
    dequeue a non-hook event, we increment kbd_fetch_ptr.  We say that
@@ -1421,10 +1422,10 @@ command_loop_1 (void)
       /* Execute the command.  */
 
       {
- total_keys += total_keys < NUM_RECENT_KEYS;
+ total_keys += total_keys < lossage_limit;
  ASET (recent_keys, recent_keys_index,
       Fcons (Qnil, cmd));
- if (++recent_keys_index >= NUM_RECENT_KEYS)
+ if (++recent_keys_index >= lossage_limit)
   recent_keys_index = 0;
       }
       Vthis_command = cmd;
@@ -3248,15 +3249,15 @@ record_char (Lisp_Object c)
       int ix1, ix2, ix3;
 
       if ((ix1 = recent_keys_index - 1) < 0)
- ix1 = NUM_RECENT_KEYS - 1;
+ ix1 = lossage_limit - 1;
       ev1 = AREF (recent_keys, ix1);
 
       if ((ix2 = ix1 - 1) < 0)
- ix2 = NUM_RECENT_KEYS - 1;
+ ix2 = lossage_limit - 1;
       ev2 = AREF (recent_keys, ix2);
 
       if ((ix3 = ix2 - 1) < 0)
- ix3 = NUM_RECENT_KEYS - 1;
+ ix3 = lossage_limit - 1;
       ev3 = AREF (recent_keys, ix3);
 
       if (EQ (XCAR (c), Qhelp_echo))
@@ -3307,12 +3308,12 @@ record_char (Lisp_Object c)
     {
       if (!recorded)
  {
-  total_keys += total_keys < NUM_RECENT_KEYS;
+  total_keys += total_keys < lossage_limit;
   ASET (recent_keys, recent_keys_index,
                 /* Copy the event, in case it gets modified by side-effect
                    by some remapping function (bug#30955).  */
                 CONSP (c) ? Fcopy_sequence (c) : c);
-  if (++recent_keys_index >= NUM_RECENT_KEYS)
+  if (++recent_keys_index >= lossage_limit)
     recent_keys_index = 0;
  }
       else if (recorded < 0)
@@ -3326,10 +3327,10 @@ record_char (Lisp_Object c)
 
   while (recorded++ < 0 && total_keys > 0)
     {
-      if (total_keys < NUM_RECENT_KEYS)
+      if (total_keys < lossage_limit)
  total_keys--;
       if (--recent_keys_index < 0)
- recent_keys_index = NUM_RECENT_KEYS - 1;
+ recent_keys_index = lossage_limit - 1;
       ASET (recent_keys, recent_keys_index, Qnil);
     }
  }
@@ -10410,6 +10411,55 @@ If CHECK-TIMERS is non-nil, timers that are ready to run will do so.  */)
   ? Qt : Qnil);
 }
 
+static void
+update_recent_keys (int new_size, int kept_keys)
+{
+  int osize = ASIZE (recent_keys);
+  eassert (recent_keys_index < osize);
+  eassert (kept_keys <= min (osize, new_size));
+  Lisp_Object v = make_nil_vector (new_size);
+  int i, idx;
+  for (i = 0; i < kept_keys; ++i)
+    {
+      idx = recent_keys_index - kept_keys + i;
+      while (idx < 0)
+        idx += osize;
+      ASET (v, i, AREF (recent_keys, idx));
+    }
+  recent_keys = v;
+  total_keys = kept_keys;
+  recent_keys_index = total_keys % new_size;
+  lossage_limit = new_size;
+
+}
+
+/* Reallocate recent_keys copying the keystrokes in the right order */
+DEFUN ("update-lossage-limit", Fupdate_lossage_limit,
+       Supdate_lossage_limit, 1, 1, 0,
+       doc: /* Update the maximum number of saved keystrokes to ARG.
+usage: (update-lossage-limit ARG)  */)
+  (Lisp_Object arg)
+{
+  if (!FIXNATP (arg))
+    user_error ("Value must be a positive integer");
+  int osize = ASIZE (recent_keys);
+  eassert (lossage_limit == osize);
+  int min_size = MIN_NUM_RECENT_KEYS;
+  int new_size = XFIXNAT (arg);
+
+  if (new_size == osize)
+    return Qnil;
+  if (new_size < min_size)
+    {
+      AUTO_STRING (fmt, "Value must be >= %d");
+      Fsignal (Quser_error, list1 (CALLN (Fformat, fmt, make_fixnum (min_size))));
+    }
+  int kept_keys = new_size > osize ? total_keys : min (new_size, total_keys);
+  update_recent_keys (new_size, kept_keys);
+
+  return Qnil;
+}
+
 DEFUN ("recent-keys", Frecent_keys, Srecent_keys, 0, 1, 0,
        doc: /* Return vector of last few events, not counting those from keyboard macros.
 If INCLUDE-CMDS is non-nil, include the commands that were run,
@@ -10419,21 +10469,21 @@ represented as pseudo-events of the form (nil . COMMAND).  */)
   bool cmds = !NILP (include_cmds);
 
   if (!total_keys
-      || (cmds && total_keys < NUM_RECENT_KEYS))
+      || (cmds && total_keys < lossage_limit))
     return Fvector (total_keys,
     XVECTOR (recent_keys)->contents);
   else
     {
       Lisp_Object es = Qnil;
-      int i = (total_keys < NUM_RECENT_KEYS
+      int i = (total_keys < lossage_limit
        ? 0 : recent_keys_index);
-      eassert (recent_keys_index < NUM_RECENT_KEYS);
+      eassert (recent_keys_index < lossage_limit);
       do
  {
   Lisp_Object e = AREF (recent_keys, i);
   if (cmds || !CONSP (e) || !NILP (XCAR (e)))
     es = Fcons (e, es);
-  if (++i >= NUM_RECENT_KEYS)
+  if (++i >= lossage_limit)
     i = 0;
  } while (i != recent_keys_index);
       es = Fnreverse (es);
@@ -10531,8 +10581,8 @@ The value is always a vector.  */)
 DEFUN ("clear-this-command-keys", Fclear_this_command_keys,
        Sclear_this_command_keys, 0, 1, 0,
        doc: /* Clear out the vector that `this-command-keys' returns.
-Also clear the record of the last 100 events, unless optional arg
-KEEP-RECORD is non-nil.  */)
+Also clear the record of the last `lossage-limit' keystroke events,
+unless optional arg KEEP-RECORD is non-nil.  */)
   (Lisp_Object keep_record)
 {
   int i;
@@ -11686,7 +11736,23 @@ syms_of_keyboard (void)
     staticpro (&modifier_symbols);
   }
 
-  recent_keys = make_nil_vector (NUM_RECENT_KEYS);
+  DEFVAR_INT ("lossage-limit", lossage_limit,
+              doc: /* Maximum number of stored keyboard events and commands run.
+
+Please, do not set this variable in Lisp with `setq' neither
+let-bind it, that will likely crash Emacs.  This is because
+`setq' only changes the variable, but it doesn't update
+the size of the internal vector that holds the keystrokes.
+
+To update this variable use the customization menu, or
+call from Lisp the following expression:
+
+  (update-lossage-limit new-limit)
+
+That takes care of both, the variable and the internal vector.*/);
+  lossage_limit = 3 * MIN_NUM_RECENT_KEYS;
+
+  recent_keys = make_nil_vector (lossage_limit);
   staticpro (&recent_keys);
 
   this_command_keys = make_nil_vector (40);
@@ -11736,6 +11802,7 @@ syms_of_keyboard (void)
   defsubr (&Srecursive_edit);
   defsubr (&Sinternal_track_mouse);
   defsubr (&Sinput_pending_p);
+  defsubr (&Supdate_lossage_limit);
   defsubr (&Srecent_keys);
   defsubr (&Sthis_command_keys);
   defsubr (&Sthis_command_keys_vector);
diff --git a/test/src/keyboard-tests.el b/test/src/keyboard-tests.el
index 1988ba51a7..017d239246 100644
--- a/test/src/keyboard-tests.el
+++ b/test/src/keyboard-tests.el
@@ -32,5 +32,16 @@
                         (read-event nil nil 2))
                  ?\C-b)))
 
+(ert-deftest keyboard-lossage-limit ()
+  "Test `lossage-limit' updates."
+  (dolist (val (list 100 100 200 500 300 1000 700))
+    (update-lossage-limit val)
+    (should (= val lossage-limit)))
+  (let ((current-limit lossage-limit))
+    (should-error (update-lossage-limit 5))
+    (should-error (update-lossage-limit "200"))
+    (should (= lossage-limit current-limit))))
+
+
 (provide 'keyboard-tests)
 ;;; keyboard-tests.el ends here

commit a60a05994aff16bc27f153ea8f765e15b92f21be
Author: Tino Calancha <[hidden email]>
Date:   Mon Jun 22 22:41:34 2020 +0200

    Allow disable the record of keystrokes (lossage)
   
    Use 1 as the minimum value for lossage-limit; such a value
    is equivalent to not recording the keystrokes: having just 1 entry,
    will be overwritten with the view-lossage call itself.
   
    * test/src/keyboard-tests.el (keyboard-lossage-limit): Update test.
    * src/keyboard.c (MIN_NUM_RECENT_KEYS): Delete it.
    (lossage_limit): Add security note in the doctring.
    * lisp/cus-start.el (lossage-limit): Let users choose to disable
    the record of the keystrokes.
    * doc/emacs/help.texi (Misc Help): Update manual.
    * etc/NEWS (Changes in Emacs 28.1):
    Mention that it's possible to disable the record of keystrokes.

diff --git a/doc/emacs/help.texi b/doc/emacs/help.texi
index e2c0dc6802..69ea96763b 100644
--- a/doc/emacs/help.texi
+++ b/doc/emacs/help.texi
@@ -569,8 +569,14 @@ Misc Help
 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 option @code{lossage-limit}.
-If you see commands that you are not familiar with, you can use @kbd{C-h k} or
-@kbd{C-h f} to find out what they do.
+If you see commands that you are not familiar with, you can use @kbd{C-h k}
+or @kbd{C-h f} to find out what they do.
+If you don't like that Emacs saves your keystrokes, then you can
+set @code{lossage-limit} equal to 1; such a value effectively disables the
+record of the keystrokes.  Please, do not set this option with @code{setq}
+neither let-bind it; that will likely crash Emacs.  Use instead the
+customization menu, which also updates the internal structure holding
+the keystrokes.
 
 @kindex C-h e
 @findex view-echo-area-messages
diff --git a/etc/NEWS b/etc/NEWS
index da204c5825..17776173e0 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -72,7 +72,8 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 
 +++
 ** The new option 'lossage-limit' controls the maximum number
-of keystrokes and commands recorded.
+of keystrokes and commands recorded.  The value 1 disables
+the record of keystrokes.
 
 ** Support for '(box . SIZE)' 'cursor-type'.
 By default, 'box' cursor always has a filled box shape.  But if you
diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index 1ebf554b2b..bd4ff3a74b 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -352,7 +352,11 @@ minibuffer-prompt-properties--setter
      ;; indent.c
      (indent-tabs-mode indent boolean)
      ;; keyboard.c
-             (lossage-limit keyboard integer "28.1"
+             (lossage-limit keyboard
+                            (choice (const :tag "Do not record keystrokes" 1)
+                                    integer)
+                            "28.1"
+                            :standard 300
                             :set (lambda (_ val) (update-lossage-limit val)))
      (meta-prefix-char keyboard character)
      (auto-save-interval auto-save integer)
diff --git a/src/keyboard.c b/src/keyboard.c
index a4d27c94fb..9af1b9ba50 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -103,8 +103,6 @@ static KBOARD *all_kboards;
 /* True in the single-kboard state, false in the any-kboard state.  */
 static bool single_kboard;
 
-#define MIN_NUM_RECENT_KEYS (100)
-
 /* Index for storing next element into recent_keys.  */
 static int recent_keys_index;
 
@@ -10444,7 +10442,7 @@ usage: (update-lossage-limit ARG)  */)
     user_error ("Value must be a positive integer");
   int osize = ASIZE (recent_keys);
   eassert (lossage_limit == osize);
-  int min_size = MIN_NUM_RECENT_KEYS;
+  int min_size = 1;
   int new_size = XFIXNAT (arg);
 
   if (new_size == osize)
@@ -11749,8 +11747,11 @@ call from Lisp the following expression:
 
   (update-lossage-limit new-limit)
 
-That takes care of both, the variable and the internal vector.*/);
-  lossage_limit = 3 * MIN_NUM_RECENT_KEYS;
+That takes care of both, the variable and the internal vector.
+
+Security note: The value 1 makes impossible to recover a typed string
+with `view-lossage'.*/);
+  lossage_limit = 300;
 
   recent_keys = make_nil_vector (lossage_limit);
   staticpro (&recent_keys);
diff --git a/test/src/keyboard-tests.el b/test/src/keyboard-tests.el
index 017d239246..4541c389d0 100644
--- a/test/src/keyboard-tests.el
+++ b/test/src/keyboard-tests.el
@@ -38,7 +38,7 @@
     (update-lossage-limit val)
     (should (= val lossage-limit)))
   (let ((current-limit lossage-limit))
-    (should-error (update-lossage-limit 5))
+    (should-error (update-lossage-limit 0))
     (should-error (update-lossage-limit "200"))
     (should (= lossage-limit current-limit))))
 

--8<-----------------------------cut here---------------end--------------->8---

In GNU Emacs 28.0.50 (build 18, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
 of 2020-06-26 built on calancha-pc.dy.bbexcite.jp
Repository revision: ffb89ed5f07491e33fc79d8b4be49d9deba2ad4a
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)



Reply | Threaded
Open this post in threaded view
|

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

Tino Calancha-2


On Sat, 27 Jun 2020, Eli Zaretskii wrote:

> My personal view is that we should allow only growing the size and
> resetting it to the original size of 300.  Disabling the key record
> should be a separate feature, most probably implemented by means other
> than shrinking the recent_keys vector.

I totally agree: they are clearly 2 separated features.

Thanks for the comments, I will work on them and get back once
completed and tested.




Reply | Threaded
Open this post in threaded view
|

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

Stefan Monnier
>> My personal view is that we should allow only growing the size and
>> resetting it to the original size of 300.  Disabling the key record
>> should be a separate feature, most probably implemented by means other
>> than shrinking the recent_keys vector.
>
> I totally agree: they are clearly 2 separated features.
>
> Thanks for the comments, I will work on them and get back once
> completed and tested.

I agree that disabling should not necessarily be implemented by
"abusing" the max-lossage setting.

But I don't see any reason to impose a 300 minimum either.  I think it's
fine to impose a minimum, but it should be dictated by the limits of the
code.  I'm not saying we should work to push this lower limit down, but
just that it should reflect what the code can do safely rather than
being an arbitrary number like 300 (I'm pretty sure 100 would be safe as
well, since that's what we've used for many years).


        Stefan




Reply | Threaded
Open this post in threaded view
|

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

Tino Calancha-2
On Sun, 28 Jun 2020, Drew Adams wrote:
> OP here.
I was waiting for you.  Welcome!! :-)


Stefan Monnier <[hidden email]> writes:

> I agree that disabling should not necessarily be implemented by
> "abusing" the max-lossage setting.
Yeah, it was appealing to me at first look: the code was offering
another feature for free!
After your comments I rethink about it and become less excited: it
always posible that we might be hit back in other parts of the code
if they assume a 'sensible large enough' value.

> But I don't see any reason to impose a 300 minimum either.  I think it's
> fine to impose a minimum, but it should be dictated by the limits of the
> code.  I'm not saying we should work to push this lower limit down, but
> just that it should reflect what the code can do safely rather than
> being an arbitrary number like 300 (I'm pretty sure 100 would be safe as
> well, since that's what we've used for many years).
Sure, 100 must be absolutely fine.

I have updated the branch 'bug#38796-lossage-limit'.
Now, the limit is not exposed to Lisp anymore.
The function to update it now is a command 'update-lossage-size'
We also have a 'lossage-size' function to retrieve its current value.

--8<-----------------------------cut here---------------start------------->8---
commit c86d060b460bdaecdd71d297fa84012fcd397d66
Author: Tino Calancha <[hidden email]>
Date:   Sun Jun 28 23:30:07 2020 +0200

    Do not expose the size of recent_keys to Lisp
   
    That prevents from unintentional crashes if the users
    modify the variable with setq or if they let-bind it.
   
    Users can still safely modify the lossage limit with the
    command `update-lossage-size'.  For convenience, add
    a function `lossage-size' to return the current limit.
   
    * src/keyboard.c (lossage_limit): Do not expose it
    to Lisp; make it a static variable.
    Keep the current Emacs default (300); accept only values >= 100.
    (lossage-size): New function; it returns the current size
    of recent_keys.
    (update-lossage-size):
    Rename it from update-lossage-limit (all callers updated);
    make it a command.
   
    * doc/emacs/help.texi (Misc Help)
    * etc/NEWS
    * lisp/cus-start.el
    * lisp/edmacro.el
    * lisp/help.el:
    Update all references with the new name.
   
    * test/src/keyboard-tests.el (keyboard-lossage-limit):
    Amend the test.

diff --git a/doc/emacs/help.texi b/doc/emacs/help.texi
index e2c0dc6802..26cdf161d5 100644
--- a/doc/emacs/help.texi
+++ b/doc/emacs/help.texi
@@ -563,12 +563,14 @@ Misc Help
 
 @kindex C-h l
 @findex view-lossage
-@vindex lossage-limit
+@findex update-lossage-size
+@findex lossage-size
   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
 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 option @code{lossage-limit}.
+stores the last 300 events; if you wish, you can change this number with
+the command @code{update-lossage-size}.  The function @code{lossage-size}
+returns the current value for the lossage limit.
 If you see commands that you are not familiar with, you can use @kbd{C-h k} or
 @kbd{C-h f} to find out what they do.
 
diff --git a/etc/NEWS b/etc/NEWS
index da204c5825..5fc4cb4d87 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -71,8 +71,9 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 * Changes in Emacs 28.1
 
 +++
-** The new option 'lossage-limit' controls the maximum number
-of keystrokes and commands recorded.
+** The new command 'update-lossage-size' allow users to set the maximum
+number of keystrokes and commands recorded.  The new function 'lossage-size'
+returns the current value of this limit
 
 ** Support for '(box . SIZE)' 'cursor-type'.
 By default, 'box' cursor always has a filled box shape.  But if you
diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index 1ebf554b2b..6632687da4 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -352,8 +352,6 @@ minibuffer-prompt-properties--setter
      ;; indent.c
      (indent-tabs-mode indent boolean)
      ;; keyboard.c
-             (lossage-limit keyboard integer "28.1"
-                            :set (lambda (_ val) (update-lossage-limit val)))
      (meta-prefix-char keyboard character)
      (auto-save-interval auto-save integer)
              (auto-save-no-message auto-save boolean "27.1")
diff --git a/lisp/edmacro.el b/lisp/edmacro.el
index 089fea17ef..52cbf5b5a6 100644
--- a/lisp/edmacro.el
+++ b/lisp/edmacro.el
@@ -35,7 +35,7 @@
 ;;  * `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 `lossage-limit' most recent
+;;  * `C-h l' (view-lossage), to edit the 300 most recent
 ;;    keystrokes and install them as the "current" macro.
 ;;
 ;;  * any key sequence whose definition is a keyboard macro.
@@ -88,7 +88,7 @@ edit-kbd-macro
   "Edit a keyboard macro.
 At the prompt, type any key sequence which is bound to a keyboard macro.
 Or, type `\\[kmacro-end-and-call-macro]' or RET to edit the last
-keyboard macro, `\\[view-lossage]' to edit the last `lossage-limit'
+keyboard macro, `\\[view-lossage]' to edit the last 300
 keystrokes as a keyboard macro, or `\\[execute-extended-command]'
 to edit a macro by its command name.
 With a prefix argument, format the macro in a more concise way."
diff --git a/lisp/help.el b/lisp/help.el
index 9e3d0922d0..01ee739a16 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -459,7 +459,7 @@ view-lossage
   "Display last input keystrokes and the commands run.
 For convenience this uses the same format as
 `edit-last-kbd-macro'.
-See `lossage-limit' to update the number of recorded keystrokes.
+See `update-lossage-size' to update the number of recorded keystrokes.
 
 To record all your input, use `open-dribble-file'."
   (interactive)
diff --git a/src/keyboard.c b/src/keyboard.c
index a4d27c94fb..29b157700c 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -103,6 +103,7 @@ static KBOARD *all_kboards;
 /* True in the single-kboard state, false in the any-kboard state.  */
 static bool single_kboard;
 
+/* Minimum allowed size of the recent_keys vector */
 #define MIN_NUM_RECENT_KEYS (100)
 
 /* Index for storing next element into recent_keys.  */
@@ -111,6 +112,9 @@ static int recent_keys_index;
 /* Total number of elements stored into recent_keys.  */
 static int total_keys;
 
+/* Size of the recent_keys vector */
+static int lossage_limit = 3 * MIN_NUM_RECENT_KEYS;
+
 /* This vector holds the last lossage_limit keystrokes.  */
 static Lisp_Object recent_keys;
 
@@ -10411,6 +10415,15 @@ If CHECK-TIMERS is non-nil, timers that are ready to run will do so.  */)
   ? Qt : Qnil);
 }
 
+DEFUN ("lossage-size", Flossage_size, Slossage_size, 0, 0, 0,
+       doc: /* Return the maximum number of saved keystrokes.
+These are the records shown by `view-lossage'. */ )
+  (void)
+{
+  return make_fixnum(lossage_limit);
+}
+
+/* Reallocate recent_keys copying the keystrokes in the right order */
 static void
 update_recent_keys (int new_size, int kept_keys)
 {
@@ -10433,10 +10446,10 @@ update_recent_keys (int new_size, int kept_keys)
 
 }
 
-/* Reallocate recent_keys copying the keystrokes in the right order */
-DEFUN ("update-lossage-limit", Fupdate_lossage_limit,
-       Supdate_lossage_limit, 1, 1, 0,
+DEFUN ("update-lossage-size", Fupdate_lossage_size,
+       Supdate_lossage_size, 1, 1, "nnew-size: ",
        doc: /* Update the maximum number of saved keystrokes to ARG.
+These are the records shown by `view-lossage'.
 usage: (update-lossage-limit ARG)  */)
   (Lisp_Object arg)
 {
@@ -11736,22 +11749,6 @@ syms_of_keyboard (void)
     staticpro (&modifier_symbols);
   }
 
-  DEFVAR_INT ("lossage-limit", lossage_limit,
-              doc: /* Maximum number of stored keyboard events and commands run.
-
-Please, do not set this variable in Lisp with `setq' neither
-let-bind it, that will likely crash Emacs.  This is because
-`setq' only changes the variable, but it doesn't update
-the size of the internal vector that holds the keystrokes.
-
-To update this variable use the customization menu, or
-call from Lisp the following expression:
-
-  (update-lossage-limit new-limit)
-
-That takes care of both, the variable and the internal vector.*/);
-  lossage_limit = 3 * MIN_NUM_RECENT_KEYS;
-
   recent_keys = make_nil_vector (lossage_limit);
   staticpro (&recent_keys);
 
@@ -11802,7 +11799,8 @@ That takes care of both, the variable and the internal vector.*/);
   defsubr (&Srecursive_edit);
   defsubr (&Sinternal_track_mouse);
   defsubr (&Sinput_pending_p);
-  defsubr (&Supdate_lossage_limit);
+  defsubr (&Slossage_size);
+  defsubr (&Supdate_lossage_size);
   defsubr (&Srecent_keys);
   defsubr (&Sthis_command_keys);
   defsubr (&Sthis_command_keys_vector);
diff --git a/test/src/keyboard-tests.el b/test/src/keyboard-tests.el
index 017d239246..8f2cfe4adc 100644
--- a/test/src/keyboard-tests.el
+++ b/test/src/keyboard-tests.el
@@ -32,15 +32,15 @@
                         (read-event nil nil 2))
                  ?\C-b)))
 
-(ert-deftest keyboard-lossage-limit ()
-  "Test `lossage-limit' updates."
-  (dolist (val (list 100 100 200 500 300 1000 700))
-    (update-lossage-limit val)
-    (should (= val lossage-limit)))
-  (let ((current-limit lossage-limit))
-    (should-error (update-lossage-limit 5))
-    (should-error (update-lossage-limit "200"))
-    (should (= lossage-limit current-limit))))
+(ert-deftest keyboard-lossage-size ()
+  "Test `update-lossage-size'."
+  (dolist (val (list 100 300 400 400 500 1000 700 300))
+    (update-lossage-size val)
+    (should (= val (lossage-size))))
+  (let ((current-size (lossage-size)))
+    (should-error (update-lossage-size 5))
+    (should-error (update-lossage-size "200"))
+    (should (= (lossage-size) current-size))))
 
 
 (provide 'keyboard-tests)

--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 28.0.50 (build 25, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
 of 2020-06-28 built on calancha-pc.dy.bbexcite.jp
Repository revision: e13fd1fc373b35f510b70998bfddbb7af5989912
Repository branch: bug#38796-lossage-limit
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)



Reply | Threaded
Open this post in threaded view
|

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

Drew Adams
> > OP here.
> I was waiting for you.  Welcome!! :-)

Sure, but the rest of your reply is a reply to Stefan's
message, not mine.  And the `>' quoting suggests you're
quoting me in the rest of your message too, but you're
quoting him.