bug#49104: [PATCH] 28.0.50; Handle remapped commands for M-TAB in `flyspell-prog-mode'

bug#49104: [PATCH] 28.0.50; Handle remapped commands for M-TAB in `flyspell-prog-mode'

[Jim Porter]

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

`flyspell-prog-mode' uses the original M-TAB binding outside of
comments, which is nice (see bug#18533). However, it doesn't account
for remapped commands. For example, I remap `completion-at-point' to
`company-complete' when `company-mode' is active. If I hit M-TAB while
`company-mode' and `flyspell-prog-mode' are active, I get
`completion-at-point', which isn't what I want.

To reproduce this issue, you can do the following:

  emacs -Q
  (global-set-key [remap completion-at-point]
                  (lambda () (interactive) (message "remapped")))
  M-x flyspell-prog-mode
  M-TAB ;; Or C-M-i

I've attached a patch that fixes the issue (hopefully it works in all
cases; it works for me).

[-- Attachment #2: 0001-Handle-remapped-commands-for-original-M-TAB-binding-.patch --]
[-- Type: application/octet-stream, Size: 1299 bytes --]

From 734890adff84cf8fbec9cff0a5bb6ad620877216 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 18 Jun 2021 16:02:07 -0700
Subject: [PATCH] Handle remapped commands for original M-TAB binding in
 'flyspell-prog-mode'

* lisp/textmodes/flyspell.el (flyspell-prog-mode): Get the remapped
command for the original M-TAB binding if necessary.
---
 lisp/textmodes/flyspell.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index ba48e5de21..eb4c5cde28 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -410,9 +410,11 @@ flyspell-prog-mode
   (interactive)
   (setq flyspell-generic-check-word-predicate
         #'flyspell-generic-progmode-verify)
-  (setq-local flyspell--prev-meta-tab-binding
-              (or (local-key-binding "\M-\t" t)
-                  (global-key-binding "\M-\t" t)))
+  (let ((prev-meta-tab-binding (or (local-key-binding "\M-\t" t)
+                                   (global-key-binding "\M-\t" t))))
+    (setq-local flyspell--prev-meta-tab-binding
+                (or (command-remapping prev-meta-tab-binding)
+                    prev-meta-tab-binding)))
   (flyspell-mode 1)
   (run-hooks 'flyspell-prog-mode-hook))
 
-- 
2.25.1

bug#49104: [PATCH] 28.0.50; Handle remapped commands for M-TAB in `flyspell-prog-mode'

[Eli Zaretskii]

> From: Jim Porter <jporterbugs@gmail.com>
> Date: Fri, 18 Jun 2021 16:09:55 -0700
> 
> `flyspell-prog-mode' uses the original M-TAB binding outside of
> comments, which is nice (see bug#18533). However, it doesn't account
> for remapped commands. For example, I remap `completion-at-point' to
> `company-complete' when `company-mode' is active. If I hit M-TAB while
> `company-mode' and `flyspell-prog-mode' are active, I get
> `completion-at-point', which isn't what I want.
> 
> To reproduce this issue, you can do the following:
> 
>   emacs -Q
>   (global-set-key [remap completion-at-point]
>                   (lambda () (interactive) (message "remapped")))
>   M-x flyspell-prog-mode
>   M-TAB ;; Or C-M-i

Isn't it a bug that global/local-key-binding don't return such
remapped bindings, at least optionally?

bug#49104: [PATCH] 28.0.50; Handle remapped commands for M-TAB in `flyspell-prog-mode'

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

>> To reproduce this issue, you can do the following:
>> 
>>   emacs -Q
>>   (global-set-key [remap completion-at-point]
>>                   (lambda () (interactive) (message "remapped")))
>>   M-x flyspell-prog-mode
>>   M-TAB ;; Or C-M-i
>
> Isn't it a bug that global/local-key-binding don't return such
> remapped bindings, at least optionally?

