GNU bug report logs - #36156
[PATCH] Make toolbar show keyboard equivalents in its tooltips

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 36156 in the body.
You can then email your comments to 36156 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Make toolbar show keyboard equivalents in its tooltips
Date: Mon, 10 Jun 2019 03:14:12 +0200

[Message part 1 (text/plain, inline)]

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

[0001-Make-toolbar-show-keyboard-equivalents-in-its-toolti.patch (application/octet-stream, attachment)]

Message #8 received at 36156 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 36156 <at> debbugs.gnu.org
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in its
 tooltips
Date: Sun, 09 Jun 2019 22:06:31 -0400

Stefan Kangas <stefan <at> 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)

Message #11 received at 36156 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 36156 <at> debbugs.gnu.org
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in its
 tooltips
Date: Mon, 10 Jun 2019 05:30:20 +0200

[Message part 1 (text/plain, inline)]

Noam Postavsky <npostavs <at> 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

[0001-Make-toolbar-show-keyboard-equivalents-in-its-toolti.patch (application/octet-stream, attachment)]

Message #14 received at 36156 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 36156 <at> debbugs.gnu.org, npostavs <at> gmail.com
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in its
 tooltips
Date: Mon, 10 Jun 2019 19:54:47 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Mon, 10 Jun 2019 05:30:20 +0200
> Cc: 36156 <at> 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.

Message #17 received at 36156 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36156 <at> debbugs.gnu.org, Noam Postavsky <npostavs <at> gmail.com>
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in its
 tooltips
Date: Tue, 11 Jun 2019 23:28:50 +0200

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> 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

[0001-Make-toolbar-show-keyboard-equivalents-in-its-toolti-3.patch (application/octet-stream, attachment)]

Message #22 received at 36156-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 36156-done <at> debbugs.gnu.org, npostavs <at> gmail.com
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in its
 tooltips
Date: Sat, 22 Jun 2019 12:13:29 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Tue, 11 Jun 2019 23:28:50 +0200
> Cc: Noam Postavsky <npostavs <at> gmail.com>, 36156 <at> debbugs.gnu.org
> 
> Eli Zaretskii <eliz <at> 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.

Message #29 received at 36156 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 36156 <at> debbugs.gnu.org
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in
 its tooltips
Date: Sat, 24 Aug 2019 23:40:10 +0300

[Message part 1 (text/plain, inline)]

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

[36156_1.patch (text/x-diff, inline)]

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));
     }
 

Message #32 received at 36156 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Juri Linkov <juri <at> linkov.net>
Cc: 36156 <at> debbugs.gnu.org
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in its
 tooltips
Date: Sun, 25 Aug 2019 13:03:06 +0200

Juri Linkov <juri <at> 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

Message #35 received at 36156 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 36156 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in its
 tooltips
Date: Sun, 25 Aug 2019 14:46:22 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Sun, 25 Aug 2019 13:03:06 +0200
> Cc: 36156 <at> debbugs.gnu.org
> 
> Juri Linkov <juri <at> 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?

Message #38 received at 36156 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36156 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in its
 tooltips
Date: Sun, 25 Aug 2019 14:58:42 +0200

Eli Zaretskii <eliz <at> 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

Message #41 received at 36156 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 36156 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in its
 tooltips
Date: Sun, 25 Aug 2019 16:10:14 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Sun, 25 Aug 2019 14:58:42 +0200
> Cc: Juri Linkov <juri <at> linkov.net>, 36156 <at> debbugs.gnu.org
> 
> Eli Zaretskii <eliz <at> 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".

Message #44 received at 36156 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 36156 <at> debbugs.gnu.org, Stefan Kangas <stefan <at> marxist.se>
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in
 its tooltips
Date: Sun, 25 Aug 2019 23:18:20 +0300

>> > 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);

Message #47 received at 36156 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 36156 <at> debbugs.gnu.org, stefan <at> marxist.se
Subject: Re: bug#36156: [PATCH] Make toolbar show keyboard equivalents in
 its tooltips
Date: Mon, 26 Aug 2019 09:26:35 +0300

> From: Juri Linkov <juri <at> linkov.net>
> Cc: Stefan Kangas <stefan <at> marxist.se>,  36156 <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.