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

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]

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

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.

Context:  I am writing a package that displays toolbars attached to an 
Emacs window instead of attached to the frame.  You can see the package 
at <https://github.com/chaosemer/window-tool-bar>.

There's no intrinsic reason toolbars can't be useful in terminals and 
adding additional (usually ignored) keymap entries seems very safe.

   -- MJF

In GNU Emacs 29.1 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.37,
  cairo version 1.16.0) of 2023-09-11, modified by Debian built on melete
System Description: Debian GNU/Linux 12 (bookworm)

Configured using:
  'configure --build x86_64-linux-gnu --prefix=/usr
  --sharedstatedir=/var/lib --libexecdir=/usr/libexec
  --localstatedir=/var/lib --infodir=/usr/share/info
  --mandir=/usr/share/man --with-libsystemd --with-pop=yes
  
--enable-locallisppath=/etc/emacs:/usr/local/share/emacs/29.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/29.1/site-lisp:/usr/share/emacs/site-lisp
  --with-sound=alsa --without-gconf --with-mailutils
  --with-native-compilation --build x86_64-linux-gnu --prefix=/usr
  --sharedstatedir=/var/lib --libexecdir=/usr/libexec
  --localstatedir=/var/lib --infodir=/usr/share/info
  --mandir=/usr/share/man --with-libsystemd --with-pop=yes
  
--enable-locallisppath=/etc/emacs:/usr/local/share/emacs/29.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/29.1/site-lisp:/usr/share/emacs/site-lisp
  --with-sound=alsa --without-gconf --with-mailutils
  --with-native-compilation --with-pgtk 'CFLAGS=-g -O2
  -ffile-prefix-map=/build/emacs-N816CI/emacs-29.1+1=. 
-fstack-protector-strong
  -Wformat -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
  -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP
NOTIFY INOTIFY PDUMPER PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM GTK3 ZLIB

Important settings:
   value of $LANG: en_US.UTF-8
   locale-coding-system: utf-8-unix

Major mode: ELisp/l

Minor modes in effect:
   global-window-tool-bar-mode: t
   window-tool-bar-mode: t
   pixel-scroll-precision-mode: t
   recentf-mode: t
   global-subword-mode: t
   subword-mode: t
   global-form-feed-st-mode: t
   form-feed-st-mode: t
   icomplete-mode: t
   fido-mode: t
   electric-pair-mode: t
   delete-selection-mode: t
   cua-mode: t
   bar-cursor-mode: t
   url-handler-mode: t
   global-eldoc-mode: t
   eldoc-mode: t
   show-paren-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   context-menu-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   window-divider-mode: t
   column-number-mode: t
   line-number-mode: t
   transient-mark-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr smerge-mode diff pulse jka-compr help-fns
radix-tree find-func xref project misearch multi-isearch mm-archive
gnutls network-stream url-cache url-http url-auth url-gw nsm emacsbug
ediff-vers ediff ediff-merg ediff-mult ediff-wind ediff-diff ediff-help
ediff-init ediff-util vc-hg vc-svn vc-dir ewoc vc checkdoc lisp-mnt
vc-git diff-mode vc-dispatcher cursor-sensor finder-inf comp comp-cstr
cl-extra help-mode markdown-mode rx color thingatpt noutline outline
cus-edit pp cus-start cus-load window-tool-bar easy-mmode tab-line
pixel-scroll recentf tree-widget wid-edit cap-words superword subword
form-feed-st icomplete elec-pair delsel cua-base bar-cursor ls-lisp
advice log-edit message sendmail yank-media puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util
time-date mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047
rfc2045 mm-util ietf-drums mail-prsvr mailabbrev mail-utils gmm-utils
mailheader pcvs-util add-log warnings icons compile text-property-search
comint ansi-osc ansi-color ring init-dir bar-cursor-autoloads
benchmark-init-autoloads dired-icon-autoloads form-feed-st-autoloads
init-dir-autoloads markdown-mode-autoloads modus-themes-autoloads
package-lint-autoloads compat-autoloads info slime-autoloads
window-tool-bar-autoloads package browse-url url url-proxy url-privacy
url-expand url-methods url-history url-cookie generate-lisp-file
url-domsuf url-util mailcap url-handlers url-parse auth-source cl-seq
eieio eieio-core password-cache json map byte-opt bytecomp byte-compile
url-vars modus-vivendi-theme modus-themes cl-macs pcase subr-x
cl-loaddefs cl-lib gv rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/pgtk-win pgtk-win term/common-win pgtk-dnd tool-bar dnd fontset
image regexp-opt fringe tabulated-list replace newcomment text-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo gtk pgtk
lcms2 multi-tty make-network-process native-compile emacs)

