GNU bug report logs - #31707
[PATCH 1/1] ido: add ido-fallback special variable

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 31707 in the body.
You can then email your comments to 31707 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: Christophe Junke <junke.christophe <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Christophe Junke <junke.christophe <at> gmail.com>
Subject: [PATCH 1/1] ido: add ido-fallback special variable
Date: Mon,  4 Jun 2018 10:39:43 +0200

The Ido module has been compiled with "lexical-binding: t" for some
time now. Previously, when the bindings were dynamic, it was possible
for other packages to modify the "fallback" variables declared inside
"ido-file-internal" and "ido-buffer-internal".

In particular, that was the case in magit-extras.el, which runs
magit-status on current path when exiting Ido. This feature is now
broken since "fallback" is lexical. For reference, the current code
for "ido-enter-magit-status" does the following:

    (with-no-warnings ; FIXME these are internal variables
      (setq ido-exit 'fallback fallback 'magit-status))
    (exit-minibuffer)

I think it would be cleaner to have it do:

    (ido-fallback-command 'magit-status)

The current patch:

- Introduces an ido-fallback special variable, which, when set,
  overrides the local, lexical, "fallback" variable; it does so only
  when ido-exit is set to 'fallback.

- Adds an optional parameter to "ido-fallback-command" that is used to
  specify which fallback command to run on exit.

* lisp/ido.el (ido-fallback): add new variable,
(ido-buffer-internal): reset ido-fallback to nil before prompting user,
(ido-buffer-internal): use ido-fallback when ido-exit is 'fallback,
(ido-file-internal): reset ido-fallback to nil before prompting user,
(ido-file-internal): use ido-fallback when ido-exit is 'fallback,
(ido-fallback-command): add optional argument fallback-command
---
 lisp/ido.el | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index 705e7dd630..00d0f4f446 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -1242,6 +1242,9 @@ Only used if `ido-use-virtual-buffers' is non-nil.")
 ;; Dynamically bound in ido-read-internal.
 (defvar ido-completing-read)
 
+;; If dynamically set when ido-exit is 'fallback, overrides fallback command.
+(defvar ido-fallback nil)
+
 ;;; FUNCTIONS
 
 (defun ido-active (&optional merge)
@@ -2220,6 +2223,7 @@ If cursor is not at the end of the user input, move to end of input."
 	(run-hook-with-args 'ido-before-fallback-functions
 			    (or fallback 'switch-to-buffer))
 	(call-interactively (or fallback 'switch-to-buffer)))
+    (setq ido-fallback nil)
     (let* ((ido-context-switch-command switch-cmd)
 	   (ido-current-directory nil)
 	   (ido-directory-nonreadable nil)
@@ -2245,7 +2249,7 @@ If cursor is not at the end of the user input, move to end of input."
 
        ((eq ido-exit 'fallback)
 	(let ((read-buffer-function nil))
-	  (setq this-command (or fallback 'switch-to-buffer))
+	  (setq this-command (or ido-fallback fallback 'switch-to-buffer))
 	  (run-hook-with-args 'ido-before-fallback-functions this-command)
 	  (call-interactively this-command)))
 
@@ -2341,6 +2345,7 @@ If cursor is not at the end of the user input, move to end of input."
   ;; Internal function for ido-find-file and friends
   (unless item
     (setq item 'file))
+  (setq ido-fallback nil)
   (let ((ido-current-directory (ido-expand-directory default))
 	(ido-context-switch-command switch-cmd)
 	ido-directory-nonreadable ido-directory-too-big
@@ -2412,7 +2417,7 @@ If cursor is not at the end of the user input, move to end of input."
 	;; we don't want to change directory of current buffer.
 	(let ((default-directory ido-current-directory)
 	      (read-file-name-function nil))
-	  (setq this-command (or fallback 'find-file))
+	  (setq this-command (or ido-fallback fallback 'find-file))
 	  (run-hook-with-args 'ido-before-fallback-functions this-command)
 	  (call-interactively this-command)))
 
@@ -2821,13 +2826,15 @@ If no buffer or file exactly matching the prompt exists, maybe create a new one.
   (setq ido-exit 'takeprompt)
   (exit-minibuffer))
 
-(defun ido-fallback-command ()
-  "Fallback to non-Ido version of current command."
+(defun ido-fallback-command (&optional fallback-command)
+  "Fallback to non-Ido version of current command.
+The optional fallback-command argument indicates which command to run."
   (interactive)
   (let ((i (length ido-text)))
     (while (> i 0)
       (push (aref ido-text (setq i (1- i))) unread-command-events)))
   (setq ido-exit 'fallback)
+  (setq ido-fallback fallback-command)
   (exit-minibuffer))
 
 (defun ido-enter-find-file ()
-- 
2.14.1

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christophe Junke <junke.christophe <at> gmail.com>
Cc: 31707 <at> debbugs.gnu.org
Subject: Re: bug#31707: [PATCH 1/1] ido: add ido-fallback special variable
Date: Sat, 09 Jun 2018 10:00:56 +0300

> From: Christophe Junke <junke.christophe <at> gmail.com>
> Date: Mon,  4 Jun 2018 10:39:43 +0200
> Cc: Christophe Junke <junke.christophe <at> gmail.com>
> 
> The Ido module has been compiled with "lexical-binding: t" for some
> time now. Previously, when the bindings were dynamic, it was possible
> for other packages to modify the "fallback" variables declared inside
> "ido-file-internal" and "ido-buffer-internal".
> 
> In particular, that was the case in magit-extras.el, which runs
> magit-status on current path when exiting Ido. This feature is now
> broken since "fallback" is lexical. For reference, the current code
> for "ido-enter-magit-status" does the following:
> 
>     (with-no-warnings ; FIXME these are internal variables
>       (setq ido-exit 'fallback fallback 'magit-status))
>     (exit-minibuffer)
> 
> I think it would be cleaner to have it do:
> 
>     (ido-fallback-command 'magit-status)
> 
> The current patch:
> 
> - Introduces an ido-fallback special variable, which, when set,
>   overrides the local, lexical, "fallback" variable; it does so only
>   when ido-exit is set to 'fallback.
> 
> - Adds an optional parameter to "ido-fallback-command" that is used to
>   specify which fallback command to run on exit.

Thanks.

However, if the problem was caused by using lexical-binding in ido.el,
then I wonder why you need anything except one additional line:

  (defvar ido-fallback)

This should make ido-fallback a dynamically-bound variable, and all
the rest should "just work" as it did before.  Right?

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

From: Noam Postavsky <npostavs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: junke.christophe <at> gmail.com, 31707 <at> debbugs.gnu.org
Subject: Re: bug#31707: [PATCH 1/1] ido: add ido-fallback special variable
Date: Sat, 23 Jun 2018 21:52:29 -0400

[moving discussion in #31783 back to original #31707]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Noam Postavsky <npostavs <at> gmail.com>
>> Cc: junke.christophe <at> gmail.com,  31783 <at> debbugs.gnu.org
>> Date: Fri, 22 Jun 2018 07:32:14 -0400
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> >
>> > What new feature is being introduced there?  I couldn't see anything
>> > new, just fixing a breakage.
>> 
>> The optional parameter to ido-fallback-command is new.
>> 
>> The ido-fallback is sort of a new variable, although it's arguably just
>> a proper prefixing of the old `fallback' one.
>
> Maybe I'm confused about which patch will eventually go in.  Can you
> show it?

The one in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31707#5.

Depending how strict we want to be about changes, we could split off the
ido-fallback-command parameter addition from the ido-fallback variable.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: junke.christophe <at> gmail.com, 31707 <at> debbugs.gnu.org
Subject: Re: bug#31707: [PATCH 1/1] ido: add ido-fallback special variable
Date: Sun, 24 Jun 2018 17:54:33 +0300

> From: Noam Postavsky <npostavs <at> gmail.com>
> Cc: junke.christophe <at> gmail.com,  31707 <at> debbugs.gnu.org
> Date: Sat, 23 Jun 2018 21:52:29 -0400
> 
> >> > What new feature is being introduced there?  I couldn't see anything
> >> > new, just fixing a breakage.
> >> 
> >> The optional parameter to ido-fallback-command is new.
> >> 
> >> The ido-fallback is sort of a new variable, although it's arguably just
> >> a proper prefixing of the old `fallback' one.
> >
> > Maybe I'm confused about which patch will eventually go in.  Can you
> > show it?
> 
> The one in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31707#5.

So I wasn't confused, thanks.

> Depending how strict we want to be about changes, we could split off the
> ido-fallback-command parameter addition from the ido-fallback variable.

No, I don't think this would be necessary.  We are adding a variable
to fix breakage.  I think this can go to emacs-26 as-is.

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

From: Noam Postavsky <npostavs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: junke.christophe <at> gmail.com, 31707 <at> debbugs.gnu.org
Subject: Re: bug#31707: [PATCH 1/1] ido: add ido-fallback special variable
Date: Mon, 25 Jun 2018 20:40:23 -0400

tags 31707 fixed
close 31707 26.2
quit

Eli Zaretskii <eliz <at> gnu.org> writes:

> No, I don't think this would be necessary.  We are adding a variable
> to fix breakage.  I think this can go to emacs-26 as-is.

Done.

[1: 12c77f6918]: 2018-06-25 20:05:53 -0400
  Add ido-fallback special variable (Bug#31707)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=12c77f6918c4a60dbbae3f716a58300b4026e8da

Previous Next

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