bug#9293: say what they are shadowed by

bug#9293: say what they are shadowed by

[jidanni]

   Tar mode defined in `tar-mode.el':...
   0 .. 9		digit-argument  (binding currently shadowed)

Shadowed by what?

   e .. f		tar-extract  (binding currently shadowed)

   C-d		tar-flag-deleted
   RET		tar-extract
     (that binding is currently shadowed by another mode)
   C-n		tar-next-line
   C-p		tar-previous-line
   SPC		tar-next-line
     (that binding is currently shadowed by another mode)

Say what mode on each of such lines!

bug#9293: say what they are shadowed by

[Stefan Monnier]

severity 9293 wishlist
thanks

>    Tar mode defined in `tar-mode.el':...
>    0 .. 9		digit-argument  (binding currently shadowed)

> Shadowed by what?

>    e .. f		tar-extract  (binding currently shadowed)

>    C-d		tar-flag-deleted
>    RET		tar-extract
>      (that binding is currently shadowed by another mode)
>    C-n		tar-next-line
>    C-p		tar-previous-line
>    SPC		tar-next-line
>      (that binding is currently shadowed by another mode)

> Say what mode on each of such lines!

That would be nice, indeed.  In this case, I can guess it's the
view-mode bindings, probably because you used `v' from dired to visit
that file.


        Stefan

bug#9293: say what they are shadowed by

[Lennart Borgman]

On Mon, Aug 22, 2011 at 23:29, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> severity 9293 wishlist
> thanks
>
>>    Tar mode defined in `tar-mode.el':...
>>    0 .. 9             digit-argument  (binding currently shadowed)
>
>> Shadowed by what?
>
>>    e .. f             tar-extract  (binding currently shadowed)
>
>>    C-d                tar-flag-deleted
>>    RET                tar-extract
>>      (that binding is currently shadowed by another mode)
>>    C-n                tar-next-line
>>    C-p                tar-previous-line
>>    SPC                tar-next-line
>>      (that binding is currently shadowed by another mode)
>
>> Say what mode on each of such lines!
>
> That would be nice, indeed.

I agree.

I took a look at this quite some time ago now and wrote the command
describe-key-and-map-briefly (which is in ourcomment-utils.el in
nXhtml). This uses some adhod guessing to find the keymap that the
used key is defined in. Perhaps this can be expanded to take care of
the case above too (I am not quite sure at the moment). The command
itself is also useful and might replace describe-key-briefly.

However when writing this command I noticed that there is not enough
information available so some guessing must be done (or the
information must be enhanced somehow, of course).

bug#9293: say what they are shadowed by

[jidanni]