Memory information:
((conses 16 604030 131636)
  (symbols 48 25469 2)
  (strings 32 156826 21765)
  (string-bytes 1 7239778)
  (vectors 16 41746)
  (vector-slots 8 769733 48672)
  (floats 8 276 1190)
  (intervals 56 4786 466)
  (buffers 984 33))

[-- 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: 1333 bytes --]

From b01cdf53a45a60332de48285d510790907152ea8 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 | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/lisp/tool-bar.el b/lisp/tool-bar.el
index 4ca81fb01e0..36a80522c83 100644
--- a/lisp/tool-bar.el
+++ b/lisp/tool-bar.el
@@ -180,15 +180,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

bug#68334: window-tool-bar (bug#68334)

[Daniel Mendler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> 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.
>
> Context:  I am writing a package that displays toolbars attached to an Emacs
> window instead of attached to the frame.  You can see the package at
> <https://github.com/chaosemer/window-tool-bar>.

Your idea of attaching toolbars to windows instead of frames sounds
interesting, e.g., in the context of touch screens. Would it make sense
to add this feature or your package to Emacs directly?

Daniel

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

[Stefan Kangas]

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

> Context:  I am writing a package that displays toolbars attached to an
> Emacs window instead of attached to the frame.  You can see the package
> at <https://github.com/chaosemer/window-tool-bar>.

Shouldn't this be a feature in core Emacs?  I believe that we have
discussed that in the past.

Eli, WDYT?

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

[Juri Linkov]

>> Context:  I am writing a package that displays toolbars attached to an
>> Emacs window instead of attached to the frame.  You can see the package
>> at <https://github.com/chaosemer/window-tool-bar>.
>
> Shouldn't this be a feature in core Emacs?  I believe that we have
> discussed that in the past.

This would be a nice feature, but I think whether to include it in core
Emacs depends on the implementation.  If it overwrites the tab line,
then this might be disappointing for users of tab-line-mode.
The README mentions adding (:eval (window-tool-bar-string))
to tab-line-format, but I don't understand how this could
solve the problem.

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

[Eli Zaretskii]

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

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?

bug#68334: window-tool-bar (bug#68334)

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org, jared@finder.org
> Date: Tue, 09 Jan 2024 00:04:20 +0100
> From:  Daniel Mendler via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Jared Finder via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs@gnu.org> writes:
> 
> > Context:  I am writing a package that displays toolbars attached to an Emacs
> > window instead of attached to the frame.  You can see the package at
> > <https://github.com/chaosemer/window-tool-bar>.
> 
> Your idea of attaching toolbars to windows instead of frames sounds
> interesting, e.g., in the context of touch screens. Would it make sense
> to add this feature or your package to Emacs directly?

I'd love to see a usable tool bar in "M-x gdb" on TTY frames, FWIW.
Right now, we have a very poor substitute there, which looks not very
nice, to put it mildly.

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

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Mon, 8 Jan 2024 20:57:47 -0800
> Cc: Eli Zaretskii <eliz@gnu.org>
> 
> Jared Finder via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs@gnu.org> writes:
> 
> > Context:  I am writing a package that displays toolbars attached to an
> > Emacs window instead of attached to the frame.  You can see the package
> > at <https://github.com/chaosemer/window-tool-bar>.
> 
> Shouldn't this be a feature in core Emacs?  I believe that we have
> discussed that in the past.
> 
> Eli, WDYT?

I won't mind, if Jared thinks the package will be useful in enough
situations or for enough packages.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Context:  I am writing a package that displays toolbars attached to an
>> > Emacs window instead of attached to the frame.  You can see the package
>> > at <https://github.com/chaosemer/window-tool-bar>.
>>
>> Shouldn't this be a feature in core Emacs?  I believe that we have
>> discussed that in the past.
>>
>> Eli, WDYT?
>
> I won't mind, if Jared thinks the package will be useful in enough
> situations or for enough packages.

I didn't intend to propose including the existing implementation as is,
which I hadn't looked closely at.  Having done that just now, it seems
like it's implemented on the tab bar, as Juri has pointed out.  I think
this is probably not the right thing for a feature in Emacs itself.

This feature would be most useful as a first class feature, but that
might require C changes.  My intention here is merely to ask if anyone
(perhaps Jared) would be interested in working on that.

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

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Tue, 9 Jan 2024 10:39:05 -0800
> Cc: jared@finder.org, 68334@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Context:  I am writing a package that displays toolbars attached to an
> >> > Emacs window instead of attached to the frame.  You can see the package
> >> > at <https://github.com/chaosemer/window-tool-bar>.
> >>
> >> Shouldn't this be a feature in core Emacs?  I believe that we have
> >> discussed that in the past.
> >>
> >> Eli, WDYT?
> >
> > I won't mind, if Jared thinks the package will be useful in enough
> > situations or for enough packages.
> 
> I didn't intend to propose including the existing implementation as is,
> which I hadn't looked closely at.  Having done that just now, it seems
> like it's implemented on the tab bar, as Juri has pointed out.  I think
> this is probably not the right thing for a feature in Emacs itself.

How else do you expect this to be implemented on TTY frames?  It can
either overwrite parts of the menu bar, or it could overwrite parts of
the tab bar.  Something's gotta give, no?

I also don't understand why the tab bar is deemed more important than
the tool bar.  Let's let users decide what they prefer.  (Of course,
if there's a way to let them have the cake and eat it, too, that would
be the best.)

