Tool-bar and multi-tty

Tool-bar and multi-tty

[Chong Yidong]

Bug 1133 notes a problem with tool-bars and multi-tty.  Basically,
tool-bar-map is not set up if the first frame is on a tty; it is only
initialized when the first graphical frame is created.  This causes
problems for major modes that define their own tool-bar maps.

I think the solution is to define a normal hook, tool-bar-setup-hook, to
be run after the tool-bar is set up successfully for the first time.

Each mode which defines its own tool-bar map will have to check if
tool-bar-setup is nil at the time the mode is loaded; if not, it must
use this hook to initialize the tool-bar variable when the first
graphical frame is created.  (Note that we can't load the tool-bar
variables ahead of time, because the icons used may depend on X settings
which are unavailable until X is initialized.)

See the attached patch.  Similar changes will have to be made to all
modes which use the tool-bar.

WDYT?


*** trunk/lisp/tool-bar.el.~1.19.~	2008-10-09 21:52:12.000000000 -0400
--- trunk/lisp/tool-bar.el	2008-10-09 22:02:57.000000000 -0400
***************
*** 260,266 ****
--- 260,274 ----
  (defvar tool-bar-setup nil
    "Non-nil if the tool-bar has been set up by `tool-bar-setup'.")
  
+ (defvar tool-bar-setup-hook nil
+   "Normal hook run after `tool-bar-setup' is called successfully.")
+ 
  (defun tool-bar-setup (&optional frame)
+   "Set up the tool-bar on frame FRAME.
+ If FRAME is nil, default to the selected frame.
+ This does nothing if the variable `tool-bar-setup' is non-nil, or
+ `tool-bar-mode' is disabled, or FRAME is on a text-only terminal.
+ Otherwise, set up the tool bar and run `tool-bar-setup-hook'."
    (unless (or tool-bar-setup
  	      (null tool-bar-mode)
  	      ;; No-op if the initial frame is on a tty, deferring
***************
*** 321,327 ****
                                      (popup-menu menu-bar-help-menu))
                             'help
                             :help "Pop up the Help menu"))
!       (setq tool-bar-setup t))))
  
  
  (provide 'tool-bar)
--- 329,336 ----
                                      (popup-menu menu-bar-help-menu))
                             'help
                             :help "Pop up the Help menu"))
!       (setq tool-bar-setup t)
!       (run-hooks 'tool-bar-setup-hook))))
  
  
  (provide 'tool-bar)
*** trunk/lisp/progmodes/compile.el.~1.478.~	2008-10-09 11:24:39.000000000 -0400
--- trunk/lisp/progmodes/compile.el	2008-10-09 22:19:14.000000000 -0400
***************
*** 1470,1496 ****
    "Keymap for compilation log buffers.
  `compilation-minor-mode-map' is a parent of this.")
  
! (defvar compilation-mode-tool-bar-map
!   (if (display-graphic-p)
!       (let ((map (butlast (copy-keymap tool-bar-map)))
! 	    (help (last tool-bar-map))) ;; Keep Help last in tool bar
! 	(tool-bar-local-item
! 	 "left-arrow" 'previous-error-no-select 'previous-error-no-select map
! 	 :rtl "right-arrow"
! 	 :help "Goto previous error")
! 	(tool-bar-local-item
! 	 "right-arrow" 'next-error-no-select 'next-error-no-select map
! 	 :rtl "left-arrow"
! 	 :help "Goto next error")
! 	(tool-bar-local-item
! 	 "cancel" 'kill-compilation 'kill-compilation map
! 	 :enable '(let ((buffer (compilation-find-buffer)))
! 		    (get-buffer-process buffer))
! 	 :help "Stop compilation")
! 	(tool-bar-local-item
! 	 "refresh" 'recompile 'recompile map
! 	 :help "Restart compilation")
! 	(append map help))))
  
  (put 'compilation-mode 'mode-class 'special)
  
--- 1470,1498 ----
    "Keymap for compilation log buffers.
  `compilation-minor-mode-map' is a parent of this.")
  
! (defvar compilation-mode-tool-bar-map (make-sparse-keymap))
! 
! (defun compilation-tool-bar-setup ()
!   (let ((map compilation-mode-tool-bar-map)
! 	(help (last tool-bar-map))) ;; Keep Help last in tool bar
!     (setcdr map (cdr (butlast (copy-keymap tool-bar-map))))
!     (tool-bar-local-item
!      "left-arrow" 'previous-error-no-select 'previous-error-no-select map
!      :rtl "right-arrow"
!      :help "Goto previous error")
!     (tool-bar-local-item
!      "right-arrow" 'next-error-no-select 'next-error-no-select map
!      :rtl "left-arrow"
!      :help "Goto next error")
!     (tool-bar-local-item
!      "cancel" 'kill-compilation 'kill-compilation map
!      :enable '(let ((buffer (compilation-find-buffer)))
! 		(get-buffer-process buffer))
!      :help "Stop compilation")
!     (tool-bar-local-item
!      "refresh" 'recompile 'recompile map
!      :help "Restart compilation")
!     (append map help)))
  
  (put 'compilation-mode 'mode-class 'special)
  
***************
*** 1509,1514 ****
--- 1511,1519 ----
    (use-local-map compilation-mode-map)
    ;; Let windows scroll along with the output.
    (set (make-local-variable 'window-point-insertion-type) t)
+   (if tool-bar-setup
+       (compilation-tool-bar-setup)
+     (add-hook 'tool-bar-setup-hook 'compilation-tool-bar-setup))
    (set (make-local-variable 'tool-bar-map) compilation-mode-tool-bar-map)
    (setq major-mode 'compilation-mode
  	mode-name (or name-of-mode "Compilation"))

Re: Tool-bar and multi-tty

[Eli Zaretskii]

> From: Chong Yidong <cyd@stupidchicken.com>
> Date: Thu, 09 Oct 2008 22:30:06 -0400
> Cc: 1133@emacsbugs.donarmstrong.com, emacs-devel@gnu.org
> 
> ! (defvar compilation-mode-tool-bar-map
> !   (if (display-graphic-p)
> !       (let ((map (butlast (copy-keymap tool-bar-map)))

display-graphic-p should probably be called with the right frame as an
argument, or else it runs the risk of doing the wrong thing.

Re: Tool-bar and multi-tty

[Eli Zaretskii]

> From: Chong Yidong <cyd@stupidchicken.com>
> Date: Thu, 09 Oct 2008 22:30:06 -0400
> Cc: 1133@emacsbugs.donarmstrong.com, emacs-devel@gnu.org
> 
> Bug 1133 notes a problem with tool-bars and multi-tty.  Basically,
> tool-bar-map is not set up if the first frame is on a tty; it is only
> initialized when the first graphical frame is created.  This causes
> problems for major modes that define their own tool-bar maps.
> 
> I think the solution is to define a normal hook, tool-bar-setup-hook, to
> be run after the tool-bar is set up successfully for the first time.

This sounds like a very complicated solution.  Why can't we set up
tool-bar-map unconditionally?  Perhaps changes that make that possible
are much smaller and cleaner than what you suggest here?

Re: Tool-bar and multi-tty

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

> This sounds like a very complicated solution.  Why can't we set up
> tool-bar-map unconditionally?

After thinking about that, I think it wouldn't work.

As I said, various properties of the tool-bar, such as the icons to use,
depend on X resources and GTK resources which are not available until
the first graphical frame is loaded.

And Emacs can't always access X resources and GTK resources ahead of
time: for instance, imagine ssh'ing to a server without an X forwarding
agent, and running an emacs server; then ssh in with a forwarding agent,
and run `emacsclient -c'.

Re: Tool-bar and multi-tty

[Eli Zaretskii]

> From: Chong Yidong <cyd@stupidchicken.com>
> Cc: dann@ics.uci.edu,  1133@emacsbugs.donarmstrong.com,  emacs-devel@gnu.org
> Date: Fri, 10 Oct 2008 13:05:00 -0400
> 
> As I said, various properties of the tool-bar, such as the icons to use,
> depend on X resources and GTK resources which are not available until
> the first graphical frame is loaded.

What properties require X resources?  Can we refactor the code so that
it only accesses X resources when the tool bar is about to be
displayed?

Re: Tool-bar and multi-tty

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

> What properties require X resources?  Can we refactor the code so that
> it only accesses X resources when the tool bar is about to be
> displayed?

OK, I looked further into this.  I was mistaken in my diagnosis of the
problem: it's not X resources that are the issue.

First, let me explain the problem more precisely.

To preload the tool-bar, we just need to make the following change to
tool-bar.el:

*** trunk/lisp/tool-bar.el.~1.19.~	2008-10-10 15:15:38.000000000 -0400
--- trunk/lisp/tool-bar.el	2008-10-10 15:16:57.000000000 -0400
***************
*** 262,273 ****
  
  (defun tool-bar-setup (&optional frame)
    (unless (or tool-bar-setup
! 	      (null tool-bar-mode)
! 	      ;; No-op if the initial frame is on a tty, deferring
! 	      ;; action until called from x-create-frame-with-faces.
! 	      ;; Tool-bar icons can depend on X settings, which are
! 	      ;; initially unavailable in this case.
! 	      (not (display-graphic-p frame)))
      (with-selected-frame (or frame (selected-frame))
        ;; People say it's bad to have EXIT on the tool bar, since users
        ;; might inadvertently click that button.
--- 262,268 ----
  
  (defun tool-bar-setup (&optional frame)
    (unless (or tool-bar-setup
! 	      (null tool-bar-mode))
      (with-selected-frame (or frame (selected-frame))
        ;; People say it's bad to have EXIT on the tool bar, since users
        ;; might inadvertently click that button.


Suppose you apply this patch, and start emacs on a tty with `emacs -nw',
then start a server with M-x server-start RET.  Then, open a graphical
client with `emacsclient -c'.

The result is an ugly, low-color toolbar; not the usual high-color
toolbar.

I thought this was due to GTK settings, but further investigation
reveals that the low-color settings come from these lines in
tool-bar-local-item-from-menu:

 (image-exp `(tool-bar-find-image
   (if (display-color-p)
       ',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec)
     ',(list pbm-spec xbm-spec xpm-lo-spec xpm-spec))))

Calling tool-bar-local-item-from-menu from the tty loads the lo-spec
images, whereas deferring this call until X is started up loads the
hi-spec images.  But deferring the call, as I suggested, doesn't seem to
be satisfactory either.

Probably the way to do this correctly is to change update_frame_tool_bar
to dynamically update the icons to deal with the latest display
settings.

Alternatively, we could drop support for "low-color" images on the
toolbar entirely.

Re: Tool-bar and multi-tty

[Chong Yidong]

Chong Yidong <cyd@stupidchicken.com> writes:

> Alternatively, we could drop support for "low-color" images on the
> toolbar entirely.

Just to elaborate, this would involve dropping the "xpm-lo-spec" image
options from tool-bar-local-item and tool-bar-local-item-from-menu, when
Emacs is searching for the icon image file.  Currently, whenever
(display-color-cells) is less than 256, low-color versions of the icons
are given a higher priority.

Presumably, low-color displays would still be able to display the icons
anyway, just uglier.  I don't know how many people use Emacs on such
displays.

Re: Tool-bar and multi-tty

[Eli Zaretskii]

> From: Chong Yidong <cyd@stupidchicken.com>
> Cc: dann@ics.uci.edu,  1133@emacsbugs.donarmstrong.com,  emacs-devel@gnu.org
> Date: Fri, 10 Oct 2008 15:43:30 -0400
> 
>   (defun tool-bar-setup (&optional frame)
>     (unless (or tool-bar-setup
> ! 	      (null tool-bar-mode))
>       (with-selected-frame (or frame (selected-frame))
>         ;; People say it's bad to have EXIT on the tool bar, since users
>         ;; might inadvertently click that button.
> 
> 
> Suppose you apply this patch, and start emacs on a tty with `emacs -nw',
> then start a server with M-x server-start RET.  Then, open a graphical
> client with `emacsclient -c'.
> 
> The result is an ugly, low-color toolbar; not the usual high-color
> toolbar.
> 
> I thought this was due to GTK settings, but further investigation
> reveals that the low-color settings come from these lines in
> tool-bar-local-item-from-menu:
> 
>  (image-exp `(tool-bar-find-image
>    (if (display-color-p)
>        ',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec)
>      ',(list pbm-spec xbm-spec xpm-lo-spec xpm-spec))))
> 
> Calling tool-bar-local-item-from-menu from the tty loads the lo-spec
> images, whereas deferring this call until X is started up loads the
> hi-spec images.  But deferring the call, as I suggested, doesn't seem to
> be satisfactory either.
> 
> Probably the way to do this correctly is to change update_frame_tool_bar
> to dynamically update the icons to deal with the latest display
> settings.
> 
> Alternatively, we could drop support for "low-color" images on the
> toolbar entirely.

I'd rather not drop support for low-color images, unless the dynamic
update you are suggesting turns out to be too hard.  Dynamically
updating the icons sounds right to me, especially since we could have
several frames with different color capabilities, at least in
principle.

Re: Tool-bar and multi-tty

[Stefan Monnier]

>  (image-exp `(tool-bar-find-image
>    (if (display-color-p)
>        ',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec)
>      ',(list pbm-spec xbm-spec xpm-lo-spec xpm-spec))))

> Calling tool-bar-local-item-from-menu from the tty loads the lo-spec
> images, whereas deferring this call until X is started up loads the
> hi-spec images.  But deferring the call, as I suggested, doesn't seem to
> be satisfactory either.

Why?  The `if' is left unevaluated in the image-exp.  So IIUC it's only
be evaluated later when the image is actually displayed.


        Stefan

Re: Tool-bar and multi-tty

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>  (image-exp `(tool-bar-find-image
>>    (if (display-color-p)
>>        ',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec)
>>      ',(list pbm-spec xbm-spec xpm-lo-spec xpm-spec))))
>
>> Calling tool-bar-local-item-from-menu from the tty loads the lo-spec
>> images, whereas deferring this call until X is started up loads the
>> hi-spec images.  But deferring the call, as I suggested, doesn't seem to
>> be satisfactory either.
>
> Why?  The `if' is left unevaluated in the image-exp.  So IIUC it's only
> be evaluated later when the image is actually displayed.

Aha: the number of display colors is checked earlier in the function.
If we put that check into the image expression instead, everything
works.

I've just checked in a fix into CVS.  I think this should resolve the
problems between multi-tty and tool bars.

Re: Tool-bar and multi-tty

[Andreas Schwab]

Chong Yidong <cyd@stupidchicken.com> writes:

> I've just checked in a fix into CVS.  I think this should resolve the
> problems between multi-tty and tool bars.

It crashes if tool-bar-mode is disabled, since tool-bar-map is empty.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Tool-bar and multi-tty

[Andreas Schwab]

Chong Yidong <cyd@stupidchicken.com> writes:

> I've just checked in a fix into CVS.  I think this should resolve the
> problems between multi-tty and tool bars.

Why does tool-bar-setup depend on the selected frame?  That does not
make any sense since it is never run more than once.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Tool-bar and multi-tty

[Chong Yidong]

Andreas Schwab <schwab@suse.de> writes:

> Why does tool-bar-setup depend on the selected frame?  That does not
> make any sense since it is never run more than once.

It used to be called in faces.el; that call is now not necessary, I
think.  I just checked in a fix to clean it up.

Re: Tool-bar and multi-tty

[Andreas Schwab]

Chong Yidong <cyd@stupidchicken.com> writes:

> Andreas Schwab <schwab@suse.de> writes:
>
>> Why does tool-bar-setup depend on the selected frame?  That does not
>> make any sense since it is never run more than once.
>
> It used to be called in faces.el; that call is now not necessary, I
> think.  I just checked in a fix to clean it up.

If you load compile before tool-bar-mode is enabled
then compilation-mode-tool-bar-map is never set.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Tool-bar and multi-tty

[Chong Yidong]

Andreas Schwab <schwab@suse.de> writes:

> If you load compile before tool-bar-mode is enabled then
> compilation-mode-tool-bar-map is never set.

Thanks for the heads-up.  I've checked in a fix to startup.el.

Now, we ought to be setting up tool-bar-mode during startup, except when
Emacs is noninteractive, rather than trying to be clever and setting it
up conditionally (which doesn't work well when multi-tty comes into the
picture).

Re: Tool-bar and multi-tty

[Stefan Monnier]

>> Bug 1133 notes a problem with tool-bars and multi-tty.  Basically,
>> tool-bar-map is not set up if the first frame is on a tty; it is only
>> initialized when the first graphical frame is created.  This causes
>> problems for major modes that define their own tool-bar maps.
>> 
>> I think the solution is to define a normal hook, tool-bar-setup-hook, to
>> be run after the tool-bar is set up successfully for the first time.

> This sounds like a very complicated solution.  Why can't we set up
> tool-bar-map unconditionally?  Perhaps changes that make that possible
> are much smaller and cleaner than what you suggest here?

My thoughts exactly... Why?


        Stefan

Re: Tool-bar and multi-tty

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> This sounds like a very complicated solution.  Why can't we set up
>> tool-bar-map unconditionally?  Perhaps changes that make that
>> possible are much smaller and cleaner than what you suggest here?
>
> My thoughts exactly... Why?

See my other reply.  I don't see any way to set up the tool-bar-map
unconditionally, without losing X and GTK settings.  Do you?