bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

[Drew Adams]

eamcs -Q
 
1. Visit a Dired buffer, then switch back to *scratch*.
 
2. Type this: (substitute-command-keys "\\{dired-mode-map}")

3. Then hit C-j.
 
Look at where you find these entries: `e..f' and `0..9'.
They are out of order.
 
The order provided in all Emacs versions prior to Emacs 23 is
reasonable and user-friendly.  It puts `e..f' among the
lowercase letter, in alphabetical order, i.e., between `d' and
`g'.  It puts `0..9' between `.' and `<', i.e., in ASCII order
(`:' is not bound in Emacs prior to Emacs 23).
 
This was broken first in Emacs 23, which puts `0..9' and `e..f'
first in the list, in that order.
 
Then it was broken differently in Emacs 24, which puts `e..f'
first in the list, but restores `0..9' to its rightful place.
 
Users should be able to find chars/keys in ASCII order.  This is
particularly important for letters.
 

In GNU Emacs 24.1.50.1 (i386-mingw-nt5.1.2600)
 of 2012-04-23 on MARVIN
Bzr revision: 108006
agustin.martin@hispalinux.es-20120423103325-xmra3329elgzhmpc
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
 `configure --with-gcc (4.6) --no-opt --enable-checking --cflags
 -ID:/devel/emacs/libs/libXpm-3.5.8/include
 -ID:/devel/emacs/libs/libXpm-3.5.8/src
 -ID:/devel/emacs/libs/libpng-dev_1.4.3-1/include
 -ID:/devel/emacs/libs/zlib-dev_1.2.5-2/include
 -ID:/devel/emacs/libs/giflib-4.1.4-1/include
 -ID:/devel/emacs/libs/jpeg-6b-4/include
 -ID:/devel/emacs/libs/tiff-3.8.2-1/include
 -ID:/devel/emacs/libs/gnutls-3.0.9/include
 -ID:/devel/emacs/libs/libiconv-1.13.1-1-dev/include
 -ID:/devel/emacs/libs/libxml2-2.7.8/include/libxml2'
 

bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

[Drew Adams]

ping

regression

bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

[Lars Ingebrigtsen]

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

> Then it was broken differently in Emacs 24, which puts `e..f'
> first in the list, but restores `0..9' to its rightful place.

Isn't the main problem here that it even tries to do a two-letter range?
Seems awfully odd to me:

(substitute-command-keys "\\{dired-mode-map}")
"key             binding
---             -------

e .. f		dired-find-file

C-c		Prefix Command
RET		dired-find-file
C-o		dired-display-file
C-t		Prefix Command
ESC		Prefix Command
SPC		dired-next-line
!		dired-do-shell-command
#		dired-flag-auto-save-files
$		dired-hide-subdir
%		Prefix Command
&		dired-do-async-shell-command
(		dired-hide-details-mode
*		Prefix Command
+		dired-create-directory
-		negative-argument
.		dired-clean-directory
0 .. 9		digit-argument


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

bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

[Drew Adams]

> > Then it was broken differently in Emacs 24, which puts `e..f'
> > first in the list, but restores `0..9' to its rightful place.
> 
> Isn't the main problem here that it even tries to do a two-letter
> range?

You could argue that also.  But there is no ambiguity in such a
notation, given that if `e .. f' were a multiple-key sequence
then it would be handled differently (via `Prefix Key').

Anyway, that is not what this bug report is about.  But yes,
if we got rid of that notation then presumably `e' and `f' would
go back to their rightful places alphabetically.

> Seems awfully odd to me:

Being out of order is odd.  That is what this bug thread is about.

bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

[Lars Ingebrigtsen]

This is more mysterious than I thought.  describe_map is responsible for
outputting each map, and I've been staring at it for minutes without
seeing anything odd.

But let's look at the output again:

(substitute-command-keys "\\{dired-mode-map}")
  "key             binding
---             -------

e .. f		dired-find-file

C-c		Prefix Command
RET		dired-find-file
C-o		dired-display-file
[...]
0 .. 9		digit-argument
[...]
c		dired-do-compress-to
d		dired-flag-file-deletion
g		revert-buffer
[...]
S-SPC		dired-previous-line
<follow-link>	mouse-face
<mouse-2>	dired-mouse-find-file-other-window
<remap>		Prefix Command

C-c d		lars-copy-directory

C-t C-t		image-dired-dired-toggle-marked-thumbs
C-t .		image-dired-display-thumb
C-t a		image-dired-display-thumbs-append
C-t c		image-dired-dired-comment-files
C-t d		image-dired-display-thumbs
C-t e		image-dired-dired-edit-comment-and-tags
C-t f		image-dired-mark-tagged-files
C-t i		image-dired-dired-display-image
C-t j		image-dired-jump-thumbnail-buffer

and so on.  The think to observe is that there's an extra newline
after the first "e .. f" line.  This means that it's being output as its
own keymap, I think.  describe_map does not add any extra empty blank
lines, and it sorts ranges just fine, as we can see from the "0 .. 9"
line.

So something is deciding that "e" and "f" come from a separate keymap,
and calling describe_map on that.  Hm...

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

bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> This is more mysterious than I thought.  describe_map is responsible for
> outputting each map, and I've been staring at it for minutes without
> seeing anything odd.
>
> But let's look at the output again:

That was totally wrong.  What happens is that the e .. f is output by
describe_vector, but all the other characters are in the else.

  for (tail = map; CONSP (tail); tail = XCDR (tail))
    {
      QUIT;

      if (VECTORP (XCAR (tail))
	  || CHAR_TABLE_P (XCAR (tail)))
	describe_vector (XCAR (tail),
			 prefix, Qnil, elt_describer, partial, shadow, map,
			 1, mention_shadow);
      else if (CONSP (XCAR (tail)))

For some reason or other.

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

bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

[Lars Ingebrigtsen]

Here's a test case that's easier to work with.

(defvar foo-map
  (let ((map (make-keymap)))
    (define-key map "a" 'left-char)
    (define-key map "f" 'right-char)
    (define-key map "g" 'right-char)
    (define-key map "z" 'left-char)
    map))

This gives the following wrong output:

(insert (substitute-command-keys "\\{foo-map}"))
key             binding
---             -------

f .. g		right-char

a		left-char
z		left-char

If I use a `make-sparse-keymap' instead, I get the following, correct
output:

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

a		left-char
f .. g		right-char
z		left-char

So in non-sparse keymaps, there's something odd about how consecutive
key bindings are represented, and this leaks out in
`substitute-command-keys'.

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

bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> "Drew Adams" <drew.adams@oracle.com> writes:
>
>> Then it was broken differently in Emacs 24, which puts `e..f'
>> first in the list, but restores `0..9' to its rightful place.
>
> Isn't the main problem here that it even tries to do a two-letter range?
> Seems awfully odd to me:

This could be easily fixed using:

diff --git a/src/keymap.c b/src/keymap.c
index ca1dbe368e..1c6f75a767 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -3101,10 +3101,13 @@ describe_vector (Lisp_Object vector,
Lisp_Object prefix, Lisp_Object args,
 	    }
 	}

-      /* If we have a range of more than one character,
-	 print where the range reaches to.  */
+      /* If we have a range of more than two characters, print where
+	 the range reaches to.  We specifically avoid printing two
+	 character ranges, as they aren't very easy on the reader.  */

-      if (i != starting_i)
+      if ((i - starting_i) < 2)
+	i = starting_i;
+      else
 	{
 	  insert (" .. ", 4);

> (substitute-command-keys "\\{dired-mode-map}")
> "key             binding
> ---             -------
>
> e .. f		dired-find-file
>
> C-c		Prefix Command
> RET		dired-find-file
> C-o		dired-display-file
> C-t		Prefix Command
> ESC		Prefix Command
> SPC		dired-next-line
> !		dired-do-shell-command
> #		dired-flag-auto-save-files
> $		dired-hide-subdir
> %		Prefix Command
> &		dired-do-async-shell-command
> (		dired-hide-details-mode
> *		Prefix Command
> +		dired-create-directory
> -		negative-argument
> .		dired-clean-directory
> 0 .. 9		digit-argument

bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

>> Isn't the main problem here that it even tries to do a two-letter range?
>> Seems awfully odd to me:
>
> This could be easily fixed using:

Looks good to me (but I haven't tried the patch).

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

bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

[Lars Ingebrigtsen]

I've now fixed this in Emacs 29 for dired (and other two-character
ranges).  Bigger ranges are still output separately in a block at the
start, but I think that's OK.  (And it's quite rare.)

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

bug#11325: [External] : Re: bug#11325: 24.1.50; regression: bad order for `substitute-command-keys' with keymap

[Drew Adams]

> I've now fixed this in Emacs 29 for dired (and other two-character
> ranges).  Bigger ranges are still output separately in a block at the
> start, but I think that's OK.  (And it's quite rare.)

The question is whether the regression was fixed.
Is the output as sane as what it used to be,
before it was broken?  See the bug report for
more detail.

If the regression isn't fixed then please change
the status to "won't fix".