Re: tool-bar-setup overwrites local tool-bar-map

Re: tool-bar-setup overwrites local tool-bar-map

[Bill Wohler]

Bill Wohler <wohler@olgas.newt.com> wrote:

>   emacs22 -Q
>   M-x info
>   M-x tool-bar-mode
> 
> Note how Preferences and Help icons are present.
> 
>   C-x b *scratch* RET
> 
> Note how Preferences and Help icons are absent.
> 
> To see what should have happened:
> 
>   emacs22 -Q
>   M-x tool-bar-mode
> 
> Note how Preferences and Help icons are present.
> 
>   M-x info
> 
> Note how Preferences and Help icons are absent.
> 
> This is particularly bad if you fire up mh-rmail before tool-bar-mode
> since you get two pairs of Help and Preferences. 
> 
> I see that tool-bar-add-item-from-menu passes (default-value
> 'tool-bar-map) to tool-bar-local-item-from-menu while tool-bar-add-item
> does not. Coincidentally, it is the tool-bar-add-item calls in
> tool-bar-setup which are a problem (Preferences and Help). Changing
> tool-bar-add-item to match tool-bar-item-from-menu fixes the problem.
> 
> Does anyone know why tool-bar-add-item-from-menu got the (default-value
> 'tool-bar-map) and tool-bar-add-item did not? If not, may I check in
> this patch?

No, I may not ;-). It breaks the idiom from the tool-bar-add-item
documentation, namely:

          (defvar foo-tool-bar-map
            (let ((tool-bar-map (make-sparse-keymap)))
              (tool-bar-add-item ...)
              ...
              tool-bar-map))

It also did some mean and nasty things to the MH-E toolbar. The
following patch (to tool-bar-setup) seems to be working a little better.

Index: tool-bar.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/tool-bar.el,v
retrieving revision 1.5
diff -u -u -r1.5 tool-bar.el
--- tool-bar.el	6 Feb 2006 14:33:35 -0000	1.5
+++ tool-bar.el	21 Apr 2006 00:25:35 -0000
@@ -267,14 +267,16 @@
   ;;(tool-bar-add-item-from-menu 'compose-mail "mail/compose")
 
   (tool-bar-add-item-from-menu 'print-buffer "print")
-  (tool-bar-add-item "preferences" 'customize 'customize
-		     :help "Edit preferences (customize)")
+  (tool-bar-local-item "preferences" 'customize 'customize
+                       (default-value 'tool-bar-map)
+                       :help "Edit preferences (customize)")
 
-  (tool-bar-add-item "help" (lambda ()
-			      (interactive)
-			      (popup-menu menu-bar-help-menu))
-		     'help
-		     :help "Pop up the Help menu")
+  (tool-bar-local-item "help" (lambda ()
+                                (interactive)
+                                (popup-menu menu-bar-help-menu))
+                       'help
+                       (default-value 'tool-bar-map)
+                       :help "Pop up the Help menu")
   )
 
 (provide 'tool-bar)

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.

Re: tool-bar-setup overwrites local tool-bar-map

[Richard Stallman]

    > To see what should have happened:
    > 
    >   emacs22 -Q
    >   M-x tool-bar-mode
    > 
    > Note how Preferences and Help icons are present.
    > 
    >   M-x info
    > 
    > Note how Preferences and Help icons are absent.

In what sense is that "what should have happened"?  I do not follow.

Shouldn't the Preferences and Help icons always be present?

    No, I may not ;-). It breaks the idiom from the tool-bar-add-item
    documentation, namely:

	      (defvar foo-tool-bar-map
		(let ((tool-bar-map (make-sparse-keymap)))
		  (tool-bar-add-item ...)
		  ...
		  tool-bar-map))

    It also did some mean and nasty things to the MH-E toolbar. The
    following patch (to tool-bar-setup) seems to be working a little better.

What are you asking us to do?

Re: tool-bar-setup overwrites local tool-bar-map

[Bill Wohler]

Richard Stallman <rms@gnu.org> wrote:

>     > To see what should have happened:
>     > 
>     >   emacs22 -Q
>     >   M-x tool-bar-mode
>     > 
>     > Note how Preferences and Help icons are present.
>     > 
>     >   M-x info
>     > 
>     > Note how Preferences and Help icons are absent.
> 
> In what sense is that "what should have happened"?  I do not follow.

You get the tool bar as the info package had intended.

> Shouldn't the Preferences and Help icons always be present?

Arguably yes, but their function is context-dependent. For example, the
MH-E package defines the actions as mh-tool-bar-customize and
mh-tool-bar-folder-help or mh-tool-bar-letter-help where the default
actions are customize and help.

With the current tool-bar code, the behavior is inconsistent.

If one runs tool-bar-mode first, and then MH-E, one sees the one pair of
Preferences/Help buttons (the ones defined by MH-E). If vice-versa, one
sees *two* pairs of Preferences/Help buttons (the ones defined by MH-E
plus the ones defined by tool-bar).

Similarly, if one runs tool-bar-mode first, and then info, one does not
see the pair of Preferences/Help buttons since the info package does not
define them. If vice-versa, one sees the pair of Preferences/Help
buttons defined by tool-bar.

Does that help clear things up?

> What are you asking us to do?

My patch makes the tool-bar's behavior consistent. It prevents the
default tool bar from interfering with the mode's tool bar if
tool-bar-mode is run after the mode has been initialized. I just wanted
a review and a "Please check it in" before checking it in. Perhaps Nick
Roberts? Here it is again.

Index: tool-bar.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/tool-bar.el,v
retrieving revision 1.5
diff -u -u -r1.5 tool-bar.el
--- tool-bar.el	6 Feb 2006 14:33:35 -0000	1.5
+++ tool-bar.el	30 Apr 2006 18:21:10 -0000
@@ -267,14 +267,16 @@
   ;;(tool-bar-add-item-from-menu 'compose-mail "mail/compose")
 
   (tool-bar-add-item-from-menu 'print-buffer "print")
-  (tool-bar-add-item "preferences" 'customize 'customize
-		     :help "Edit preferences (customize)")
+  (tool-bar-local-item "preferences" 'customize 'customize
+                       (default-value 'tool-bar-map)
+                       :help "Edit preferences (customize)")
 
-  (tool-bar-add-item "help" (lambda ()
-			      (interactive)
-			      (popup-menu menu-bar-help-menu))
-		     'help
-		     :help "Pop up the Help menu")
+  (tool-bar-local-item "help" (lambda ()
+                                (interactive)
+                                (popup-menu menu-bar-help-menu))
+                       'help
+                       (default-value 'tool-bar-map)
+                       :help "Pop up the Help menu")
   )
 
 (provide 'tool-bar)

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.

Re: tool-bar-setup overwrites local tool-bar-map

[Richard Stallman]

    If one runs tool-bar-mode first, and then MH-E, one sees the one pair of
    Preferences/Help buttons (the ones defined by MH-E). If vice-versa, one
    sees *two* pairs of Preferences/Help buttons (the ones defined by MH-E
    plus the ones defined by tool-bar).

Now I understand the point of your patch.
But it seems to me that ALL the standard tool-bar items
ought to be added to the standard tool-bar map.
Not just Help and Preferences.

Ah, now I see.  All the rest are added using
tool-bar-add-item-from-menu, which uses (default-value 'tool-bar-map).

So I believe your patch is correct.  But I think this is cleaner.
Does it work?


*** tool-bar.el	07 Feb 2006 18:16:16 -0500	1.5
--- tool-bar.el	30 Apr 2006 23:58:17 -0400	
***************
*** 267,280 ****
    ;;(tool-bar-add-item-from-menu 'compose-mail "mail/compose")
  
    (tool-bar-add-item-from-menu 'print-buffer "print")
-   (tool-bar-add-item "preferences" 'customize 'customize
- 		     :help "Edit preferences (customize)")
  
!   (tool-bar-add-item "help" (lambda ()
! 			      (interactive)
! 			      (popup-menu menu-bar-help-menu))
! 		     'help
! 		     :help "Pop up the Help menu")
    )
  
  (provide 'tool-bar)
--- 267,286 ----
    ;;(tool-bar-add-item-from-menu 'compose-mail "mail/compose")
  
    (tool-bar-add-item-from-menu 'print-buffer "print")
  
!   ;; tool-bar-add-item-from-menu itself operates on
!   ;; (default-value 'tool-bar-map), but when we don't use that function,
!   ;; we must explicitly operate on the default value.
! 
!   (let ((tool-bar-map (default-value 'tool-bar-map)))
!     (tool-bar-add-item "preferences" 'customize 'customize
! 		       :help "Edit preferences (customize)")
! 
!     (tool-bar-add-item "help" (lambda ()
! 				(interactive)
! 				(popup-menu menu-bar-help-menu))
! 		       'help
! 		       :help "Pop up the Help menu"))
    )
  
  (provide 'tool-bar)

Re: tool-bar-setup overwrites local tool-bar-map

[Bill Wohler]

Richard Stallman <rms@gnu.org> wrote:

>     If one runs tool-bar-mode first, and then MH-E, one sees the one pair of
>     Preferences/Help buttons (the ones defined by MH-E). If vice-versa, one
>     sees *two* pairs of Preferences/Help buttons (the ones defined by MH-E
>     plus the ones defined by tool-bar).
> 
> Now I understand the point of your patch.
> But it seems to me that ALL the standard tool-bar items
> ought to be added to the standard tool-bar map.
> Not just Help and Preferences.
> 
> Ah, now I see.  All the rest are added using
> tool-bar-add-item-from-menu, which uses (default-value 'tool-bar-map).
> 
> So I believe your patch is correct.  But I think this is cleaner.
> Does it work?

I agree. Your patch is cleaner and is closer to the documented usage.
And it works (for me). I'd be happy if you checked it in. Thanks.

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.