GNU bug report logs - #12717
24.2.50; [PATCH] `imenu--split-submenus' incorrectly distinguishes submenus

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 12717 in the body.
You can then email your comments to 12717 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: <bug-gnu-emacs <at> gnu.org>
Subject: 24.2.50;
	[PATCH] `imenu--split-submenus' incorrectly distinguishes submenus
Date: Tue, 23 Oct 2012 17:07:11 -0700

Imenu allows the use of something it calls "special" menu elements,
which have this form: (INDEX-NAME POSITION FUNCTION ARGUMENTS...)
 
`imenu--split-submenus' needs to distinguish submenus from ordinary menu
elements, including from special menu elements.  Currently this is
bugged.
 
The current code for `imenu--split-submenus' does this:
 
(defun imenu--split-submenus (alist)
  "..."
  (mapcar (lambda (elt)
            (if (and (consp elt) (stringp (car elt)) (listp (cdr elt)))
                (imenu--split-menu (cdr elt) (car elt))
              elt))
   alist))
 
The `if' condition should instead test whether the element is a submenu.
We have a function that does that: `imenu--subalist-p'.  I believe this
is the correct code:
 
(defun imenu--split-submenus (alist)
  "..."
  (mapcar (lambda (elt)
            (if (imenu--subalist-p elt)
                (imenu--split-menu (cdr elt) (car elt))
              elt))
   alist))

In GNU Emacs 24.2.50.1 (i386-mingw-nt5.1.2600)
 of 2012-10-22 on DANI-PC
Bzr revision: 110618 monnier <at> iro.umontreal.ca-20121022132928-232zm0fecassmhfb
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
 `configure --with-gcc (4.7) --no-opt --enable-checking --cflags
 -I../../libs/libxpm-3.5.8/include -I../../libs/libxpm-3.5.8/src
 -I../../libs/libpng-1.4.10 -I../../libs/zlib-1.2.6
 -I../../libs/giflib-4.1.4-1/include -I../../libs/jpeg-6b-4/include
 -I../../libs/tiff-3.8.2-1/include
 -I../../libs/libxml2-2.7.8-w32-bin/include/libxml2
 -I../../libs/gnutls-3.0.16/include
 -I../../libs/libiconv-1.14-2-mingw32-dev/include'
 

Message #8 received at 12717 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: "Drew Adams" <drew.adams <at> oracle.com>
Cc: 12717 <at> debbugs.gnu.org
Subject: Re: bug#12717: 24.2.50;
	[PATCH] `imenu--split-submenus' incorrectly distinguishes submenus
Date: Mon, 29 Oct 2012 23:05:15 -0400

> (defun imenu--split-submenus (alist)
>   "..."
>   (mapcar (lambda (elt)
>             (if (and (consp elt) (stringp (car elt)) (listp (cdr elt)))
>                 (imenu--split-menu (cdr elt) (car elt))
>               elt))
>    alist))
 
> The `if' condition should instead test whether the element is a submenu.
> We have a function that does that: `imenu--subalist-p'.  I believe this
> is the correct code:
 
> (defun imenu--split-submenus (alist)
>   "..."
>   (mapcar (lambda (elt)
>             (if (imenu--subalist-p elt)
>                 (imenu--split-menu (cdr elt) (car elt))
>               elt))
>    alist))

This looks eminently reasonable, except that I don't understand the code
enough to be sure (e.g. the current test includes (consp elt) whereas
imenu--split-menu starts right off by calling cdr).  Do you happen to
have some kind of test case?


        Stefan

Message #11 received at 12717 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Stefan Monnier'" <monnier <at> iro.umontreal.ca>
Cc: 12717 <at> debbugs.gnu.org
Subject: RE: bug#12717: 24.2.50;
	[PATCH] `imenu--split-submenus' incorrectly distinguishes submenus
Date: Mon, 29 Oct 2012 23:18:30 -0700

> This looks eminently reasonable, except that I don't 
> understand the code enough to be sure (e.g. the current test
> includes (consp elt) whereas imenu--split-menu starts right
> off by calling cdr).  Do you happen to have some kind of test
> case?

I ran into the bug when trying to make use of so-called "special" menu items.

I add two general toggle items to the menus, along with the general item
*Rescan*.  In doing that I discovered the bug.

This is the redefinition of `imenu--make-index-alist' that I use.  It is here
that I add two menu items: one to toggle sorting on/off, and one to toggle
case-sensitivity of sorting when sorting by name.

