* 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).