bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Stefan Kangas]

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

I have implemented the following item from etc/TODO:

** The toolbar should show keyboard equivalents in its tooltips.

I'm an absolute beginner to Emacs internals and had a lot of fun
solving this.  Please let me know what you think.

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-Make-toolbar-show-keyboard-equivalents-in-its-toolti.patch --]
[-- Type: application/octet-stream, Size: 3671 bytes --]

From 082f6405334d4cffaa77cf17dccb3e64e923cdfd Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 9 Jun 2019 04:27:09 +0200
Subject: [PATCH] Make toolbar show keyboard equivalents in its tooltips

* src/keyboard.c (parse_tool_bar_item): Add equivalent key binding to
the tooltip string of toolbar buttons.
* etc/NEWS: Announce it.
* etc/TODO: Remove its entry.
* src/fns.c (concat4): New function.
* src/lisp.h (concat4): Declare.
---
 etc/NEWS       |  3 +++
 etc/TODO       |  2 --
 src/fns.c      |  7 +++++++
 src/keyboard.c | 13 +++++++++++++
 src/lisp.h     |  1 +
 5 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index c9da98b0ad..7726443551 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -121,6 +121,9 @@ This is intended mostly to help developers.
 ** Emacs now requires GTK 2.24 and GTK 3.10 for the GTK 2 and GTK 3
 builds respectively.
 
+---
+** The toolbar now shows the equivalent key binding in its tooltips.
+
 \f
 * Startup Changes in Emacs 27.1
 
diff --git a/etc/TODO b/etc/TODO
index f8c2d285ee..b9c8c0aae9 100644
--- a/etc/TODO
+++ b/etc/TODO
@@ -176,8 +176,6 @@ See the 'test' directory for examples.
 ** In Emacs Info, examples of using Customize should be clickable
    and they should create Custom buffers.
 
-** The toolbar should show keyboard equivalents in its tooltips.
-
 ** Add function to redraw the tool bar.
 
 ** Redesign the load-history data structure so it can cope better
diff --git a/src/fns.c b/src/fns.c
index eaa2c07fbe..f089503939 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -536,6 +536,13 @@ concat3 (Lisp_Object s1, Lisp_Object s2, Lisp_Object s3)
   return concat (3, ((Lisp_Object []) {s1, s2, s3}), Lisp_String, 0);
 }
 
+/* ARGSUSED */
+Lisp_Object
+concat4 (Lisp_Object s1, Lisp_Object s2, Lisp_Object s3, Lisp_Object s4)
+{
+  return concat (4, ((Lisp_Object []) {s1, s2, s3, s4}), Lisp_String, 0);
+}
+
 DEFUN ("append", Fappend, Sappend, 0, MANY, 0,
        doc: /* Concatenate all the arguments and make the result a list.
 The result is a list whose elements are the elements of all the arguments.
diff --git a/src/keyboard.c b/src/keyboard.c
index bb4d185c91..02ae05e08d 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -8297,6 +8297,19 @@ parse_tool_bar_item (Lisp_Object key, Lisp_Object item)
   if (CONSP (get_keymap (PROP (TOOL_BAR_ITEM_BINDING), 0, 1)))
     return 0;
 
+  /* If there is a key binding, add it to the help, which will be
+     displayed as a tooltip for this entry. */
+  Lisp_Object binding = PROP (TOOL_BAR_ITEM_BINDING);
+  Lisp_Object keys = Fwhere_is_internal (binding, Qnil, Qt, Qnil, Qnil);
+  if (!NILP (keys))
+    {
+      AUTO_STRING (beg, "  [");
+      AUTO_STRING (end, "]");
+      Lisp_Object orig = PROP (TOOL_BAR_ITEM_HELP);
+      Lisp_Object desc = Fkey_description (keys, Qnil);
+      set_prop (TOOL_BAR_ITEM_HELP, concat4 (orig, beg, desc, end));
+    }
+
   /* Enable or disable selection of item.  */
   if (!EQ (PROP (TOOL_BAR_ITEM_ENABLED_P), Qt))
     set_prop (TOOL_BAR_ITEM_ENABLED_P,
diff --git a/src/lisp.h b/src/lisp.h
index 77fc22d118..4faf9d0bf1 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3613,6 +3613,7 @@ #define CONS_TO_INTEGER(cons, type, var)				\
 extern Lisp_Object do_yes_or_no_p (Lisp_Object);
 extern Lisp_Object concat2 (Lisp_Object, Lisp_Object);
 extern Lisp_Object concat3 (Lisp_Object, Lisp_Object, Lisp_Object);
+extern Lisp_Object concat4 (Lisp_Object, Lisp_Object, Lisp_Object, Lisp_Object);
 extern bool equal_no_quit (Lisp_Object, Lisp_Object);
 extern Lisp_Object nconc2 (Lisp_Object, Lisp_Object);
 extern Lisp_Object assq_no_quit (Lisp_Object, Lisp_Object);
-- 
2.21.0

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Noam Postavsky]

Stefan Kangas <stefan@marxist.se> writes:

> I have implemented the following item from etc/TODO:
>
> ** The toolbar should show keyboard equivalents in its tooltips.
>
> I'm an absolute beginner to Emacs internals and had a lot of fun
> solving this.  Please let me know what you think.

> +      set_prop (TOOL_BAR_ITEM_HELP, concat4 (orig, beg, desc, end));

Instead of introducing concat4, you should be able to use

    CALLN (Fconcat, orig, beg, desc, end)

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Stefan Kangas]

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

