GNU bug report logs - #14974
24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item

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 14974 in the body.
You can then email your comments to 14974 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: Sebastian Wiesner <lunaryorn <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.3.50;
 adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
Date: Sun, 28 Jul 2013 15:32:12 +0200

adaptive-wrap 0.3 adds an autoload for a call to "(easy-menu-add-item
…)".  However, "easy-menu-add-item" itself is not autoloaded, and
"adaptive-wrap" does not "(require 'easymenu)" before calling
"easy-menu-add-item" in an autoload.

Hence, *activating* (not even loading!) "adaptive-wrap" fails unless
"easymenu" incidentally happens to be loaded before.

Obviously this breaks all sorts of things, most notably package
installation in "emacs -Q --batch".

Greetings,
Sebastian Wiesner

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Sebastian Wiesner <lunaryorn <at> gmail.com>
Cc: 14974-done <at> debbugs.gnu.org
Subject: Re: bug#14974: 24.3.50;
 adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
Date: Mon, 29 Jul 2013 13:59:21 +0200

On Sun, 28 Jul 2013 15:32:12 +0200 Sebastian Wiesner <lunaryorn <at> gmail.com> wrote:

> adaptive-wrap 0.3 adds an autoload for a call to "(easy-menu-add-item
> …)".  However, "easy-menu-add-item" itself is not autoloaded, and
> "adaptive-wrap" does not "(require 'easymenu)" before calling
> "easy-menu-add-item" in an autoload.
>
> Hence, *activating* (not even loading!) "adaptive-wrap" fails unless
> "easymenu" incidentally happens to be loaded before.
>
> Obviously this breaks all sorts of things, most notably package
> installation in "emacs -Q --batch".

Thanks for the report; fixed.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: 14974 <at> debbugs.gnu.org
Cc: lunaryorn <at> gmail.com, stephen.berman <at> gmx.net
Subject: Re: bug#14974: 24.3.50;
 adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
Date: Mon, 29 Jul 2013 11:24:17 -0400

> Thanks for the report; fixed.

I don't think that's enough: the error is signaled before
adaptive-wrap.el is loaded, i.e. before seeing the code you modified.
You'd need to add a ;;;###autoload in front of the new (require 'easymenu).


        Stefan

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

From: Sebastian Wiesner <lunaryorn <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 14974 <at> debbugs.gnu.org, stephen.berman <at> gmx.net
Subject: Re: bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void
 function easy-menu-add-item
Date: Mon, 29 Jul 2013 17:32:28 +0200

2013/7/29 Stefan Monnier <monnier <at> iro.umontreal.ca>:
>> Thanks for the report; fixed.
>
> I don't think that's enough: the error is signaled before
> adaptive-wrap.el is loaded, i.e. before seeing the code you modified.
> You'd need to add a ;;;###autoload in front of the new (require 'easymenu).

Shouldn't adaptive-wrap rather use one of the autoloadable functions
from "easymenu"?

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Sebastian Wiesner <lunaryorn <at> gmail.com>
Cc: 14974 <at> debbugs.gnu.org, stephen.berman <at> gmx.net
Subject: Re: bug#14974: 24.3.50;
 adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
Date: Mon, 29 Jul 2013 12:40:31 -0400

>>> Thanks for the report; fixed.
>> I don't think that's enough: the error is signaled before
>> adaptive-wrap.el is loaded, i.e. before seeing the code you modified.
>> You'd need to add a ;;;###autoload in front of the new (require 'easymenu).
> Shouldn't adaptive-wrap rather use one of the autoloadable functions
> from "easymenu"?

It could, but that would load easymenu just as well.

An alternative would be to do the same with define-key rather than
using easymenu.


        Stefan

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Sebastian Wiesner <lunaryorn <at> gmail.com>, 14974 <at> debbugs.gnu.org
Subject: Re: bug#14974: 24.3.50;
 adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