> This feature would be most useful as a first class feature, but that
> might require C changes.

IMO, usurping one more screen line from TTY frames could be
problematic, especially on terminals that display a relatively small
number of lines.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> I didn't intend to propose including the existing implementation as is,
>> which I hadn't looked closely at.  Having done that just now, it seems
>> like it's implemented on the tab bar, as Juri has pointed out.  I think
>> this is probably not the right thing for a feature in Emacs itself.
>
> How else do you expect this to be implemented on TTY frames?  It can
> either overwrite parts of the menu bar, or it could overwrite parts of
> the tab bar.  Something's gotta give, no?
>
> I also don't understand why the tab bar is deemed more important than
> the tool bar.  Let's let users decide what they prefer.  (Of course,
> if there's a way to let them have the cake and eat it, too, that would
> be the best.)
>
>> This feature would be most useful as a first class feature, but that
>> might require C changes.
>
> IMO, usurping one more screen line from TTY frames could be
> problematic, especially on terminals that display a relatively small
> number of lines.

You could add another line, so that you could use the tab-bar and the
tool-bar at the same time.  That would provide the most flexibility for
users, I think.  They would themselves get to decide how many lines to
sacrifice.  (Many users also have huge screens these days, including
vertical ones.)

That said, I don't at all object to the current implementation as an
optional feature in core, if you think it's a good idea.  There's no
reason to allow perfect to be the enemy of the good.

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]

