bug#788: menu indications of key bindings for remapped commands

bug#788: menu indications of key bindings for remapped commands

[Chong Yidong]

> As described in:
>
> http://lists.gnu.org/archive/html/emacs-devel/2007-05/msg01020.html
>
> key bindings defined via remap look odd in menus.

I changed the code so that [remap foo] commands are not displayed in the
menus.

Ideally, we would follow the remappings, but I think this could mess up
the menu cache (e.g., if the remapped-to command is rebound).  So that's
a larger, post-release project.

bug#788: menu indications of key bindings for remapped commands

[Drew Adams]

> From: Chong Yidong Sent: Sunday, March 15, 2009 11:00 AM
> > As described in:
> > http://lists.gnu.org/archive/html/emacs-devel/2007-05/msg01020.html
> > key bindings defined via remap look odd in menus.
> 
> I changed the code so that [remap foo] commands are not 
> displayed in the menus.
> 
> Ideally, we would follow the remappings, but I think this 
> could mess up the menu cache (e.g., if the remapped-to
> command is rebound).  So that's a larger, post-release project.

I haven't tried after your fix, but from your description of the fix, (a) the
bug is not fixed at all, and (b) the change is negative.

1. It is inappropriate to *remove* such commands (menu items) from the menu. Or
did you mean just remove the confusing key bindings from the menu items?

The latter would be OK as a temporary workaround, but it represents a loss of
information, and it does not fix the bug. To fix the bug, the real, user-level
key binding needs to be shown.


2. The bug needs to be fixed properly, as Richard requested two years ago. It's
inappropriate to just close it now with a statement that we are too close to the
release to fix it properly.

The goal should be to improve Emacs by fixing bugs, not simply to reduce the bug
count by closing bugs without fixing them.

If you want to keep this bug open and fix it sometime after the release, that
would be appropriate, but closing it with a non-fix fix is inappropriate.

bug#788: menu indications of key bindings for remapped commands

[Chong Yidong]

"Drew Adams" <drew.adams@oracle.com> writes:

> The goal should be to improve Emacs by fixing bugs, not simply to
> reduce the bug count by closing bugs without fixing them.
>
> If you want to keep this bug open and fix it sometime after the
> release, that would be appropriate, but closing it with a non-fix fix
> is inappropriate.

This bug is still open.  Spare me the lecture on bug management.

bug#788: menu indications of key bindings for remapped commands

[Drew Adams]

> From: Chong Yidong Sent: Sunday, March 15, 2009 6:17 PM
> This bug is still open.  Spare me the lecture on bug management.

Keeping the bug open is good. Thanks.

How about the question wrt what change was actually made?

> > I changed the code so that [remap foo] commands are not 
> > displayed in the menus.
>
> 1. It is inappropriate to *remove* such commands (menu items) 
> from the menu. Or did you mean just remove the confusing key
> bindings from the menu items?

You didn't confirm or deny whether you in fact removed the items from the menu
(as you said) or just removed the key bindings (which would be OK as temporary
workaround).

If you did as you said, then please revert your change. It is more important to
keep the menu items, even if their key-binding mentions are not yet ideal.

bug#788: menu indications of key bindings for remapped commands

[Chong Yidong]

"Drew Adams" <drew.adams@oracle.com> writes:

>> > I changed the code so that [remap foo] commands are not 
>> > displayed in the menus.
>>
>> 1. It is inappropriate to *remove* such commands (menu items) 
>> from the menu. Or did you mean just remove the confusing key
>> bindings from the menu items?
>
> You didn't confirm or deny whether you in fact removed the items from
> the menu (as you said) or just removed the key bindings (which would
> be OK as temporary workaround).

The latter.

bug#788: menu indications of key bindings for remapped commands

[Drew Adams]

> >> > I changed the code so that [remap foo] commands are not 
> >> > displayed in the menus.
> >>
> >> 1. It is inappropriate to *remove* such commands (menu items) 
> >> from the menu. Or did you mean just remove the confusing key
> >> bindings from the menu items?
> >
> > You didn't confirm or deny whether you in fact removed the 
> > items from the menu (as you said) or just removed the key
> > bindings (which would be OK as temporary workaround).
> 
> The latter.

Great, thanks. And thanks for clearing that up.

bug#788: menu indications of key bindings for remapped commands