Noam Postavsky <npostavs@gmail.com> writes:
> > +      set_prop (TOOL_BAR_ITEM_HELP, concat4 (orig, beg, desc, end));
>
> Instead of introducing concat4, you should be able to use
>
>     CALLN (Fconcat, orig, beg, desc, end)

That worked, thanks.  I've attached an updated patch.

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Make-toolbar-show-keyboard-equivalents-in-its-toolti.patch --]
[-- Type: application/octet-stream, Size: 2312 bytes --]

From ca965decc25a5d7a668fa35c1910fb353e3c9cd5 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 9 Jun 2019 04:27:09 +0200
Subject: [PATCH] Make toolbar show keyboard equivalents in its tooltips

* src/keyboard.c (parse_tool_bar_item): Add equivalent key binding to
the tooltip string of toolbar buttons.
* etc/NEWS: Announce it.
* etc/TODO: Remove its entry.
---
 etc/NEWS       |  3 +++
 etc/TODO       |  2 --
 src/keyboard.c | 13 +++++++++++++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index c9da98b0ad..7726443551 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -121,6 +121,9 @@ This is intended mostly to help developers.
 ** Emacs now requires GTK 2.24 and GTK 3.10 for the GTK 2 and GTK 3
 builds respectively.
 
+---
+** The toolbar now shows the equivalent key binding in its tooltips.
+
 \f
 * Startup Changes in Emacs 27.1
 
diff --git a/etc/TODO b/etc/TODO
index f8c2d285ee..b9c8c0aae9 100644
--- a/etc/TODO
+++ b/etc/TODO
@@ -176,8 +176,6 @@ See the 'test' directory for examples.
 ** In Emacs Info, examples of using Customize should be clickable
    and they should create Custom buffers.
 
-** The toolbar should show keyboard equivalents in its tooltips.
-
 ** Add function to redraw the tool bar.
 
 ** Redesign the load-history data structure so it can cope better
diff --git a/src/keyboard.c b/src/keyboard.c
index bb4d185c91..707c400b7d 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -8297,6 +8297,19 @@ parse_tool_bar_item (Lisp_Object key, Lisp_Object item)
   if (CONSP (get_keymap (PROP (TOOL_BAR_ITEM_BINDING), 0, 1)))
     return 0;
 
