unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
@ 2013-07-28 13:32 Sebastian Wiesner
  2013-07-29 11:59 ` Stephen Berman
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Wiesner @ 2013-07-28 13:32 UTC (permalink / raw)
  To: 14974


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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
  2013-07-28 13:32 bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item Sebastian Wiesner
@ 2013-07-29 11:59 ` Stephen Berman
  2013-07-29 15:24   ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Berman @ 2013-07-29 11:59 UTC (permalink / raw)
  To: Sebastian Wiesner; +Cc: 14974-done

On Sun, 28 Jul 2013 15:32:12 +0200 Sebastian Wiesner <lunaryorn@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.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
  2013-07-29 11:59 ` Stephen Berman
@ 2013-07-29 15:24   ` Stefan Monnier
  2013-07-29 15:32     ` Sebastian Wiesner
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2013-07-29 15:24 UTC (permalink / raw)
  To: 14974; +Cc: lunaryorn, stephen.berman

> 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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
  2013-07-29 15:24   ` Stefan Monnier
@ 2013-07-29 15:32     ` Sebastian Wiesner
  2013-07-29 16:40       ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Wiesner @ 2013-07-29 15:32 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 14974, stephen.berman

2013/7/29 Stefan Monnier <monnier@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"?





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
  2013-07-29 15:32     ` Sebastian Wiesner
@ 2013-07-29 16:40       ` Stefan Monnier
  2013-07-29 21:35         ` Stephen Berman
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2013-07-29 16:40 UTC (permalink / raw)
  To: Sebastian Wiesner; +Cc: 14974, stephen.berman

>>> 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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
  2013-07-29 16:40       ` Stefan Monnier
@ 2013-07-29 21:35         ` Stephen Berman
  2013-07-29 22:15           ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Berman @ 2013-07-29 21:35 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Sebastian Wiesner, 14974

On Mon, 29 Jul 2013 12:40:31 -0400 Stefan Monnier <monnier@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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
  2013-07-29 21:35         ` Stephen Berman
@ 2013-07-29 22:15           ` Stefan Monnier
  2013-07-29 23:30             ` Stephen Berman
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2013-07-29 22:15 UTC (permalink / raw)
  To: Stephen Berman; +Cc: Sebastian Wiesner, 14974

> 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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
  2013-07-29 22:15           ` Stefan Monnier
@ 2013-07-29 23:30             ` Stephen Berman
  2013-07-30  1:08               ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Berman @ 2013-07-29 23:30 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Sebastian Wiesner, 14974

On Mon, 29 Jul 2013 18:15:14 -0400 Stefan Monnier <monnier@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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
  2013-07-29 23:30             ` Stephen Berman
@ 2013-07-30  1:08               ` Stefan Monnier
  2013-07-30  8:54                 ` Stephen Berman
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2013-07-30  1:08 UTC (permalink / raw)
  To: Stephen Berman; +Cc: Sebastian Wiesner, 14974

> 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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
  2013-07-30  1:08               ` Stefan Monnier
@ 2013-07-30  8:54                 ` Stephen Berman
  2013-07-30 14:14                   ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Berman @ 2013-07-30  8:54 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Sebastian Wiesner, 14974

On Mon, 29 Jul 2013 21:08:48 -0400 Stefan Monnier <monnier@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@gmx.net>
  ;;         Stefan Monnier <monnier@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@gmx.net>
  ;;         Stefan Monnier <monnier@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






^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item
  2013-07-30  8:54                 ` Stephen Berman
@ 2013-07-30 14:14                   ` Stefan Monnier
  0 siblings, 0 replies; 11+ messages in thread
From: Stefan Monnier @ 2013-07-30 14:14 UTC (permalink / raw)
  To: Stephen Berman, eliz, cyd; +Cc: Sebastian Wiesner, 14974

> 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





^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-07-30 14:14 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-28 13:32 bug#14974: 24.3.50; adaptive-wrap 0.3 calls potentially void function easy-menu-add-item Sebastian Wiesner
2013-07-29 11:59 ` Stephen Berman
2013-07-29 15:24   ` Stefan Monnier
2013-07-29 15:32     ` Sebastian Wiesner
2013-07-29 16:40       ` Stefan Monnier
2013-07-29 21:35         ` Stephen Berman
2013-07-29 22:15           ` Stefan Monnier
2013-07-29 23:30             ` Stephen Berman
2013-07-30  1:08               ` Stefan Monnier
2013-07-30  8:54                 ` Stephen Berman
2013-07-30 14:14                   ` Stefan Monnier

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).