Date: Mon, 29 Jul 2013 23:35:11 +0200

On Mon, 29 Jul 2013 12:40:31 -0400 Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>>>> Thanks for the report; fixed.
>>> I don't think that's enough: the error is signaled before
>>> adaptive-wrap.el is loaded, i.e. before seeing the code you modified.
>>> You'd need to add a ;;;###autoload in front of the new (require 'easymenu).

Hmm, I guess I don't understand what is going wrong -- what error is
being signaled and what is causing it?  From the shell I invoked 

emacs -Q --batch --eval "(package-install 'adaptive-wrap)"

and that returns "package.el is not yet initialized!" -- but the same
thing happens with any other package, so that can't be the problem.
What is a recipe to getting the error, so I can understand what the
issue is?

>> Shouldn't adaptive-wrap rather use one of the autoloadable functions
>> from "easymenu"?
>
> It could, but that would load easymenu just as well.
>
> An alternative would be to do the same with define-key rather than
> using easymenu.

The following seems to DTRT (and also allows dispensing with
adaptive-wrap-unload-function):

(define-key-after (lookup-key menu-bar-options-menu [line-wrapping])
  [adaptive-wrap]
  '(menu-item "Adaptive Wrap" (lambda ()
				(interactive)
				(if adaptive-wrap-prefix-mode
				    (adaptive-wrap-prefix-mode -1)
				  (adaptive-wrap-prefix-mode 1)))
	      :enable t
	      :visible (and (menu-bar-menu-frame-live-and-visible-p)
			    (featurep 'adaptive-wrap))
	      :help "Show wrapped long lines with an adjustable prefix"
	      :button (:toggle . adaptive-wrap-prefix-mode))
  word-wrap)

Should I install it in place of the easy-menu definition?  (Though in
any case I'd still like to understand just what the problem with the
latter is.)

Steve Berman

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: Sebastian Wiesner <lunaryorn <at> gmail.com>, 14974 <at> debbugs.gnu.org
Subject: Re: bug#14974: 24.3.50;
 adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
Date: Mon, 29 Jul 2013 18:15:14 -0400

> What is a recipe to getting the error, so I can understand what the
> issue is?

1- install adaptive-wrap from ELPA.
2- restart Emacs.

>   '(menu-item "Adaptive Wrap" (lambda ()
> 				(interactive)
> 				(if adaptive-wrap-prefix-mode
> 				    (adaptive-wrap-prefix-mode -1)
> 				  (adaptive-wrap-prefix-mode 1)))

The binding should simply be adaptive-wrap-prefix-mode, shouldn't it?

> 	      :enable t
> 	      :visible (and (menu-bar-menu-frame-live-and-visible-p)
> 			    (featurep 'adaptive-wrap))

Why not always show it?

> 	      :button (:toggle . adaptive-wrap-prefix-mode))

adaptive-wrap-prefix-mode only exists after adaptive-wrap has been
loaded, so better use (bound-and-true-p adaptive-wrap-prefix-mode).


        Stefan

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Sebastian Wiesner <lunaryorn <at> gmail.com>, 14974 <at> debbugs.gnu.org
Subject: Re: bug#14974: 24.3.50;
 adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
Date: Tue, 30 Jul 2013 01:30:55 +0200

On Mon, 29 Jul 2013 18:15:14 -0400 Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>> What is a recipe to getting the error, so I can understand what the
>> issue is?
>
> 1- install adaptive-wrap from ELPA.
> 2- restart Emacs.

Now I see, thanks.

>>   '(menu-item "Adaptive Wrap" (lambda ()
>> 				(interactive)
>> 				(if adaptive-wrap-prefix-mode
>> 				    (adaptive-wrap-prefix-mode -1)
>> 				  (adaptive-wrap-prefix-mode 1)))
>
> The binding should simply be adaptive-wrap-prefix-mode, shouldn't it?