On 2024-01-09 11:08, Stefan Kangas wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>>> I didn't intend to propose including the existing implementation as 
>>> is,
>>> which I hadn't looked closely at.  Having done that just now, it 
>>> seems
>>> like it's implemented on the tab bar, as Juri has pointed out.  I 
>>> think
>>> this is probably not the right thing for a feature in Emacs itself.
>> 
>> How else do you expect this to be implemented on TTY frames?  It can
>> either overwrite parts of the menu bar, or it could overwrite parts of
>> the tab bar.  Something's gotta give, no?
>> 
>> I also don't understand why the tab bar is deemed more important than
>> the tool bar.  Let's let users decide what they prefer.  (Of course,
>> if there's a way to let them have the cake and eat it, too, that would
>> be the best.)
>> 
>>> This feature would be most useful as a first class feature, but that
>>> might require C changes.
>> 
>> IMO, usurping one more screen line from TTY frames could be
>> problematic, especially on terminals that display a relatively small
>> number of lines.
> 
> You could add another line, so that you could use the tab-bar and the
> tool-bar at the same time.  That would provide the most flexibility for
> users, I think.  They would themselves get to decide how many lines to
> sacrifice.  (Many users also have huge screens these days, including
> vertical ones.)
> 
> That said, I don't at all object to the current implementation as an
> optional feature in core, if you think it's a good idea.  There's no
> reason to allow perfect to be the enemy of the good.

If this goes in core, I'd like to make window-tool-bar-mode and 
tab-line-mode work nicely together.  Right now they both just overwrite 
tab-line-format, but I think a better implementation would result in 
them both appears in the tab-line side by side when both are enabled.  
In other words, if both are enabled tab-line-format would have a value 
like

((:eval (tab-line-format)) " " (:eval (window-tool-bar-string))).


I'm also happy to also add an additional line.  At that point there'd be 
three lines, so would it be worth it to add an option to control the 
order of the lines?  I can imagine a variable that contains lines / 
prefix events for lines above a buffer and controls the order.  I 
certainly would appreciate this flexibility.

Let me know what you think is the right way to proceed.

   -- MJF

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

[Eli Zaretskii]

> Date: Tue, 09 Jan 2024 11:28:17 -0800
> From: Jared Finder <jared@finder.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, 68334@debbugs.gnu.org
> 
> If this goes in core, I'd like to make window-tool-bar-mode and 
> tab-line-mode work nicely together.  Right now they both just overwrite 
> tab-line-format, but I think a better implementation would result in 
> them both appears in the tab-line side by side when both are enabled.  
> In other words, if both are enabled tab-line-format would have a value 
> like
> 
> ((:eval (tab-line-format)) " " (:eval (window-tool-bar-string))).
> 
> 
> I'm also happy to also add an additional line.  At that point there'd be 
> three lines, so would it be worth it to add an option to control the 
> order of the lines?  I can imagine a variable that contains lines / 
> prefix events for lines above a buffer and controls the order.  I 
> certainly would appreciate this flexibility.

I'd prefer not to add another line, if possible.

Thanks.

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

[Juri Linkov]

> If this goes in core, I'd like to make window-tool-bar-mode and
> tab-line-mode work nicely together.  Right now they both just overwrite
> tab-line-format, but I think a better implementation would result in them
> both appears in the tab-line side by side when both are enabled.  In other
> words, if both are enabled tab-line-format would have a value like
>
> ((:eval (tab-line-format)) " " (:eval (window-tool-bar-string))).
>
> I'm also happy to also add an additional line.  At that point there'd be
> three lines, so would it be worth it to add an option to control the order
> of the lines?  I can imagine a variable that contains lines / prefix events
> for lines above a buffer and controls the order.  I certainly would
> appreciate this flexibility.

Instead of asking the users to set tab-line-format to the expression above
adding a simple option would be nice.  Such option could define
how to combine the window tool-line with the tab-line.

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]

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

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

[Eli Zaretskii]

> Date: Wed, 10 Jan 2024 14:59:25 -0800
> From: Jared Finder <jared@finder.org>
> Cc: 68334@debbugs.gnu.org
> 
> > 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.

Thanks, installed on master.

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

OK, so I'm closing this bug.