+  /* If there is a key binding, add it to the help, which will be
+     displayed as a tooltip for this entry. */
+  Lisp_Object binding = PROP (TOOL_BAR_ITEM_BINDING);
+  Lisp_Object keys = Fwhere_is_internal (binding, Qnil, Qt, Qnil, Qnil);
+  if (!NILP (keys))
+    {
+      AUTO_STRING (beg, "  [");
+      AUTO_STRING (end, "]");
+      Lisp_Object orig = PROP (TOOL_BAR_ITEM_HELP);
+      Lisp_Object desc = Fkey_description (keys, Qnil);
+      set_prop (TOOL_BAR_ITEM_HELP, CALLN (Fconcat, orig, beg, desc, end));
+    }
+
   /* Enable or disable selection of item.  */
   if (!EQ (PROP (TOOL_BAR_ITEM_ENABLED_P), Qt))
     set_prop (TOOL_BAR_ITEM_ENABLED_P,
-- 
2.21.0

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Mon, 10 Jun 2019 05:30:20 +0200
> Cc: 36156@debbugs.gnu.org
> 
> +  Lisp_Object binding = PROP (TOOL_BAR_ITEM_BINDING);
> +  Lisp_Object keys = Fwhere_is_internal (binding, Qnil, Qt, Qnil, Qnil);
> +  if (!NILP (keys))
> +    {
> +      AUTO_STRING (beg, "  [");
> +      AUTO_STRING (end, "]");

This is going to start a bikeshedding, but I'm not sure I like the
[FOO] format.  It's different from what we use in menus, for example.

Thanks.

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Stefan Kangas]

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

Eli Zaretskii <eliz@gnu.org> writes:
> This is going to start a bikeshedding, but I'm not sure I like the
> [FOO] format.  It's different from what we use in menus, for example.

FWIW, I tried both with brackets and parentheses and concluded
that the former is more readable, especially in cases where we
also use parentheses in the tooltip string.

Also, I'm not sure how important consistency with menus are here,
since the parentheses are not shown on my GTK Emacs -- the key
binding is displayed to the right.  On macOS, the key binding is
indeed shown in parentheses (but it would be better, IMO, if it
was also here displayed to the right with no parentheses).  Not
sure what happens in other toolkits.

That said, I'm fine either way.  I have attached a patch which
uses parentheses instead of brackets.  Please feel free too
install whichever version you prefer more.

And do it quick before anyone has time to start bikeshedding... ;)

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-Make-toolbar-show-keyboard-equivalents-in-its-toolti-3.patch --]
[-- Type: application/octet-stream, Size: 2312 bytes --]

From d1239c26a903d3654c6490b52d0defbc396615bd Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 9 Jun 2019 04:27:09 +0200
Subject: [PATCH] Make toolbar show keyboard equivalents in its tooltips

* src/keyboard.c (parse_tool_bar_item): Add equivalent key binding to
the tooltip string of toolbar buttons.
* etc/NEWS: Announce it.
* etc/TODO: Remove its entry.
---
 etc/NEWS       |  3 +++
 etc/TODO       |  2 --
 src/keyboard.c | 13 +++++++++++++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index c9da98b0ad..7726443551 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -121,6 +121,9 @@ This is intended mostly to help developers.
 ** Emacs now requires GTK 2.24 and GTK 3.10 for the GTK 2 and GTK 3
 builds respectively.
 
+---
+** The toolbar now shows the equivalent key binding in its tooltips.
+
 \f
 * Startup Changes in Emacs 27.1
 
diff --git a/etc/TODO b/etc/TODO
index f8c2d285ee..b9c8c0aae9 100644
--- a/etc/TODO
+++ b/etc/TODO
@@ -176,8 +176,6 @@ See the 'test' directory for examples.
 ** In Emacs Info, examples of using Customize should be clickable
    and they should create Custom buffers.
 
-** The toolbar should show keyboard equivalents in its tooltips.
-
 ** Add function to redraw the tool bar.
 
 ** Redesign the load-history data structure so it can cope better