I don't, but I wonder why flyspell-prog-mode uses those functions
instead of using just `key-binding` (which seems simpler and does pay
attention to remapping).

IOW, what would be the problem with the patch below?


        Stefan


diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index fb9361e45d..30b44daf54 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -415,8 +415,7 @@ flyspell-prog-mode
   (setq flyspell-generic-check-word-predicate
         #'flyspell-generic-progmode-verify)
   (setq-local flyspell--prev-meta-tab-binding
-              (or (local-key-binding "\M-\t" t)
-                  (global-key-binding "\M-\t" t)))
+              (key-binding "\M-\t"))
   (flyspell-mode 1)
   (run-hooks 'flyspell-prog-mode-hook))
 

bug#49104: [PATCH] 28.0.50; Handle remapped commands for M-TAB in `flyspell-prog-mode'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Jim Porter <jporterbugs@gmail.com>,  49104@debbugs.gnu.org
> Date: Sat, 19 Jun 2021 07:51:29 -0400
> 
> >> To reproduce this issue, you can do the following:
> >> 
> >>   emacs -Q
> >>   (global-set-key [remap completion-at-point]
> >>                   (lambda () (interactive) (message "remapped")))
> >>   M-x flyspell-prog-mode
> >>   M-TAB ;; Or C-M-i
> >
> > Isn't it a bug that global/local-key-binding don't return such
> > remapped bindings, at least optionally?
> 
> I don't

Why not?

> but I wonder why flyspell-prog-mode uses those functions
> instead of using just `key-binding`

Because the latter is not described in the ELisp manual where
global/local-key-binding are, and isn't referenced from there?

bug#49104: [PATCH] 28.0.50; Handle remapped commands for M-TAB in `flyspell-prog-mode'

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

>> > Isn't it a bug that global/local-key-binding don't return such
>> > remapped bindings, at least optionally?
>> I don't
> Why not?

Good question.  I think I meant to write "I don't know" instead of just
"I don't".

>> but I wonder why flyspell-prog-mode uses those functions
>> instead of using just `key-binding`
> Because the latter is not described in the ELisp manual where
> global/local-key-binding are, and isn't referenced from there?

In that case, `key-binding` might be a good replacement.
I see that using `key-binding` could find flyspell's own binding, tho, so we
should probably use the (first) patch below instead.

Tho I also wonder why we do the lookup when flyspell is enabled instead
of doing directly in flyspell-auto-correct-word as in the second
patch below.


        Stefan


diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index ba48e5de21..11ff0f426f 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -411,8 +411,8 @@ flyspell-prog-mode
   (setq flyspell-generic-check-word-predicate
         #'flyspell-generic-progmode-verify)
   (setq-local flyspell--prev-meta-tab-binding
-              (or (local-key-binding "\M-\t" t)
-                  (global-key-binding "\M-\t" t)))
+              (let ((flyspell-mode nil))
+                (key-binding "\M-\t")))
   (flyspell-mode 1)
   (run-hooks 'flyspell-prog-mode-hook))
 



diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index ba48e5de21..649057270e 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -401,18 +401,12 @@ flyspell-generic-progmode-verify
     (let ((f (get-text-property (1- (point)) 'face)))
       (memq f flyspell-prog-text-faces))))
 
-(defvar flyspell--prev-meta-tab-binding nil
-  "Records the binding of M-TAB in effect before flyspell was activated.")
-
 ;;;###autoload
 (defun flyspell-prog-mode ()
   "Turn on `flyspell-mode' for comments and strings."
   (interactive)
   (setq flyspell-generic-check-word-predicate
         #'flyspell-generic-progmode-verify)
-  (setq-local flyspell--prev-meta-tab-binding
-              (or (local-key-binding "\M-\t" t)
-                  (global-key-binding "\M-\t" t)))
   (flyspell-mode 1)
   (run-hooks 'flyspell-prog-mode-hook))
 
@@ -1990,13 +1984,10 @@ flyspell-auto-correct-word
   (interactive)
   ;; If we are not in the construct where flyspell should be active,
   ;; invoke the original binding of M-TAB, if that was recorded.
-  (if (and (local-variable-p 'flyspell--prev-meta-tab-binding)
-           (commandp flyspell--prev-meta-tab-binding t)
-           (functionp flyspell-generic-check-word-predicate)
-           (not (funcall flyspell-generic-check-word-predicate))
-           (equal (where-is-internal 'flyspell-auto-correct-word nil t)
-                  [?\M-\t]))
-      (call-interactively flyspell--prev-meta-tab-binding)
+  (if (and (functionp flyspell-generic-check-word-predicate)
+           (not (funcall flyspell-generic-check-word-predicate)))
+      (let ((flyspell-mode nil))
+        (execut-command (key-binding (this-command-keys))))
     (let ((pos     (point))
           (old-max (point-max)))
       ;; Flush a possibly stale cache from previous invocations of

bug#49104: [PATCH] 28.0.50; Handle remapped commands for M-TAB in `flyspell-prog-mode'

[Jim Porter]

On Sat, Jun 19, 2021 at 10:24 AM Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
> In that case, `key-binding` might be a good replacement.
> I see that using `key-binding` could find flyspell's own binding, tho, so we
> should probably use the (first) patch below instead.
>
> Tho I also wonder why we do the lookup when flyspell is enabled instead
> of doing directly in flyspell-auto-correct-word as in the second
> patch below.

Indeed, while I was looking into this some more, I saw that
`flyspell-auto-correct-word' is bound to both `M-TAB' and `C-.' in
`flyspell-mode-map'. Your second patch should properly delegate to the
right command for either binding, which would be very nice. The
original code (and my patch) always act as though you typed `M-TAB'
when delegating, which isn't quite right.

bug#49104: [PATCH] 28.0.50; Handle remapped commands for M-TAB in `flyspell-prog-mode'

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

I pushed a slightly cleaner version of my patch,


        Stefan


Jim Porter [2021-06-19 10:31:38] wrote:

> On Sat, Jun 19, 2021 at 10:24 AM Stefan Monnier
> <monnier@iro.umontreal.ca> wrote:
>> In that case, `key-binding` might be a good replacement.
>> I see that using `key-binding` could find flyspell's own binding, tho, so we
>> should probably use the (first) patch below instead.
>>
>> Tho I also wonder why we do the lookup when flyspell is enabled instead
>> of doing directly in flyspell-auto-correct-word as in the second
>> patch below.
>
> Indeed, while I was looking into this some more, I saw that
> `flyspell-auto-correct-word' is bound to both `M-TAB' and `C-.' in
> `flyspell-mode-map'. Your second patch should properly delegate to the
> right command for either binding, which would be very nice. The
> original code (and my patch) always act as though you typed `M-TAB'
> when delegating, which isn't quite right.

bug#49104: [PATCH] 28.0.50; Handle remapped commands for M-TAB in `flyspell-prog-mode'

[Jim Porter]

On Sun, Jun 27, 2021 at 12:07 PM Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>
> I pushed a slightly cleaner version of my patch,

I tried it out locally and everything works perfectly with my config.
Thanks for the fix.

- Jim

bug#49104: [PATCH] 28.0.50; Handle remapped commands for M-TAB in `flyspell-prog-mode'

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> On Sun, Jun 27, 2021 at 12:07 PM Stefan Monnier
> <monnier@iro.umontreal.ca> wrote:
>>
>> I pushed a slightly cleaner version of my patch,
>
> I tried it out locally and everything works perfectly with my config.
> Thanks for the fix.

This bug report was left open, so I'm closing it now.  (I only skimmed
it lightly, but if I understood correctly, Stefan's patched fixed the
reported problem.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no