>>>>> "SM" == Stefan Monnier <monnier@iro.umontreal.ca> writes:
SM> That would be nice, indeed.  In this case, I can guess it's the
SM> view-mode bindings, probably because you used `v' from dired to visit
SM> that file.
I guess that is a good guess. But a bad to guess that the users like to guess.

bug#9293: say what they are shadowed by

[Le Wang]

I just wanted to add that I've been bitten by this unhelpful message as well.

I just came across an issue where I made a derived mode based on
diff-mode, but my key bindings were overwridden no matter what I
tried.  Finally I found this bug, and used Lennart's
`describe-key-and-map-briefly', which gave me the information needed
to resolve my issue (I didn't know about
`minor-mode-overriding-map-alist', I should've RTFM).

If the "C-h m" text included this information directly, I would have
avoided a lot of frustration.

Best,
Le

bug#9293: say what they are shadowed by

[Lars Ingebrigtsen]

jidanni@jidanni.org writes:

>    Tar mode defined in `tar-mode.el':...
>    0 .. 9		digit-argument  (binding currently shadowed)
>
> Shadowed by what?
>
>    e .. f		tar-extract  (binding currently shadowed)

[...]

> Say what mode on each of such lines!

This was eight years ago -- has this been fixed in the meantime?

If not -- what are the steps to reproduce this bug?  I'm not getting any
shadowed bindings.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#9293: say what they are shadowed by

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> jidanni@jidanni.org writes:
>
> >    Tar mode defined in `tar-mode.el':...
> >    0 .. 9             digit-argument  (binding currently shadowed)
> >
> > Shadowed by what?
> >
> >    e .. f             tar-extract  (binding currently shadowed)
>
> [...]
>
> > Say what mode on each of such lines!
>
> This was eight years ago -- has this been fixed in the meantime?

Nope.

> If not -- what are the steps to reproduce this bug?  I'm not getting any
> shadowed bindings.

I am working on the branch scratch/substitute-command-keys to replace
s-c-k with a Lisp version, but I have not had much time lately so
there has been no progress in a couple of months. I can't remember the
current status, but I think there may be some tests for this there. Or
I didn't get to that yet, sorry, I can't remember now.

I hope that this would be easier to fix once we have the Lisp version
of s-c-k (and I would personally prefer not to see any changes to the
C version meanwhile ;-)).

Best regards,
Stefan Kangas

bug#9293: say what they are shadowed by

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> I am working on the branch scratch/substitute-command-keys to replace
> s-c-k with a Lisp version, but I have not had much time lately so
> there has been no progress in a couple of months. I can't remember the
> current status, but I think there may be some tests for this there. Or
> I didn't get to that yet, sorry, I can't remember now.
>
> I hope that this would be easier to fix once we have the Lisp version
> of s-c-k (and I would personally prefer not to see any changes to the
> C version meanwhile ;-)).

Yeah, that makes sense.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#9293: say what they are shadowed by

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> jidanni@jidanni.org writes:
>>
>> >    Tar mode defined in `tar-mode.el':...
>> >    0 .. 9             digit-argument  (binding currently shadowed)
>> >
>> > Shadowed by what?
>> >
>> >    e .. f             tar-extract  (binding currently shadowed)
>>
>> [...]

Note that the text here is changed to "(this binding is currently
shadowed)", see Bug#14086.

>>
>> > Say what mode on each of such lines!
>>
>> This was eight years ago -- has this been fixed in the meantime?
>
> Nope.
>
>> If not -- what are the steps to reproduce this bug?  I'm not getting any
>> shadowed bindings.
>
> I am working on the branch scratch/substitute-command-keys to replace
> s-c-k with a Lisp version, but I have not had much time lately so
> there has been no progress in a couple of months. I can't remember the
> current status, but I think there may be some tests for this there. Or
> I didn't get to that yet, sorry, I can't remember now.
>
> I hope that this would be easier to fix once we have the Lisp version
> of s-c-k

Contrary to my hopes this part was in fact _not_ converted to Lisp with
the merge of scratch/substitute-command-keys.  This was due to poor
performance of my Lisp version of that code.  So this should be as easy
or hard as it was before to fix.

The text comes from describe_map in keymap.c (at the very end of the
function) if someone wants to take a stab at improving this.

bug#9293: (no subject)

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Contrary to my hopes this part was in fact _not_ converted to Lisp with
> the merge of scratch/substitute-command-keys.  This was due to poor
> performance of my Lisp version of that code.  So this should be as easy
> or hard as it was before to fix.

Right.

So, here's the test case (since one wasn't given in the original bug
report):

C-x C-f file.tgz RET
M-x view-mode RET
C-h m

Notice

key             binding
---             -------

0 .. 9		digit-argument  (binding currently shadowed)
e .. f		tar-extract  (binding currently shadowed)

This is both confusing and...  wrong?

`C-h c e' does say "e runs the command View-exit", but
`C-h c f' says "f runs the command tar-extract".

And all of keys `0' through `9' really do call `digit-argument', so that
one is just totally wrong.