You mean (lambda () (interactive) (adaptive-wrap-prefix-mode 'toggle))?
Yes, that's better.

>> 	      :enable t
>> 	      :visible (and (menu-bar-menu-frame-live-and-visible-p)
>> 			    (featurep 'adaptive-wrap))
>
> Why not always show it?

Then the check box remains in the menu after unloading adaptive-wrap.el,
but it becomes a noop.  Making visibility depend on whether the feature
is present is, AFAICS, a way of avoiding a separate
adaptive-wrap-unload-function.  Or am I wrong about this, or do you mean
something else?

>> 	      :button (:toggle . adaptive-wrap-prefix-mode))
>
> adaptive-wrap-prefix-mode only exists after adaptive-wrap has been
> loaded, so better use (bound-and-true-p adaptive-wrap-prefix-mode).

Ok.

Steve Berman

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: Sebastian Wiesner <lunaryorn <at> gmail.com>, 14974 <at> debbugs.gnu.org
Subject: Re: bug#14974: 24.3.50;
 adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
Date: Mon, 29 Jul 2013 21:08:48 -0400

> You mean (lambda () (interactive) (adaptive-wrap-prefix-mode 'toggle))?

No, I mean `adaptive-wrap-prefix-mode', which will already do the same.

> Then the check box remains in the menu after unloading adaptive-wrap.el,

Big deal.  Don't waste code on such a non-issue that will only affect
maybe 10 people in the next century, 9 of whom won't even notice it
because they won't look at the menu.

> but it becomes a noop.  Making visibility depend on whether the
> feature is present is, AFAICS, a way of avoiding a separate
> adaptive-wrap-unload-function.  Or am I wrong about this, or do you
> mean something else?

As I said, unloading a package is largely broken, so some left-over
entries in a menu are the least of your worries in this respect.


        Stefan

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Sebastian Wiesner <lunaryorn <at> gmail.com>, 14974 <at> debbugs.gnu.org
Subject: Re: bug#14974: 24.3.50;
 adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
Date: Tue, 30 Jul 2013 10:54:09 +0200

On Mon, 29 Jul 2013 21:08:48 -0400 Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>> You mean (lambda () (interactive) (adaptive-wrap-prefix-mode 'toggle))?
>
> No, I mean `adaptive-wrap-prefix-mode', which will already do the same.

Got it, finally.  Sorry for the denseness.

>> Then the check box remains in the menu after unloading adaptive-wrap.el,
>
> Big deal.  Don't waste code on such a non-issue that will only affect
> maybe 10 people in the next century, 9 of whom won't even notice it
> because they won't look at the menu.
>
>> but it becomes a noop.  Making visibility depend on whether the
>> feature is present is, AFAICS, a way of avoiding a separate
>> adaptive-wrap-unload-function.  Or am I wrong about this, or do you
>> mean something else?
>
> As I said, unloading a package is largely broken, so some left-over
> entries in a menu are the least of your worries in this respect.

I honestly don't see having the feature check as a waste of code, but no
matter.  I assume, then, that adaptive-wrap-unload-function is also
dispensable; so is the following patch ok to check in?

Steve Berman

=== modified file 'packages/adaptive-wrap/adaptive-wrap.el'
*** packages/adaptive-wrap/adaptive-wrap.el	2013-07-29 11:56:03 +0000
--- packages/adaptive-wrap/adaptive-wrap.el	2013-07-30 08:15:54 +0000
***************
*** 4,10 ****
  
  ;; Author: Stephen Berman <stephen.berman <at> gmx.net>
  ;;         Stefan Monnier <monnier <at> iro.umontreal.ca>
! ;; Version: 0.4
  
  ;; This program is free software; you can redistribute it and/or modify
  ;; it under the terms of the GNU General Public License as published by
--- 4,10 ----
  
  ;; Author: Stephen Berman <stephen.berman <at> gmx.net>
  ;;         Stefan Monnier <monnier <at> iro.umontreal.ca>
! ;; Version: 0.5
  
  ;; This program is free software; you can redistribute it and/or modify
  ;; it under the terms of the GNU General Public License as published by
***************
*** 103,127 ****
          (widen)
          (remove-text-properties (point-min) (point-max) '(wrap-prefix nil))))))
  
! ;;;###autoload
! (easy-menu-add-item menu-bar-options-menu
!                     '("Line Wrapping in This Buffer")
!                     ["Adaptive Wrap"
!                      (lambda ()
!                        (interactive)
! 		       (if adaptive-wrap-prefix-mode
! 			   (adaptive-wrap-prefix-mode -1)
! 			 (adaptive-wrap-prefix-mode 1)))
!                      :visible (menu-bar-menu-frame-live-and-visible-p)
!                      :help "Show wrapped long lines with an adjustable prefix"
!                      :style toggle
!                      :selected adaptive-wrap-prefix-mode])
! 
! (defun adaptive-wrap-unload-function ()
!   "Cleanup adaptive-wrap package."
!   (easy-menu-remove-item menu-bar-options-menu
!                          '("Line Wrapping in This Buffer")
!                          "Adaptive Wrap"))
  
  (provide 'adaptive-wrap)
  ;;; adaptive-wrap.el ends here
--- 103,116 ----
          (widen)
          (remove-text-properties (point-min) (point-max) '(wrap-prefix nil))))))
  
! (define-key-after (lookup-key menu-bar-options-menu [line-wrapping])
!   [adaptive-wrap]
!   '(menu-item "Adaptive Wrap" adaptive-wrap-prefix-mode
! 	      :enable t
! 	      :visible (menu-bar-menu-frame-live-and-visible-p)
! 	      :help "Show wrapped long lines with an adjustable prefix"
! 	      :button (:toggle . (bound-and-true-p adaptive-wrap-prefix-mode)))
!   word-wrap)
  
  (provide 'adaptive-wrap)
  ;;; adaptive-wrap.el ends here

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stephen Berman <stephen.berman <at> gmx.net>, eliz <at> gnu.org, cyd <at> gnu.org
Cc: Sebastian Wiesner <lunaryorn <at> gmail.com>, 14974 <at> debbugs.gnu.org
Subject: Re: bug#14974: 24.3.50;
 adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
Date: Tue, 30 Jul 2013 10:14:05 -0400

> I honestly don't see having the feature check as a waste of code, but no
> matter.

KISS.

> I assume, then, that adaptive-wrap-unload-function is also dispensable;

Yes.

> so is the following patch ok to check in?

Fine, with one comment:
  
> ! (define-key-after (lookup-key menu-bar-options-menu [line-wrapping])
> !   [adaptive-wrap]
> !   '(menu-item "Adaptive Wrap" adaptive-wrap-prefix-mode
> ! 	      :enable t
> ! 	      :visible (menu-bar-menu-frame-live-and-visible-p)
> ! 	      :help "Show wrapped long lines with an adjustable prefix"
> ! 	      :button (:toggle . (bound-and-true-p adaptive-wrap-prefix-mode)))
> !   word-wrap)
  
The ":enable t" is redundant, remove it.
As for the ":visible" entry, I'm wondering what it's there for: I see
it's also used for the line-wrap entry, but I don't know what it's used for
there either.

I see that menu-bar-menu-frame-live-and-visible-p was added in 2005 by
Eli (used for :enable rather than :visible, by the way) and it
was moved to :visible later on when Yidong added the word-wrap entry.

Could someone add a comment explaining what it's for?  Usually the
menu's frame can't be invisible if the menu is visible, so I guess it
has to do with either the `ns' build or with the Gtk feature that lets
on `pin' a menu?  But why is it used on these entries and not on others?
Why use it for `:visible' rather than `:enable' or for something
yet entirely different?


        Stefan

Previous Next

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