;; REPLACE ORIGINAL in `imenu.el'.
;;
;; 1. Respect `ignore-comments-flag', if defined:
;;    use `with-comments-hidden'.
;; 2. Add Imenu+ toggle commands to menu.
;;
(defun imenu--make-index-alist (&optional noerror)
  "Create an index alist for the definitions in the current buffer.
This works by using the hook function `imenu-create-index-function'.
Report an error if the list is empty unless NOERROR is supplied and
non-nil.

If `ignore-comments-flag' is defined and non-nil, then respect it,
ignoring hidden comments.

See `imenu--index-alist' for the format of the index alist."
  (or (and imenu--index-alist
           (or (not imenu-auto-rescan)
               (and imenu-auto-rescan
                    (> (buffer-size) imenu-auto-rescan-maxout))))
      ;; Get the index; truncate if necessary
      (progn (setq imenu--index-alist
                   (save-excursion
                     (save-restriction
                       (widen)
                       (if (and (> emacs-major-version 20)
                                (require 'hide-comnt nil t))
                           (let ((search-invisible  nil))
                             (with-comments-hidden
                                 (point-min) (point-max)
                                 (funcall imenu-create-index-function)))
                         (funcall imenu-create-index-function)))))
             (imenu--truncate-items imenu--index-alist)))
  (or imenu--index-alist
      noerror
      (user-error
       "No items suitable for an index found in this buffer"))
  (or imenu--index-alist  (setq imenu--index-alist  (list nil)))
  (cons imenu--rescan-item              ; `*Rescan*'.
        (cons '("Toggle Case-Sensitive Name-Sort"
                IGNORE
                (lambda (&rest _ignore)
                  (imenup-toggle-case-sensitive-sorting)))
              (cons '("Toggle Sorting"
                      IGNORE
                      (lambda (&rest _ignore)
                        (imenup-toggle-sort)))
                    imenu--index-alist))))

Clearly, adding general menu items this way is a make-do hack.  I don't use the
POSITION, and the functions invoked don't use any of the args.

And I don't know how, using this kind of menu creation, to, for example, make
the case-sensitivity toggle item inactive when the sort method is not by name.
But I didn't find a better way to add ordinary, non-indexing menu items.

Anyway, that's how I ran into the bug.

The full code is here, FWIW:
http://www.emacswiki.org/emacs-en/download/imenu%2b.el

Message #14 received at 12717 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Stefan Monnier'" <monnier <at> iro.umontreal.ca>
Cc: 12717 <at> debbugs.gnu.org
Subject: RE: bug#12717: 24.2.50;
	[PATCH] `imenu--split-submenus' incorrectly distinguishes submenus
Date: Mon, 29 Oct 2012 23:33:59 -0700

Sorry, I was confused in my reply.  This wasn't directly about
`imenu--make-index-alist', but about `imenu--split-submenus'.  I made the fix
for that at the same time, for the same reason (to add the toggle menu items).

`imenu--subalist-p' does this:
(defun imenu--subalist-p (item)
  (and (consp (cdr item)) (listp (cadr item))
       (not (eq (car (cadr item)) 'lambda))))

I don't think it is necessary to check that the elements of the ALIST arg to
`imenu--split-submenus' are conses.  But if you want to be sure that the `cdr'
in `imenu--subalist-p' doesn't cause a problem, then either add a `consp' test
before `imenu--sublist-p' or change the `cdr' to `cdr-safe'.

I'm pretty sure that the ALIST arg will have only cons elements, but I guess
there is no guarantee.

My guess is that the original test (and (consp elt) (stringp (car elt)) (listp
(cdr elt))) was just a feeble attempt to check for a sublmenu.

Anyway, now you know as much as I about the code, and can decide how you want to
fix it.

The point of the bug report is that the current definition ends up trying to
handle a "special" menu item as if it were a submenu: it is a consp with a
string car and listp cdr, but it is not a submenu.  It ends up choking.

Message #19 received at 12717-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: "Drew Adams" <drew.adams <at> oracle.com>
Cc: 12717-done <at> debbugs.gnu.org
Subject: Re: bug#12717: 24.2.50;
	[PATCH] `imenu--split-submenus' incorrectly distinguishes submenus
Date: Wed, 14 Nov 2012 21:03:33 -0500

> This looks eminently reasonable, except that I don't understand the code
> enough to be sure (e.g. the current test includes (consp elt) whereas
> imenu--split-menu starts right off by calling cdr).  Do you happen to
> have some kind of test case?

Well, I installed it in trunk, it looks safe enough there,


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.