So there's a bug here, and an improvement request in addition.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#9293:

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> So, here's the test case (since one wasn't given in the original bug
> report):
>
> C-x C-f file.tgz RET
> M-x view-mode RET
> C-h m
>
> Notice
>
> key             binding
> ---             -------
>
> 0 .. 9		digit-argument  (binding currently shadowed)
> e .. f		tar-extract  (binding currently shadowed)
>
> This is both confusing and...  wrong?
>
> `C-h c e' does say "e runs the command View-exit", but
> `C-h c f' says "f runs the command tar-extract".

I think there are two issues:

1. If you don't enable `view-mode', you will see that in `tar-mode-map'
   both `e' and `f' are bound to `tar-extract'.

   However, when you enable `view-mode', only `e' is shadowed.

> And all of keys `0' through `9' really do call `digit-argument', so that
> one is just totally wrong.

2. `0 .. 9' are technically shadowed, in the sense that there is a
   binding for them in an overriding keymap.  Only that in this case the
   binding in the other keymap is the exact same command.

So in conclusion the output here should probably be:

  0 .. 9	digit-argument
  e		tar-extract  (binding currently shadowed)
  f		tar-extract

bug#9293: say what they are shadowed by

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

tags 9293 + patch
thanks

Stefan Kangas <stefan@marxist.se> writes:

> So in conclusion the output here should probably be:
>
>   0 .. 9	digit-argument
>   e		tar-extract  (binding currently shadowed)
>   f		tar-extract

I've attached three patches.  The first fixes the bug with broken
ranges, the second one fixes the incorrect shadowing by the same
command, and the third one takes care of the feature request.

Using the original recipe, I now get:

0 .. 9		digit-argument
e		tar-extract  (currently shadowed by ‘View-exit’)
f		tar-extract

Comments?

[-- Attachment #2: 0001-Don-t-show-key-ranges-if-shadowed-by-different-comma.patch --]
[-- Type: text/x-diff, Size: 3580 bytes --]

From ecc913fe599488c1878118de4283fd6134ff50cf Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Fri, 13 Nov 2020 15:28:29 +0100
Subject: [PATCH 1/3] Don't show key ranges if shadowed by different commands

* src/keymap.c (describe_vector): Make sure found consecutive keys
are either not shadowed or, if they are, that they are shadowed by
the same command.  (Bug#9293)
* test/src/keymap-tests.el
(help--describe-vector/bug-9293-one-shadowed-in-range): New test.
---
 src/keymap.c             | 22 ++++++++++++++++++----
 test/src/keymap-tests.el | 27 +++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/src/keymap.c b/src/keymap.c
index 181dcdad3a..4616f27c25 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -3085,6 +3085,7 @@ describe_vector (Lisp_Object vector, Lisp_Object prefix, Lisp_Object args,
   for (i = from; ; i++)
     {
       bool this_shadowed = 0;
+      Lisp_Object shadowed_by = NULL;
       int range_beg, range_end;
       Lisp_Object val;
 
@@ -3127,11 +3128,9 @@ describe_vector (Lisp_Object vector, Lisp_Object prefix, Lisp_Object args,
       /* If this binding is shadowed by some other map, ignore it.  */
       if (!NILP (shadow))
 	{
-	  Lisp_Object tem;
-
-	  tem = shadow_lookup (shadow, kludge, Qt, 0);
+	  shadowed_by = shadow_lookup (shadow, kludge, Qt, 0);
 
-	  if (!NILP (tem))
+	  if (!NILP (shadowed_by))
 	    {
 	      if (mention_shadow)
 		this_shadowed = 1;
@@ -3186,6 +3185,21 @@ describe_vector (Lisp_Object vector, Lisp_Object prefix, Lisp_Object args,
 	       && !NILP (Fequal (tem2, definition)))
 	  i++;
 
+      /* Make sure found consecutive keys are either not shadowed or,
+	 if they are, that they are shadowed by the same command.  */
+      if (CHAR_TABLE_P (vector) && i != starting_i)
+	{
+	  Lisp_Object tem;
+	  Lisp_Object key = make_nil_vector (1);
+	  for (int j = starting_i + 1; j <= i; j++)
+	    {
+	      ASET (key, 0, make_fixnum (j));
+	      tem = shadow_lookup (shadow, key, Qt, 0);
+	      if (NILP (Fequal (tem, shadowed_by)))
+		i = j - 1;
+	    }
+	}
+
       /* If we have a range of more than one character,
 	 print where the range reaches to.  */
 
diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
index e3dd8420d7..b230958b36 100644
--- a/test/src/keymap-tests.el
+++ b/test/src/keymap-tests.el
@@ -170,6 +170,33 @@ keymap-where-is-internal/preferred-modifier-is-a-string
             (where-is-internal 'execute-extended-command global-map t))
           [#x8000078])))
 
+\f
+;;;; describe_vector
+
+(ert-deftest help--describe-vector/bug-9293-one-shadowed-in-range ()
+  "Check that we only show a range if shadowed by the same command."
+  (let ((orig-map (let ((map (make-keymap)))
+                    (define-key map "e" 'foo)
+                    (define-key map "f" 'foo)
+                    (define-key map "g" 'foo)
+                    (define-key map "h" 'foo)
+                    map))
+        (shadow-map (let ((map (make-keymap)))
+                      (define-key map "f" 'bar)
+                      map)))
+    (with-temp-buffer
+      (help--describe-vector (cadr orig-map) nil #'help--describe-command
+                             t shadow-map orig-map t)
+      (should (equal (buffer-string)
+                     "
+e		foo
+f		foo  (binding currently shadowed)
+g .. h		foo
+")))))
+
+\f
+;;;; apropos-internal
+
 (ert-deftest keymap-apropos-internal ()
   (should (equal (apropos-internal "^next-line$") '(next-line)))
   (should (>= (length (apropos-internal "^help")) 100))
-- 
2.29.2


[-- Attachment #3: 0002-Don-t-shadow-bindings-by-the-same-command.patch --]
[-- Type: text/x-diff, Size: 2145 bytes --]

From 4ef177d0133d1e97b13b8dcfe98c3093ac02d897 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Fri, 13 Nov 2020 15:28:34 +0100
Subject: [PATCH 2/3] Don't shadow bindings by the same command

* src/keymap.c (describe_vector): Do not say binding is shadowed if
the other key binding points to the same command.  (Bug#9293)
* test/src/keymap-tests.el
(help--describe-vector/bug-9293-same-command-does-not-shadow): New
test.
---
 src/keymap.c             |  2 +-
 test/src/keymap-tests.el | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/src/keymap.c b/src/keymap.c
index 4616f27c25..174ee4fd50 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -3130,7 +3130,7 @@ describe_vector (Lisp_Object vector, Lisp_Object prefix, Lisp_Object args,
 	{
 	  shadowed_by = shadow_lookup (shadow, kludge, Qt, 0);
 
-	  if (!NILP (shadowed_by))
+	  if (!NILP (shadowed_by) && !EQ (shadowed_by, definition))
 	    {
 	      if (mention_shadow)
 		this_shadowed = 1;
diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
index b230958b36..340227311c 100644
--- a/test/src/keymap-tests.el
+++ b/test/src/keymap-tests.el
@@ -194,6 +194,30 @@ help--describe-vector/bug-9293-one-shadowed-in-range
 g .. h		foo
 ")))))
 
+(ert-deftest help--describe-vector/bug-9293-same-command-does-not-shadow ()
+  "Check that a command can't be shadowed by the same command."
+  (let ((range-map
+         (let ((map (make-keymap)))
+           (define-key map "0" 'foo)
+           (define-key map "1" 'foo)
+           (define-key map "2" 'foo)
+           (define-key map "3" 'foo)
+           map))
+        (shadow-map
+         (let ((map (make-keymap)))
+           (define-key map "0" 'foo)
+           (define-key map "1" 'foo)
+           (define-key map "2" 'foo)
+           (define-key map "3" 'foo)
+           map)))
+   (with-temp-buffer
+     (help--describe-vector (cadr range-map) nil #'help--describe-command
+                            t shadow-map range-map t)
+     (should (equal (buffer-string)
+                    "
+0 .. 3		foo
+")))))
+
 \f
 ;;;; apropos-internal
 
-- 
2.29.2


[-- Attachment #4: 0003-Say-which-command-shadows-a-key-binding.patch --]
[-- Type: text/x-diff, Size: 1953 bytes --]

From 8761a55b751cf2a208496d3441e35c9699127732 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Fri, 13 Nov 2020 19:15:21 +0100
Subject: [PATCH 3/3] Say which command shadows a key binding

* src/keymap.c (describe_vector): Say which command shadows this
binding.  (Bug#9293)
* test/src/keymap-tests.el
(help--describe-vector/bug-9293-one-shadowed-in-range): Adapt
test.
---
 src/keymap.c             | 8 +++++++-
 test/src/keymap-tests.el | 5 +++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/keymap.c b/src/keymap.c
index 174ee4fd50..72141bdd5a 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -3223,7 +3223,13 @@ describe_vector (Lisp_Object vector, Lisp_Object prefix, Lisp_Object args,
       if (this_shadowed)
 	{
 	  SET_PT (PT - 1);
-	  insert_string ("  (binding currently shadowed)");
+	  static char const fmt[] = "  (currently shadowed by `%s')";
+	  USE_SAFE_ALLOCA;
+	  char *buffer = SAFE_ALLOCA (sizeof fmt +
+				      SBYTES (SYMBOL_NAME (shadowed_by)));
+	  esprintf (buffer, fmt, SDATA (SYMBOL_NAME (shadowed_by)));
+	  insert_string (buffer);
+	  SAFE_FREE();
 	  SET_PT (PT + 1);
 	}
     }
diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
index 340227311c..f62589f74b 100644
--- a/test/src/keymap-tests.el
+++ b/test/src/keymap-tests.el
@@ -183,14 +183,15 @@ help--describe-vector/bug-9293-one-shadowed-in-range
                     map))
         (shadow-map (let ((map (make-keymap)))
                       (define-key map "f" 'bar)
-                      map)))
+                      map))
+        (text-quoting-style 'grave))
     (with-temp-buffer
       (help--describe-vector (cadr orig-map) nil #'help--describe-command
                              t shadow-map orig-map t)
       (should (equal (buffer-string)
                      "
 e		foo
-f		foo  (binding currently shadowed)
+f		foo  (currently shadowed by `bar')
 g .. h		foo
 ")))))
 
-- 
2.29.2

bug#9293: say what they are shadowed by

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> I've attached three patches.  The first fixes the bug with broken
> ranges, the second one fixes the incorrect shadowing by the same
> command, and the third one takes care of the feature request.
>
> Using the original recipe, I now get:
>
> 0 .. 9		digit-argument
> e		tar-extract  (currently shadowed by ‘View-exit’)
> f		tar-extract
>
> Comments?

Looks perfect; go ahead and push.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#9293: say what they are shadowed by

[Stefan Kangas]

tags 9293 fixed
close 9293 28.1
thanks

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> I've attached three patches.  The first fixes the bug with broken
>> ranges, the second one fixes the incorrect shadowing by the same
>> command, and the third one takes care of the feature request.
>>
>> Using the original recipe, I now get:
>>
>> 0 .. 9		digit-argument
>> e		tar-extract  (currently shadowed by ‘View-exit’)
>> f		tar-extract
>>
>> Comments?
>
> Looks perfect; go ahead and push.

Thanks.

No further comments within a week; pushed to master and closing.