Please add a lossage-limit option for `view-lossage'

Please add a lossage-limit option for `view-lossage'

[Drew Adams]

This thread from 2006 was never followed up, AFAICT:

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

The last post in it was this, from RMS:

    >> I won't recommend such a change for before the
    >> release, but that's just me.
    >
    > I don't see an urgent need, but it would be a
    > trivial change.

    I am glad it is so simple.  I'd still rather save
    it for after the release, but at that time, please
    make the change.

The request (and thus the change that RMS asked to be
made) was this:

  Any chance that the number of keystrokes displayed
  by `view-lossage', currently hard-coded at 100,
  could be made a user option?
                  ^^^^^^^^^^^

Since then, the hard-coded limit was upped to 300.
But there still is no user option, is there?  Could
we please add an option for this now?  The code is
in C - it's not simple for a user to change the
value without a Lisp variable for it.

Re: Please add a lossage-limit option for `view-lossage'

[Bruno Félix Rezende Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 2255 bytes --]

Hello Drew,

Drew Adams <drew.adams@oracle.com> writes:

> The request (and thus the change that RMS asked to be
> made) was this:
>   Any chance that the number of keystrokes displayed
>   by `view-lossage', currently hard-coded at 100,
>   could be made a user option?
>                   ^^^^^^^^^^^
> Since then, the hard-coded limit was upped to 300.
> But there still is no user option, is there?  Could
> we please add an option for this now?  The code is
> in C - it's not simple for a user to change the
> value without a Lisp variable for it.

I took this task as a goal in my first attempt to write code to Emacs C
core.  What I could get for now is in the patch attached.  It seems to
work fine for ‘num-recent-keys’[1] strictly positive, but it segfaults
promptly for other values (understandably).

What I did was to replace the C macro constant ‘NUM_RECENT_KEYS’ , with
the Lisp-visible variable ‘num-recent-keys’, defined using ‘DEFVAR_INT’,
and then define a C function called ‘ensure_recent_keys_size’ to keep
the size of the vector ‘recent-keys’ truthful to that; shrinking or
enlarging the vector.[2]

The problem is, I had to place calls to ‘ensure_recent_keys_size’
scattered throughout the source --- strategically before references to
‘num_recent_keys’.  This is clearly not the right approach.  What is
needed is a way to intercept changes to that variable and do the job
once, before anything else happens.  I think this would fix my fragile
hack.  Furthermore, that interception routine could prevent values
smaller than 1 --- set from the Lisp world --- to be rejected (not only
non-integers), hopefully preventing the crashes I’m experiencing now.

In the Lisp reference documentation, I was unable to find a way to do
that.  I thought that if that mechanism existed it’d be important enough
to be documented.  I couldn’t find it in a quick look around the code
neither.  How is the interception of change of variables from the Lisp
world canonically handled by Emacs C source code?


Footnotes: 

[1]  This is the requested user option.

[2] Actually creating another vector of the right size and copying the
    data over as needed.



[-- Attachment #1.2: 0001-Transform-C-constant-NUM_RECENT_KEYS-into-num-recent.patch --]
[-- Type: text/x-diff, Size: 7087 bytes --]

From f87911cf8f6d12046f25e2964bbbdbcd3ca0581d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bruno=20F=C3=A9lix=20Rezende=20Ribeiro?= <oitofelix@gnu.org>
Date: Mon, 6 Apr 2020 08:30:16 -0300
Subject: [PATCH] =?UTF-8?q?Transform=20C=20constant=20=E2=80=98NUM=5FRECEN?=
 =?UTF-8?q?T=5FKEYS=E2=80=99=20into=20=E2=80=98num-recent-keys=E2=80=99=20?=
 =?UTF-8?q?Lisp=20variable?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/keyboard.c: remove NUM_RECENT_KEYS constant.
  (ensure_recent_keys_size): new function.
  (command_loop_1, record_char, recent-keys): add calls to
  ‘ensure_recent_key_size’.
  (syms_of_keyboard): add ‘num-recent-keys’ Lisp integer variable.
* lisp/cus-start.el: add ‘num-recent-keys’.
---
 lisp/cus-start.el |  1 +
 src/keyboard.c    | 62 ++++++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index dff4d9a..001f79b 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -353,6 +353,7 @@ minibuffer-prompt-properties--setter
 	     (indent-tabs-mode indent boolean)
 	     ;; keyboard.c
 	     (meta-prefix-char keyboard character)
+             (num-recent-keys keyboard integer)
 	     (auto-save-interval auto-save integer)
              (auto-save-no-message auto-save boolean "27.1")
 	     (auto-save-timeout auto-save (choice (const :tag "off" nil)
diff --git a/src/keyboard.c b/src/keyboard.c
index 9ce168c..cbbb572 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -103,15 +103,13 @@ Copyright (C) 1985-1989, 1993-1997, 1999-2020 Free Software Foundation,
 /* True in the single-kboard state, false in the any-kboard state.  */
 static bool single_kboard;
 
-#define NUM_RECENT_KEYS (300)
-
 /* Index for storing next element into recent_keys.  */
 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 num_recent_keys keystrokes.  */
 static Lisp_Object recent_keys;
 
 /* Vector holding the key sequence that invoked the current command.
@@ -371,6 +369,7 @@ #define READABLE_EVENTS_IGNORE_SQUEEZABLES	(1 << 2)
 static void deliver_user_signal (int);
 static char *find_user_signal_name (int);
 static void store_user_signal_events (void);
+static void ensure_recent_keys_size (void);
 
 /* Advance or retreat a buffered input event pointer.  */
 
@@ -448,6 +447,20 @@ kset_system_key_syms (struct kboard *kb, Lisp_Object val)
 }
 
 \f
+void
+ensure_recent_keys_size (void)
+{
+  if (ASIZE (recent_keys) != num_recent_keys)
+    {
+      Lisp_Object new_recent_keys = make_nil_vector (num_recent_keys);
+      memcpy (XVECTOR (new_recent_keys)->contents, XVECTOR (recent_keys)->contents,
+              min (ASIZE (recent_keys), num_recent_keys)
+              * sizeof *XVECTOR (recent_keys)->contents);
+      recent_keys = new_recent_keys;
+    }
+}
+
+\f
 static bool
 echo_keystrokes_p (void)
 {
@@ -1421,10 +1434,11 @@ command_loop_1 (void)
       /* Execute the command.  */
 
       {
-	total_keys += total_keys < NUM_RECENT_KEYS;
+        ensure_recent_keys_size ();
+	total_keys += total_keys < num_recent_keys;
 	ASET (recent_keys, recent_keys_index,
 	      Fcons (Qnil, cmd));
-	if (++recent_keys_index >= NUM_RECENT_KEYS)
+	if (++recent_keys_index >= num_recent_keys)
 	  recent_keys_index = 0;
       }
       Vthis_command = cmd;
@@ -3249,16 +3263,18 @@ record_char (Lisp_Object c)
       Lisp_Object ev1, ev2, ev3;
       int ix1, ix2, ix3;
 
+      ensure_recent_keys_size ();
+
       if ((ix1 = recent_keys_index - 1) < 0)
-	ix1 = NUM_RECENT_KEYS - 1;
+	ix1 = num_recent_keys - 1;
       ev1 = AREF (recent_keys, ix1);
 
       if ((ix2 = ix1 - 1) < 0)
-	ix2 = NUM_RECENT_KEYS - 1;
+	ix2 = num_recent_keys - 1;
       ev2 = AREF (recent_keys, ix2);
 
       if ((ix3 = ix2 - 1) < 0)
-	ix3 = NUM_RECENT_KEYS - 1;
+	ix3 = num_recent_keys - 1;
       ev3 = AREF (recent_keys, ix3);
 
       if (EQ (XCAR (c), Qhelp_echo))
@@ -3309,12 +3325,13 @@ record_char (Lisp_Object c)
     {
       if (!recorded)
 	{
-	  total_keys += total_keys < NUM_RECENT_KEYS;
+          ensure_recent_keys_size ();
+	  total_keys += total_keys < num_recent_keys;
 	  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 >= num_recent_keys)
 	    recent_keys_index = 0;
 	}
       else if (recorded < 0)
@@ -3328,10 +3345,11 @@ record_char (Lisp_Object c)
 
 	  while (recorded++ < 0 && total_keys > 0)
 	    {
-	      if (total_keys < NUM_RECENT_KEYS)
+              ensure_recent_keys_size ();
+	      if (total_keys < num_recent_keys)
 		total_keys--;
 	      if (--recent_keys_index < 0)
-		recent_keys_index = NUM_RECENT_KEYS - 1;
+		recent_keys_index = num_recent_keys - 1;
 	      ASET (recent_keys, recent_keys_index, Qnil);
 	    }
 	}
@@ -10420,22 +10438,24 @@ DEFUN ("recent-keys", Frecent_keys, Srecent_keys, 0, 1, 0,
 {
   bool cmds = !NILP (include_cmds);
 
+  ensure_recent_keys_size ();
+
   if (!total_keys
-      || (cmds && total_keys < NUM_RECENT_KEYS))
+      || (cmds && total_keys < num_recent_keys))
     return Fvector (total_keys,
 		    XVECTOR (recent_keys)->contents);
   else
     {
       Lisp_Object es = Qnil;
-      int i = (total_keys < NUM_RECENT_KEYS
+      int i = (total_keys < num_recent_keys
 	       ? 0 : recent_keys_index);
-      eassert (recent_keys_index < NUM_RECENT_KEYS);
+      eassert (recent_keys_index < num_recent_keys);
       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 >= num_recent_keys)
 	    i = 0;
 	} while (i != recent_keys_index);
       es = Fnreverse (es);
@@ -11690,9 +11710,6 @@ syms_of_keyboard (void)
     staticpro (&modifier_symbols);
   }
 
-  recent_keys = make_nil_vector (NUM_RECENT_KEYS);
-  staticpro (&recent_keys);
-
   this_command_keys = make_nil_vector (40);
   staticpro (&this_command_keys);
 
@@ -11858,6 +11875,13 @@ syms_of_keyboard (void)
 result of looking up the original command in the active keymaps.  */);
   Vthis_original_command = Qnil;
 
+  DEFVAR_INT ("num-recent-keys", num_recent_keys,
+	      doc: /* Number of input events to consider. */);
+  num_recent_keys = 300;
+
+  recent_keys = make_nil_vector (num_recent_keys);
+  staticpro (&recent_keys);
+
   DEFVAR_INT ("auto-save-interval", auto_save_interval,
 	      doc: /* Number of input events between auto-saves.
 Zero means disable autosaving due to number of characters typed.  */);
-- 
2.7.4


[-- Attachment #1.3: Type: text/plain, Size: 97 bytes --]


-- 
Bruno Félix Rezende Ribeiro (oitofelix) [0x28D618AF]
<http://oitofelix.freeshell.org/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 454 bytes --]

RE: Please add a lossage-limit option for `view-lossage'