diff --git a/src/keyboard.c b/src/keyboard.c
index bb4d185c91..6a62957f3e 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -8297,6 +8297,19 @@ parse_tool_bar_item (Lisp_Object key, Lisp_Object item)
   if (CONSP (get_keymap (PROP (TOOL_BAR_ITEM_BINDING), 0, 1)))
     return 0;
 
+  /* If there is a key binding, add it to the help, which will be
+     displayed as a tooltip for this entry. */
+  Lisp_Object binding = PROP (TOOL_BAR_ITEM_BINDING);
+  Lisp_Object keys = Fwhere_is_internal (binding, Qnil, Qt, Qnil, Qnil);
+  if (!NILP (keys))
+    {
+      AUTO_STRING (beg, "  (");
+      AUTO_STRING (end, ")");
+      Lisp_Object orig = PROP (TOOL_BAR_ITEM_HELP);
+      Lisp_Object desc = Fkey_description (keys, Qnil);
+      set_prop (TOOL_BAR_ITEM_HELP, CALLN (Fconcat, orig, beg, desc, end));
+    }
+
   /* Enable or disable selection of item.  */
   if (!EQ (PROP (TOOL_BAR_ITEM_ENABLED_P), Qt))
     set_prop (TOOL_BAR_ITEM_ENABLED_P,
-- 
2.21.0

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 11 Jun 2019 23:28:50 +0200
> Cc: Noam Postavsky <npostavs@gmail.com>, 36156@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > This is going to start a bikeshedding, but I'm not sure I like the
> > [FOO] format.  It's different from what we use in menus, for example.
> 
> FWIW, I tried both with brackets and parentheses and concluded
> that the former is more readable, especially in cases where we
> also use parentheses in the tooltip string.
> 
> Also, I'm not sure how important consistency with menus are here,
> since the parentheses are not shown on my GTK Emacs -- the key
> binding is displayed to the right.  On macOS, the key binding is
> indeed shown in parentheses (but it would be better, IMO, if it
> was also here displayed to the right with no parentheses).  Not
> sure what happens in other toolkits.
> 
> That said, I'm fine either way.  I have attached a patch which
> uses parentheses instead of brackets.  Please feel free too
> install whichever version you prefer more.
> 
> And do it quick before anyone has time to start bikeshedding... ;)

Thanks, pushed.

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Juri Linkov]

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

> Please let me know what you think.

Today I found a regression in this change: when a tool-bar item
doesn't use a :help keyword (like e.g. in gud-menu-map) then
the tooltip displays just keyboard equivalents in its tooltip.
It used to display the caption in previous Emacs versions.
I believe it could be fixed by this patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 36156_1.patch --]
[-- Type: text/x-diff, Size: 503 bytes --]

diff --git a/src/keyboard.c b/src/keyboard.c
index 30686a2589..1b9a603ca1 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -8304,6 +8304,10 @@ parse_tool_bar_item (Lisp_Object key, Lisp_Object item)
       AUTO_STRING (end, ")");
       Lisp_Object orig = PROP (TOOL_BAR_ITEM_HELP);
       Lisp_Object desc = Fkey_description (keys, Qnil);
+
+      if (NILP (orig))
+        orig = PROP (TOOL_BAR_ITEM_CAPTION);
+
       set_prop (TOOL_BAR_ITEM_HELP, CALLN (Fconcat, orig, beg, desc, end));
     }
 

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Stefan Kangas]

Juri Linkov <juri@linkov.net> writes:

> Today I found a regression in this change: when a tool-bar item
> doesn't use a :help keyword (like e.g. in gud-menu-map) then
> the tooltip displays just keyboard equivalents in its tooltip.
> It used to display the caption in previous Emacs versions.
> I believe it could be fixed by this patch:

I can reproduce this regression in the gud toolbar on current master.
Your patch fixes it and looks good to me.

Thanks for finding and resolving this.