[Stefan Monnier]

> Ideally, we would follow the remappings, but I think this could mess up
> the menu cache (e.g., if the remapped-to command is rebound).

What menu cache?  AFAIK the keybindings shown in menus are not cached
any more.


        Stefan

bug#788: menu indications of key bindings for remapped commands

[Chong Yidong]

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

>> Ideally, we would follow the remappings, but I think this could mess up
>> the menu cache (e.g., if the remapped-to command is rebound).
>
> What menu cache?  AFAIK the keybindings shown in menus are not cached
> any more.

I meant the where_is_cache, which currently stores such keybindings as
[remap foo].  In parse_menu_item, we call Fwhere_is_internal, which
consults the cache and gets this result.  The cache is consulted because
the `firstonly' argument for this call is `t', which causes
Fwhere_is_internal to consult the cache; if it's nil, Fwhere_is_internal
does not use the cache.

bug#788: menu indications of key bindings for remapped commands

[Stefan Monnier]

>>> Ideally, we would follow the remappings, but I think this could mess up
>>> the menu cache (e.g., if the remapped-to command is rebound).
>> What menu cache?  AFAIK the keybindings shown in menus are not cached
>> any more.
> I meant the where_is_cache, which currently stores such keybindings as
> [remap foo].

I see, yes, this cache.  But then I don't see what problem you're
thining of.

BTW, shouldn't this remap-handling be done in Fwhere_is_internal rather
than in parse_menu_item?


        Stefan

bug#788: menu indications of key bindings for remapped commands

[Chong Yidong]

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

> BTW, shouldn't this remap-handling be done in Fwhere_is_internal
> rather than in parse_menu_item?

So, the idea is for Fwhere_is_internal to extract the cache entry (which
has the form [remap foo]), and follow the remapping before returning the
value to its caller?

Yeah, that might work, though I don't know how much existing code
depends on the current behavior.

bug#788: menu indications of key bindings for remapped commands

[Stefan Monnier]

>> BTW, shouldn't this remap-handling be done in Fwhere_is_internal
>> rather than in parse_menu_item?
> So, the idea is for Fwhere_is_internal to extract the cache entry (which
> has the form [remap foo]), and follow the remapping before returning the
> value to its caller?

Well, the behavior equivalent to your current code would be to return
nil, but yes, it could also try to "reverse-remap" the command back to
a key binding (being careful to try and avoid infinite cycles).

> Yeah, that might work, though I don't know how much existing code
> depends on the current behavior.

Good point.  Let's leave it in parse_menu_item for now, but revisit
it later.


        Stefan

bug#788: menu indications of key bindings for remapped commands

[Drew Adams]

This bug is still open.

Also, there is a regression. Yidong's temporary workaround of removing the key
reminder from the menu item no longer applies to the pretest: the <remap> <...>
key reminder has returned.

In GNU Emacs 23.1.91.1 (i386-mingw-nt5.1.2600)
 of 2010-01-02 on PRETEST
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4)'

A proper fix that shows the correct key (vs shows no key) is still needed.

bug#788: menu indications of key bindings for remapped commands

[Stefan Monnier]

> This bug is still open.

I have just installed a patch which should fix it.
At least the bar-mode example works right in my test.
Please confirm,


        Stefan

bug#788: menu indications of key bindings for remapped commands

[Drew Adams]

> I have just installed a patch which should fix it.
> At least the bar-mode example works right in my test.
> Please confirm,

What file(s) did you patch?

Anyway, I cannot get to the repository
(http://bazaar.launchpad.net/%7Evcs-imports/emacs/trunk/files/head%3A/lisp/). It
seems to still be down. I reported this to emacs-devel on 2010/01/15.

bug#788: menu indications of key bindings for remapped commands

[Drew Adams]

> Anyway, I cannot get to the repository
> (http://bazaar.launchpad.net/%7Evcs-imports/emacs/trunk/files/head%3A/lisp/).
> It seems to still be down. I reported this to emacs-devel on 2010/01/15.

(I got no reply to my 1/15 report, BTW.)

I can get to the parent directory and some of the sibling directories (e.g.
lib-src), but not to the lisp directory. I just get an "Internal Server Error"
(not very user-friendly). It has been this way for quite a while.

FWIW, the repository is useless to me if I cannot get to it using http to
download source files.