From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: Proposed patch for lookup-key Date: Thu, 14 Dec 2017 21:57:09 -0500 Message-ID: References: NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1513306679 16183 195.159.176.226 (15 Dec 2017 02:57:59 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 15 Dec 2017 02:57:59 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Dec 15 03:57:56 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ePgCB-0003xt-FZ for ged-emacs-devel@m.gmane.org; Fri, 15 Dec 2017 03:57:55 +0100 Original-Received: from localhost ([::1]:43913 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ePgCG-0002cP-QP for ged-emacs-devel@m.gmane.org; Thu, 14 Dec 2017 21:58:00 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:46820) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ePgBg-0002cJ-F6 for emacs-devel@gnu.org; Thu, 14 Dec 2017 21:57:25 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ePgBb-0000m4-J6 for emacs-devel@gnu.org; Thu, 14 Dec 2017 21:57:24 -0500 Original-Received: from [195.159.176.226] (port=47323 helo=blaine.gmane.org) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1ePgBb-0000kC-BW for emacs-devel@gnu.org; Thu, 14 Dec 2017 21:57:19 -0500 Original-Received: from list by blaine.gmane.org with local (Exim 4.84_2) (envelope-from ) id 1ePgBR-0001iF-DC for emacs-devel@gnu.org; Fri, 15 Dec 2017 03:57:09 +0100 X-Injected-Via-Gmane: http://gmane.org/ Original-Lines: 117 Original-X-Complaints-To: usenet@blaine.gmane.org Cancel-Lock: sha1:Hi3uiX2nfKMR0n5uFWfErTrx4yc= X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 195.159.176.226 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:221081 Archived-At: [ I remember thinking about this need many years ago and thinking "bah, I'll get to it when the need becomes pressing enough, but not now". FWIW currently my "best" answer in Elisp for the problem you're suggesting to solve is to `lookup-key` on the first n-1 events and then use `map-keymap` to traverse the resulting final keymap looking for the nᵗʰ event. ] > I'm wondering if the attached patch would be acceptable. The idea was to > add an optional argument to lookup-key to prevent it from stripping this > information about the key bindings. Comments below. Stefan > * src/keyboard.c (read_char): Adjust call to access_keymap > (menu_bar_items): Adjust call to access_keymap > (tool_bar_items): Adjust call to access_keymap > (follow_key): Adjust call to access_keymap > (access_keymap_keyremap): Adjust call to access_keymap > * src/keymap.c: Change get_keyelt declaration > (access_keymap_1): Add menus arg and adjust recursive calls > (get_keyelt): Add menus arg > (Fdefine_key): Adjust call to access_keymap > (Fcommand_remapping): Adjust call to Flookup_key > (Flookup_key): Add menus arg and adjust call to access_keymap > (Fkey_binding): Adjust call to Flookup_key > (Flocal_key_binding): Adjust call to Flookup_key > (Fglobal_key_binding): Adjust call to Flookup_key > (Fminor_mode_key_binding): Adjust call to Flookup_key > (accessible_keymaps_1): Adjust call to get_keyelt > (Faccessible_keymaps): Adjust call to Flookup_key > (shadow_lookup): Adjust call to Flookup_key > (where_is_internal_1): Adjust call to get_keyelt > (describe_map_tree): Adjust call to Flookup_key > (describe_map): Adjust calls to get_keyelt and Flookup_key > (describe_vector): Adjust calls to get_keyelt and Flookup_key > * src/keymap.h: Adjust access_keymap declaration You don't need to say "Adjust calls to" for all those functions. You can just say something like: (access_keymap_1): Add 'menus' arg; adjust all calls. Otherwise the actual meat of the change gets drowned within all the minor mechanical adjustments. > @@ -368,7 +368,8 @@ Return PARENT. PARENT should be nil or another keymap. */) > static Lisp_Object > access_keymap_1 (Lisp_Object map, Lisp_Object idx, > - bool t_ok, bool noinherit, bool autoload) > + bool t_ok, bool noinherit, bool autoload, > + bool menus) Please update the comment that's just before that function to document this `menus` argument. > static Lisp_Object > -get_keyelt (Lisp_Object object, bool autoload) > +get_keyelt (Lisp_Object object, bool autoload, bool menus) > { > while (1) > { > - if (!(CONSP (object))) > - /* This is really the value. */ > + if (!(CONSP (object)) || menus) > + /* This is really the value or we do not want to process > + menu-items. */ > return object; If `menus` is true, then we will always just return `object` unchanged. So an alternative would be to test `menus` before calling this function (so we don't need to add the `menus` argument to this function). Probably doesn't make much of a difference, tho. > +A non-nil value for MENUS makes `lookup-key` return full menu-items > +instead of just the associated definition. */) Elements like (STRING . COMMAND) aren't only used for menus, so I'd prefer to use another name. I don't we actually have a good name at hand for that, but a common name used elsewhere for those objects is "menu item", so maybe we could use that. BTW, one problem (and one of the reasons why I didn't implement the feature back then using an approach like the one you suggest) is that this treats a "menu item" as opaque (meaning it hides everything in lower keymaps). Yet if we do let get_keyelt do its job to look inside the menu-item it may find that the binding is sometimes nil (e.g. for bindings which are made dynamically conditional using a :filter function), and other times we'll find a binding which is a keymap which is then combined with other keymaps found in lower-precedence keymaps (e.g. some menu bar menus have entries provided by global-map as well as from other keymap). To give a concrete example, (lookup-key (current-active-maps) [menu-bar] nil 'menu-item) could return a menu-item containing a keymap with just one element (the major mode's menu, or maybe one of the minor mode's menu), whereas (lookup-key (current-active-maps) [menu-bar]) returns a keymap with many more elements (all the top-level menus, including those from the global map, those from the major mode and those from the minor modes). Solving this problem is a lot of work (because of all the different possible cases), and will rarely make a difference, so I'm not sure that it's a valid argument against your patch. Stefan