Best regards,
Stefan Kangas

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sun, 25 Aug 2019 13:03:06 +0200
> Cc: 36156@debbugs.gnu.org
> 
> Juri Linkov <juri@linkov.net> writes:
> 
> > Today I found a regression in this change: when a tool-bar item
> > doesn't use a :help keyword (like e.g. in gud-menu-map) then
> > the tooltip displays just keyboard equivalents in its tooltip.
> > It used to display the caption in previous Emacs versions.
> > I believe it could be fixed by this patch:
> 
> I can reproduce this regression in the gud toolbar on current master.
> Your patch fixes it and looks good to me.

Are we sure TOOL_BAR_ITEM_CAPTION will always produce human-readable
text?

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Are we sure TOOL_BAR_ITEM_CAPTION will always produce human-readable
> text?

I think so, because we do this above (quoted to avoid gmail mangling the code):

>  /* Get the caption of the item.  If the caption is not a string,
>     evaluate it to get a string.  If we don't get a string, skip this
>     item.  */
>  caption = XCAR (item);
>  if (!STRINGP (caption))
>    {
>      caption = menu_item_eval_property (caption);
>      if (!STRINGP (caption))
>    return 0;
>    }
>  set_prop (TOOL_BAR_ITEM_CAPTION, caption);

When this function returns 0, we don't append this item to the items
that will be displayed.

Best regards,
Stefan Kangas

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sun, 25 Aug 2019 14:58:42 +0200
> Cc: Juri Linkov <juri@linkov.net>, 36156@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Are we sure TOOL_BAR_ITEM_CAPTION will always produce human-readable
> > text?
> 
> I think so, because we do this above (quoted to avoid gmail mangling the code):
> 
> >  /* Get the caption of the item.  If the caption is not a string,
> >     evaluate it to get a string.  If we don't get a string, skip this
> >     item.  */
> >  caption = XCAR (item);
> >  if (!STRINGP (caption))
> >    {
> >      caption = menu_item_eval_property (caption);
> >      if (!STRINGP (caption))
> >    return 0;
> >    }
> >  set_prop (TOOL_BAR_ITEM_CAPTION, caption);
> 
> When this function returns 0, we don't append this item to the items
> that will be displayed.

That just makes sure it's a string, but what kind of string is that?
A caption can be something unpalatable, like "OpNuFil".

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Juri Linkov]

>> > Are we sure TOOL_BAR_ITEM_CAPTION will always produce human-readable
>> > text?
>>
>> I think so, because we do this above (quoted to avoid gmail mangling the code):
>
> That just makes sure it's a string, but what kind of string is that?
> A caption can be something unpalatable, like "OpNuFil".

Currently HELP's fallback to CAPTION is a standard way
to handle absence of HELP:

note_tool_bar_highlight has this code:

  help_echo_string = AREF (f->tool_bar_items, prop_idx + TOOL_BAR_ITEM_HELP);
  if (NILP (help_echo_string))
    help_echo_string = AREF (f->tool_bar_items, prop_idx + TOOL_BAR_ITEM_CAPTION);

xg_tool_bar_help_callback has this code:

      help = AREF (f->tool_bar_items, idx + TOOL_BAR_ITEM_HELP);
      if (NILP (help))
        help = AREF (f->tool_bar_items, idx + TOOL_BAR_ITEM_CAPTION);

The patch added the same handling to parse_tool_bar_item:

       Lisp_Object orig = PROP (TOOL_BAR_ITEM_HELP);
+      if (NILP (orig))
+        orig = PROP (TOOL_BAR_ITEM_CAPTION);

bug#36156: [PATCH] Make toolbar show keyboard equivalents in its tooltips

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Stefan Kangas <stefan@marxist.se>,  36156@debbugs.gnu.org
> Date: Sun, 25 Aug 2019 23:18:20 +0300
> 
> > That just makes sure it's a string, but what kind of string is that?
> > A caption can be something unpalatable, like "OpNuFil".
> 
> Currently HELP's fallback to CAPTION is a standard way
> to handle absence of HELP:

Then I guess we are OK here.