[Drew Adams]

Hi Bruno,

Thanks very much for working on this.  I'm sorry,
but I can't comment on the proposed fix etc. in C.
Hopefully someone else will help in that regard.

Thx - Drew

Re: Please add a lossage-limit option for `view-lossage'

[Stefan Monnier]

Hi,

> I took this task as a goal in my first attempt to write code to Emacs C core.

Very good choice!

> In the Lisp reference documentation, I was unable to find a way to do
> that.  I thought that if that mechanism existed it’d be important enough
> to be documented.  I couldn’t find it in a quick look around the code
> neither.  How is the interception of change of variables from the Lisp
> world canonically handled by Emacs C source code?

Indeed, there's no such canonical thing.

One way this kind of problem is solved sometimes is by not exporting the
variable to Lisp, and instead only providing access to the variable via
functions to read and/or set it.

But here's how I'd recommend you solve this specific case:

1- consolidate the (apprently 3) places in the current code that add an
   element to the vector into its own function.  That's a good change in
   itself, will make the code shorter (by reducing duplication) and
   more readable.

2- Replace the NUM_RECENT_KEYS constant with a num_recent_keys variable
   (tho maybe we can just replace the var with `ASIZE (recent_keys)`
   instead!).

3- Add a new Lisp variable which we could call `Vnum_recent_keys`, tho
   maybe I'd rather make it use the `recent-key` prefix, so it could be
   `Vrecent_key_size`?
   [ Notice that, by convention, Lisp variables in the C world are
     prefixed with a `V`.  ]

4- Synchronize this new var with `num_recent_keys` at just one place:
   inside the function introduced at step 1.  This means that calling
   `recent-keys` just after setting the var will not reflect the new
   value of the variable, but nobody cares about that, I think.


        Stefan


> Footnotes: 
>
> [1]  This is the requested user option.
>
> [2] Actually creating another vector of the right size and copying the
>     data over as needed.
>
>
> From f87911cf8f6d12046f25e2964bbbdbcd3ca0581d Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Bruno=20F=C3=A9lix=20Rezende=20Ribeiro?= <oitofelix@gnu.org>
> Date: Mon, 6 Apr 2020 08:30:16 -0300
> Subject: [PATCH] =?UTF-8?q?Transform=20C=20constant=20=E2=80=98NUM=5FRECEN?=
>  =?UTF-8?q?T=5FKEYS=E2=80=99=20into=20=E2=80=98num-recent-keys=E2=80=99=20?=
>  =?UTF-8?q?Lisp=20variable?=
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> * src/keyboard.c: remove NUM_RECENT_KEYS constant.
>   (ensure_recent_keys_size): new function.
>   (command_loop_1, record_char, recent-keys): add calls to
>   ‘ensure_recent_key_size’.
>   (syms_of_keyboard): add ‘num-recent-keys’ Lisp integer variable.
> * lisp/cus-start.el: add ‘num-recent-keys’.
> ---
>  lisp/cus-start.el |  1 +
>  src/keyboard.c    | 62 ++++++++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 44 insertions(+), 19 deletions(-)
>
> diff --git a/lisp/cus-start.el b/lisp/cus-start.el
> index dff4d9a..001f79b 100644
> --- a/lisp/cus-start.el
> +++ b/lisp/cus-start.el
> @@ -353,6 +353,7 @@ minibuffer-prompt-properties--setter
>  	     (indent-tabs-mode indent boolean)
>  	     ;; keyboard.c
>  	     (meta-prefix-char keyboard character)
> +             (num-recent-keys keyboard integer)
>  	     (auto-save-interval auto-save integer)
>               (auto-save-no-message auto-save boolean "27.1")
>  	     (auto-save-timeout auto-save (choice (const :tag "off" nil)
> diff --git a/src/keyboard.c b/src/keyboard.c
> index 9ce168c..cbbb572 100644
> --- a/src/keyboard.c
> +++ b/src/keyboard.c
> @@ -103,15 +103,13 @@ Copyright (C) 1985-1989, 1993-1997, 1999-2020 Free Software Foundation,
>  /* True in the single-kboard state, false in the any-kboard state.  */
>  static bool single_kboard;
>  
> -#define NUM_RECENT_KEYS (300)
> -
>  /* Index for storing next element into recent_keys.  */
>  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 num_recent_keys keystrokes.  */
>  static Lisp_Object recent_keys;
>  
>  /* Vector holding the key sequence that invoked the current command.
> @@ -371,6 +369,7 @@ #define READABLE_EVENTS_IGNORE_SQUEEZABLES	(1 << 2)
>  static void deliver_user_signal (int);
>  static char *find_user_signal_name (int);
>  static void store_user_signal_events (void);
> +static void ensure_recent_keys_size (void);
>  
>  /* Advance or retreat a buffered input event pointer.  */
>  
> @@ -448,6 +447,20 @@ kset_system_key_syms (struct kboard *kb, Lisp_Object val)
>  }
>  
>  \f
> +void
> +ensure_recent_keys_size (void)
> +{
> +  if (ASIZE (recent_keys) != num_recent_keys)
> +    {
> +      Lisp_Object new_recent_keys = make_nil_vector (num_recent_keys);
> +      memcpy (XVECTOR (new_recent_keys)->contents, XVECTOR (recent_keys)->contents,
> +              min (ASIZE (recent_keys), num_recent_keys)
> +              * sizeof *XVECTOR (recent_keys)->contents);
> +      recent_keys = new_recent_keys;
> +    }
> +}
> +
> +\f
>  static bool
>  echo_keystrokes_p (void)
>  {
> @@ -1421,10 +1434,11 @@ command_loop_1 (void)
>        /* Execute the command.  */
>  
>        {
> -	total_keys += total_keys < NUM_RECENT_KEYS;
> +        ensure_recent_keys_size ();
> +	total_keys += total_keys < num_recent_keys;
>  	ASET (recent_keys, recent_keys_index,
>  	      Fcons (Qnil, cmd));
> -	if (++recent_keys_index >= NUM_RECENT_KEYS)
> +	if (++recent_keys_index >= num_recent_keys)
>  	  recent_keys_index = 0;
>        }
>        Vthis_command = cmd;
> @@ -3249,16 +3263,18 @@ record_char (Lisp_Object c)
>        Lisp_Object ev1, ev2, ev3;
>        int ix1, ix2, ix3;
>  
> +      ensure_recent_keys_size ();
> +
>        if ((ix1 = recent_keys_index - 1) < 0)
> -	ix1 = NUM_RECENT_KEYS - 1;
> +	ix1 = num_recent_keys - 1;
>        ev1 = AREF (recent_keys, ix1);
>  
>        if ((ix2 = ix1 - 1) < 0)
> -	ix2 = NUM_RECENT_KEYS - 1;
> +	ix2 = num_recent_keys - 1;
>        ev2 = AREF (recent_keys, ix2);
>  
>        if ((ix3 = ix2 - 1) < 0)
> -	ix3 = NUM_RECENT_KEYS - 1;
> +	ix3 = num_recent_keys - 1;
>        ev3 = AREF (recent_keys, ix3);
>  
>        if (EQ (XCAR (c), Qhelp_echo))
> @@ -3309,12 +3325,13 @@ record_char (Lisp_Object c)
>      {
>        if (!recorded)
>  	{
> -	  total_keys += total_keys < NUM_RECENT_KEYS;
> +          ensure_recent_keys_size ();
> +	  total_keys += total_keys < num_recent_keys;
>  	  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 >= num_recent_keys)
>  	    recent_keys_index = 0;
>  	}
>        else if (recorded < 0)
> @@ -3328,10 +3345,11 @@ record_char (Lisp_Object c)
>  
>  	  while (recorded++ < 0 && total_keys > 0)
>  	    {
> -	      if (total_keys < NUM_RECENT_KEYS)
> +              ensure_recent_keys_size ();
> +	      if (total_keys < num_recent_keys)
>  		total_keys--;
>  	      if (--recent_keys_index < 0)
> -		recent_keys_index = NUM_RECENT_KEYS - 1;
> +		recent_keys_index = num_recent_keys - 1;
>  	      ASET (recent_keys, recent_keys_index, Qnil);
>  	    }
>  	}
> @@ -10420,22 +10438,24 @@ DEFUN ("recent-keys", Frecent_keys, Srecent_keys, 0, 1, 0,
>  {
>    bool cmds = !NILP (include_cmds);
>  
> +  ensure_recent_keys_size ();
> +
>    if (!total_keys
> -      || (cmds && total_keys < NUM_RECENT_KEYS))
> +      || (cmds && total_keys < num_recent_keys))
>      return Fvector (total_keys,
>  		    XVECTOR (recent_keys)->contents);
>    else
>      {
>        Lisp_Object es = Qnil;
> -      int i = (total_keys < NUM_RECENT_KEYS
> +      int i = (total_keys < num_recent_keys
>  	       ? 0 : recent_keys_index);
> -      eassert (recent_keys_index < NUM_RECENT_KEYS);
> +      eassert (recent_keys_index < num_recent_keys);
>        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 >= num_recent_keys)
>  	    i = 0;
>  	} while (i != recent_keys_index);
>        es = Fnreverse (es);
> @@ -11690,9 +11710,6 @@ syms_of_keyboard (void)
>      staticpro (&modifier_symbols);
>    }
>  
> -  recent_keys = make_nil_vector (NUM_RECENT_KEYS);
> -  staticpro (&recent_keys);
> -
>    this_command_keys = make_nil_vector (40);
>    staticpro (&this_command_keys);
>  
> @@ -11858,6 +11875,13 @@ syms_of_keyboard (void)
>  result of looking up the original command in the active keymaps.  */);
>    Vthis_original_command = Qnil;
>  
> +  DEFVAR_INT ("num-recent-keys", num_recent_keys,
> +	      doc: /* Number of input events to consider. */);
> +  num_recent_keys = 300;
> +
> +  recent_keys = make_nil_vector (num_recent_keys);
> +  staticpro (&recent_keys);
> +
>    DEFVAR_INT ("auto-save-interval", auto_save_interval,
>  	      doc: /* Number of input events between auto-saves.
>  Zero means disable autosaving due to number of characters typed.  */);
> -- 
> 2.7.4