* bug#31707: [PATCH 1/1] ido: add ido-fallback special variable
@ 2018-06-04 8:39 Christophe Junke
2018-06-09 7:00 ` Eli Zaretskii
0 siblings, 1 reply; 17+ messages in thread
From: Christophe Junke @ 2018-06-04 8:39 UTC (permalink / raw
To: 31707; +Cc: Christophe Junke
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* bug#31707: [PATCH 1/1] ido: add ido-fallback special variable
2018-06-04 8:39 bug#31707: [PATCH 1/1] ido: add ido-fallback special variable Christophe Junke
@ 2018-06-09 7:00 ` Eli Zaretskii
2018-06-11 8:23 ` bug#31783: [PATCH v2] ido.el: define a special ido-fallback variable Christophe Junke
0 siblings, 1 reply; 17+ messages in thread
From: Eli Zaretskii @ 2018-06-09 7:00 UTC (permalink / raw
To: Christophe Junke; +Cc: 31707
> From: Christophe Junke <junke.christophe@gmail.com>
> Date: Mon, 4 Jun 2018 10:39:43 +0200
> Cc: Christophe Junke <junke.christophe@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?
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#31783: [PATCH v2] ido.el: define a special ido-fallback variable
2018-06-09 7:00 ` Eli Zaretskii
@ 2018-06-11 8:23 ` Christophe Junke
2018-06-11 12:19 ` Noam Postavsky
0 siblings, 1 reply; 17+ messages in thread
From: Christophe Junke @ 2018-06-11 8:23 UTC (permalink / raw
To: 31783; +Cc: Christophe Junke
- Introduce a global, special ido-fallback variable
- Rename existing occurrences of fallback to ido-fallback
This allows to retain the behaviour that was available prior to
introducing per-file lexical-scope, namely that other modules could
change what fallback command to call when ido-exit is set to
'fallback.
---
Hi,
I agree that it is simpler to rename the existing variable, and just
add a defvar declaration. Here is a different version of the patch
which does only this.
Thank you for your review and your time.
lisp/ido.el | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/lisp/ido.el b/lisp/ido.el
index 705e7dd630..358e856d4a 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)
+;; Indicates which fallback command to call when ido-exit is 'fallback.
+(defvar ido-fallback nil)
+
;;; FUNCTIONS
(defun ido-active (&optional merge)
@@ -2213,13 +2216,13 @@ If cursor is not at the end of the user input, move to end of input."
(exit-minibuffer)))
;;; MAIN FUNCTIONS
-(defun ido-buffer-internal (method &optional fallback prompt default initial switch-cmd)
+(defun ido-buffer-internal (method &optional ido-fallback prompt default initial switch-cmd)
;; Internal function for ido-switch-buffer and friends
(if (not ido-mode)
(progn
(run-hook-with-args 'ido-before-fallback-functions
- (or fallback 'switch-to-buffer))
- (call-interactively (or fallback 'switch-to-buffer)))
+ (or ido-fallback 'switch-to-buffer))
+ (call-interactively (or ido-fallback 'switch-to-buffer)))
(let* ((ido-context-switch-command switch-cmd)
(ido-current-directory nil)
(ido-directory-nonreadable nil)
@@ -2245,7 +2248,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 'switch-to-buffer))
(run-hook-with-args 'ido-before-fallback-functions this-command)
(call-interactively this-command)))
@@ -2337,7 +2340,7 @@ If cursor is not at the end of the user input, move to end of input."
;; Add final slash to result in case it was missing from DEFAULT-DIRECTORY.
(ido-final-slash (expand-file-name (or dir default-directory)) t))
-(defun ido-file-internal (method &optional fallback default prompt item initial switch-cmd)
+(defun ido-file-internal (method &optional ido-fallback default prompt item initial switch-cmd)
;; Internal function for ido-find-file and friends
(unless item
(setq item 'file))
@@ -2412,7 +2415,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 'find-file))
(run-hook-with-args 'ido-before-fallback-functions this-command)
(call-interactively this-command)))
@@ -2496,10 +2499,10 @@ If cursor is not at the end of the user input, move to end of input."
((eq method 'read-only)
(ido-record-work-file filename)
(setq filename (concat ido-current-directory filename))
- (ido-record-command fallback filename)
+ (ido-record-command ido-fallback filename)
(ido-record-work-directory)
- (run-hook-with-args 'ido-before-fallback-functions fallback)
- (funcall fallback filename))
+ (run-hook-with-args 'ido-before-fallback-functions ido-fallback)
+ (funcall ido-fallback filename))
((eq method 'insert)
(ido-record-work-file filename)
--
2.14.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* bug#31783: [PATCH v2] ido.el: define a special ido-fallback variable
2018-06-11 8:23 ` bug#31783: [PATCH v2] ido.el: define a special ido-fallback variable Christophe Junke
@ 2018-06-11 12:19 ` Noam Postavsky
2018-06-11 12:54 ` Christophe Junke
2018-06-11 15:28 ` Eli Zaretskii
0 siblings, 2 replies; 17+ messages in thread
From: Noam Postavsky @ 2018-06-11 12:19 UTC (permalink / raw
To: Christophe Junke; +Cc: 31783
merge 31783 31707
quit
Christophe Junke <junke.christophe@gmail.com> writes:
> I agree that it is simpler to rename the existing variable, and just
> add a defvar declaration. Here is a different version of the patch
> which does only this.
> +;; Indicates which fallback command to call when ido-exit is 'fallback.
> +(defvar ido-fallback nil)
> -(defun ido-buffer-internal (method &optional fallback prompt default initial switch-cmd)
> +(defun ido-buffer-internal (method &optional ido-fallback prompt default initial switch-cmd)
I believe this doesn't work, function parameters are always lexically
bound. Compare
; -*- lexical-binding: t -*-
(setq lexical-binding t) ; for use in *scratch*
(defvar x nil)
(disassemble (lambda (x y)
(+ x y)))
(let ((x 1))
(disassemble (lambda (y)
(+ x y))))
So I think your first patch was fine.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#31783: [PATCH v2] ido.el: define a special ido-fallback variable
2018-06-11 12:19 ` Noam Postavsky
@ 2018-06-11 12:54 ` Christophe Junke
2018-06-11 15:28 ` Eli Zaretskii
1 sibling, 0 replies; 17+ messages in thread
From: Christophe Junke @ 2018-06-11 12:54 UTC (permalink / raw
To: Noam Postavsky; +Cc: 31783
[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]
> I believe this doesn't work, function parameters are always lexically
bound.
Indeed, there is even a warning from disassemble about the lexical argument
shadowing the dynamic one.
Thank you for noticing that.
> So I think your first patch was fine.
Fine.
To recap, the first patch also redefines "ido-fallback-command" so that it
accepts an optional parameter (the fallback command).
Is that ok for you? The idea was to let Ido be exited with a custom
fallback command through a function, and without requiring other packages
to set a variable directly.
Thank you.
On Mon, Jun 11, 2018 at 2:19 PM, Noam Postavsky <npostavs@gmail.com> wrote:
> merge 31783 31707
> quit
>
> Christophe Junke <junke.christophe@gmail.com> writes:
>
> > I agree that it is simpler to rename the existing variable, and just
> > add a defvar declaration. Here is a different version of the patch
> > which does only this.
>
> > +;; Indicates which fallback command to call when ido-exit is 'fallback.
> > +(defvar ido-fallback nil)
>
> > -(defun ido-buffer-internal (method &optional fallback prompt default
> initial switch-cmd)
> > +(defun ido-buffer-internal (method &optional ido-fallback prompt
> default initial switch-cmd)
>
> I believe this doesn't work, function parameters are always lexically
> bound. Compare
>
> ; -*- lexical-binding: t -*-
> (setq lexical-binding t) ; for use in *scratch*
>
> (defvar x nil)
>
> (disassemble (lambda (x y)
> (+ x y)))
>
> (let ((x 1))
> (disassemble (lambda (y)
> (+ x y))))
>
> So I think your first patch was fine.
>
[-- Attachment #2: Type: text/html, Size: 2426 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#31783: [PATCH v2] ido.el: define a special ido-fallback variable
2018-06-11 12:19 ` Noam Postavsky
2018-06-11 12:54 ` Christophe Junke
@ 2018-06-11 15:28 ` Eli Zaretskii
[not found] ` <CAFDFyRiHzxOB7Q6uV1hPYmuC3KfiqJRCmk=nrQ5wTPWUue_W4Q@mail.gmail.com>
1 sibling, 1 reply; 17+ messages in thread
From: Eli Zaretskii @ 2018-06-11 15:28 UTC (permalink / raw
To: Noam Postavsky; +Cc: junke.christophe, 31783
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Mon, 11 Jun 2018 08:19:03 -0400
> Cc: 31783@debbugs.gnu.org
>
> Christophe Junke <junke.christophe@gmail.com> writes:
>
> > I agree that it is simpler to rename the existing variable, and just
> > add a defvar declaration. Here is a different version of the patch
> > which does only this.
>
> > +;; Indicates which fallback command to call when ido-exit is 'fallback.
> > +(defvar ido-fallback nil)
>
> > -(defun ido-buffer-internal (method &optional fallback prompt default initial switch-cmd)
> > +(defun ido-buffer-internal (method &optional ido-fallback prompt default initial switch-cmd)
>
> I believe this doesn't work, function parameters are always lexically
> bound. Compare
>
> ; -*- lexical-binding: t -*-
> (setq lexical-binding t) ; for use in *scratch*
>
> (defvar x nil)
>
> (disassemble (lambda (x y)
> (+ x y)))
>
> (let ((x 1))
> (disassemble (lambda (y)
> (+ x y))))
>
> So I think your first patch was fine.
There's some misunderstanding here, most probably mine. Sorry; please
help me understand what am I missing.
The original report said that the problem was caused by using
lexical-binding in ido.el, so I proposed to defvar the offending
variable to make it dynamically bound, which is the boilerplate
solution for all such problems. I thought that was all that was
needed, and I definitely didn't suggest to rename anything.
What did I miss?
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-06-26 0:40 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-04 8:39 bug#31707: [PATCH 1/1] ido: add ido-fallback special variable Christophe Junke
2018-06-09 7:00 ` Eli Zaretskii
2018-06-11 8:23 ` bug#31783: [PATCH v2] ido.el: define a special ido-fallback variable Christophe Junke
2018-06-11 12:19 ` Noam Postavsky
2018-06-11 12:54 ` Christophe Junke
2018-06-11 15:28 ` Eli Zaretskii
[not found] ` <CAFDFyRiHzxOB7Q6uV1hPYmuC3KfiqJRCmk=nrQ5wTPWUue_W4Q@mail.gmail.com>
2018-06-11 16:55 ` Eli Zaretskii
2018-06-22 0:34 ` Noam Postavsky
2018-06-22 6:34 ` Eli Zaretskii
2018-06-22 8:24 ` Christophe Junke
2018-06-22 9:02 ` Eli Zaretskii
2018-06-22 11:32 ` Noam Postavsky
2018-06-22 12:45 ` Eli Zaretskii
2018-06-24 1:52 ` bug#31707: [PATCH 1/1] ido: add ido-fallback special variable Noam Postavsky
2018-06-24 14:54 ` Eli Zaretskii
2018-06-26 0:40 ` Noam Postavsky
2018-06-11 18:52 ` bug#31783: [PATCH v2] ido.el: define a special ido-fallback variable Christophe Junke
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.