bug#68334: 29.1; tool-bar-make-keymap-1 does not work on terminals

[Jared Finder via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

On 2024-01-09 04:40, Eli Zaretskii wrote:
>> Date: Mon, 08 Jan 2024 13:39:25 -0800
>> From:  Jared Finder via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> I'd like tool-bar-make-keymap-1 to return a sensible keymap on
>> terminals.  This is only a small change from current code, patch
>> attached.
> 
> I don't see any problems with the change, but please add comments
> there explaining that some parts of the code are supposed to work on
> frames where images cannot be displayed, because it is very easy to
> mistakenly assume this only needs to work on GUI frames.

New patch attached with comment added.

For the other discussions about making per-window tool bars part of 
core, I expect a handful of additional patches so I can file a separate 
bug report if that makes tracking things easier.

> Btw, what happens if display-images-p returns non-nil, but the session
> doesn't support images of the type used for tool-bar icons?

Then a keybind is generated with no image spec because find-image will 
not find any supported image.  Such binds are silently ignored by the 
current tool bar logic on PGTK and Mac builds.  You can see this 
behavior today in M-x customize, where the " Toggle hiding all values " 
bind is not displayed.  I was going to report this as a separate bug 
because this behavior seems poor -- I think a text only tool bar button 
would be preferred.

   -- MJF

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-tool-bar.el-tool-bar-make-keymap-1-Populate-on-.patch --]
[-- Type: text/x-diff; name=0001-lisp-tool-bar.el-tool-bar-make-keymap-1-Populate-on-.patch, Size: 1740 bytes --]

From 7581bfda5dfe3ad5732da6b518b8c77e350b1b9e Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Mon, 8 Jan 2024 13:20:25 -0800
Subject: [PATCH] * lisp/tool-bar.el (tool-bar-make-keymap-1): Populate on
 terminals

---
 lisp/tool-bar.el | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/lisp/tool-bar.el b/lisp/tool-bar.el
index 4ca81fb01e0..96b61c7b229 100644
--- a/lisp/tool-bar.el
+++ b/lisp/tool-bar.el
@@ -165,6 +165,8 @@ tool-bar-make-keymap
             base-keymap)
       base-keymap)))
 
+;; This function should return binds even if images can not be
+;; displayed so the tool bar can still be displayed on terminals.
 (defun tool-bar-make-keymap-1 (&optional map)
   "Generate an actual keymap from `tool-bar-map', without caching.
 MAP is either a keymap to use as a source for menu items, or nil,
@@ -180,15 +182,14 @@ tool-bar-make-keymap-1
 			 (consp image-exp)
 			 (not (eq (car image-exp) 'image))
 			 (fboundp (car image-exp)))
-		(if (not (display-images-p))
-		    (setq bind nil)
-		  (let ((image (eval image-exp)))
-		    (unless (and image (image-mask-p image))
-		      (setq image (append image '(:mask heuristic))))
-		    (setq bind (copy-sequence bind)
-			  plist (nthcdr (if (consp (nth 4 bind)) 5 4)
-					bind))
-		    (plist-put plist :image image))))
+		(let ((image (and (display-images-p)
+                                  (eval image-exp))))
+		  (unless (and image (image-mask-p image))
+		    (setq image (append image '(:mask heuristic))))
+		  (setq bind (copy-sequence bind)
+			plist (nthcdr (if (consp (nth 4 bind)) 5 4)
+				      bind))
+		  (plist-put plist :image image)))
 	      bind))
 	  (or map tool-bar-map)))
 
-- 
2.39.2