bug#72229: (setq overriding-terminal-local-map nil) in isearch-done

bug#72229: (setq overriding-terminal-local-map nil) in isearch-done

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

Hello,

exiting isearch always explicitly sets `overriding-terminal-local-map'
to nil.  This will interfere with any other users of
`overriding-terminal-local-map', including any active transient maps.

Wouldn't a less radical means suffice?  If isearch really must _set_,
i.e., completely override that variable, why not restore the original
value?


TIA,

Michael.

bug#72229: (setq overriding-terminal-local-map nil) in isearch-done

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

Michael Heerdegen via "Bug reports for GNU Emacs, the Swiss army knife
of text editors" <bug-gnu-emacs@gnu.org> writes:

> exiting isearch always explicitly sets `overriding-terminal-local-map'
> to nil.  This will interfere with any other users of
> `overriding-terminal-local-map', including any active transient maps.
>
> Wouldn't a less radical means suffice?  If isearch really must _set_,
> i.e., completely override that variable, why not restore the original
> value?

Juri, what's your opinion on this?


A little more forward looking:

In such situations I often get this thought: if the variable was
replaced with a function accepting zero arguments, then we could use
`add-function' and `remove-function' to control the return value of the
"binding".

I know the advice mechanism has the reputation of only being suitable
for end user customization/hacks, but in cases like this one we could
make the modification of the value more explicit and controllable.
Dealing with interferences would be forced to be taken into account more
directly , and we would get some useful mechanisms like priorities or
looking at the context out of the box.

We could also invent some even better mechanism, maybe.  But in this
case binding variables doesn't look like an optimal approach at least.


Michael.

bug#72229: (setq overriding-terminal-local-map nil) in isearch-done

[Juri Linkov]

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

> Michael Heerdegen via "Bug reports for GNU Emacs, the Swiss army knife
> of text editors" <bug-gnu-emacs@gnu.org> writes:
>
>> exiting isearch always explicitly sets `overriding-terminal-local-map'
>> to nil.  This will interfere with any other users of
>> `overriding-terminal-local-map', including any active transient maps.
>>
>> Wouldn't a less radical means suffice?  If isearch really must _set_,
>> i.e., completely override that variable, why not restore the original
>> value?
>
> Juri, what's your opinion on this?

Indeed, you are right, `isearch-done' should restore the original value.
The existing variable `isearch--saved-overriding-local-map' can't be used,
so a similar variable should be added like in this patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: isearch--saved-local-map.patch --]
[-- Type: text/x-diff, Size: 2279 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index dc9edf267f2..697dcdbb3d8 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -972,6 +972,7 @@ isearch-hidden
 ;; The value of input-method-function when isearch is invoked.
 (defvar isearch-input-method-function nil)
 
+(defvar isearch--saved-local-map nil)
 (defvar isearch--saved-overriding-local-map nil)
 
 ;; Minor-mode-alist changes - kind of redundant with the
@@ -1321,6 +1322,7 @@ isearch-mode
   (setq	isearch-mode " Isearch")  ;; forward? regexp?
   (force-mode-line-update)
 
+  (setq isearch--saved-local-map overriding-terminal-local-map)
   (setq overriding-terminal-local-map isearch-mode-map)
   (run-hooks 'isearch-mode-hook)
   ;; Remember the initial map possibly modified
@@ -1439,10 +1444,12 @@ isearch-update
 
 (defun isearch-done (&optional nopush edit)
   "Exit Isearch mode.
+Called by all commands that terminate isearch-mode.
 For successful search, pass no args.
 For a failing search, NOPUSH is t.
 For going to the minibuffer to edit the search string,
-NOPUSH is t and EDIT is t."
+NOPUSH is t and EDIT is t.
+If NOPUSH is non-nil, we don't push the string on the search ring."
 
   (when isearch-resume-in-command-history
     (add-to-history 'command-history
@@ -1460,9 +1467,7 @@ isearch-done
       (setq isearch--current-buffer nil)
       (setq cursor-sensor-inhibit (delq 'isearch cursor-sensor-inhibit))))
 
-  ;; Called by all commands that terminate isearch-mode.
-  ;; If NOPUSH is non-nil, we don't push the string on the search ring.
-  (setq overriding-terminal-local-map nil)
+  (setq overriding-terminal-local-map isearch--saved-local-map)
   ;; (setq pre-command-hook isearch-old-pre-command-hook) ; for lemacs
   (setq minibuffer-message-timeout isearch-original-minibuffer-message-timeout)
   (isearch-dehighlight)
@@ -2676,7 +2681,7 @@ isearch-mouse-2
 is bound to outside of Isearch."
   (interactive "e")
   (let ((w (posn-window (event-start click)))
-        (binding (let ((overriding-terminal-local-map nil)
+        (binding (let ((overriding-terminal-local-map isearch--saved-local-map)
                        ;; Key search depends on mode (bug#47755)
                        (isearch-mode nil))
                    (key-binding (this-command-keys-vector) t))))

[-- Attachment #3: Type: text/plain, Size: 970 bytes --]


> A little more forward looking:
>
> In such situations I often get this thought: if the variable was
> replaced with a function accepting zero arguments, then we could use
> `add-function' and `remove-function' to control the return value of the
> "binding".
>
> I know the advice mechanism has the reputation of only being suitable
> for end user customization/hacks, but in cases like this one we could
> make the modification of the value more explicit and controllable.
> Dealing with interferences would be forced to be taken into account more
> directly , and we would get some useful mechanisms like priorities or
> looking at the context out of the box.
>
> We could also invent some even better mechanism, maybe.  But in this
> case binding variables doesn't look like an optimal approach at least.

This mechanism looks like a variable watcher enabled by `add-variable-watcher'.
So you could add a watcher that conditionally controls variable modifications.

bug#72229: (setq overriding-terminal-local-map nil) in isearch-done

[Eli Zaretskii]

> Cc: 72229@debbugs.gnu.org
> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 23 Jul 2024 09:32:21 +0300
> 
> > Michael Heerdegen via "Bug reports for GNU Emacs, the Swiss army knife
> > of text editors" <bug-gnu-emacs@gnu.org> writes:
> >
> >> exiting isearch always explicitly sets `overriding-terminal-local-map'
> >> to nil.  This will interfere with any other users of
> >> `overriding-terminal-local-map', including any active transient maps.
> >>
> >> Wouldn't a less radical means suffice?  If isearch really must _set_,
> >> i.e., completely override that variable, why not restore the original
> >> value?
> >
> > Juri, what's your opinion on this?
> 
> Indeed, you are right, `isearch-done' should restore the original value.
> The existing variable `isearch--saved-overriding-local-map' can't be used,
> so a similar variable should be added like in this patch:

Thanks.  If this is deemed the right solution, please install on
master, not on emacs-30 (as we have lived with this issue since 1995).

bug#72229: (setq overriding-terminal-local-map nil) in isearch-done

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

Juri Linkov <juri@linkov.net> writes:

> Indeed, you are right, `isearch-done' should restore the original value.
> The existing variable `isearch--saved-overriding-local-map' can't be used,
> so a similar variable should be added like in this patch:

LGTM for master - thank you.


> This mechanism looks like a variable watcher enabled by
> `add-variable-watcher'.
> So you could add a watcher that conditionally controls variable
> modifications.

I don't think variable watchers are very helpful here.  They don't solve
the underlying problem: potentially infinite variables of the same name
can exist, shadowing each other, with values partly sharing structures.


Using variable watchers I can see whether a variable value gets shadowed
or unassigned using a set operation - but I can't know whether the
previous value still exists, as binding of some other variable, and if
it will be stored back into the variable.  Nor do I have access to old
bindings and their values until the program assigns it back to the
variable.

I saw that in Bug#70938.  Manipulations of a variable can interfere in
annoying ways.

Functions are different.  You have only one dynamic binding (unless in
the rare case of using `cl-letf', which is extremely rare).  And you
always have access to it to undo any prior modification.


Michael.

bug#72229: (setq overriding-terminal-local-map nil) in isearch-done

[Juri Linkov]

>> This mechanism looks like a variable watcher enabled by
>> `add-variable-watcher'.
>> So you could add a watcher that conditionally controls variable
>> modifications.
>
> I don't think variable watchers are very helpful here.  They don't solve
> the underlying problem: potentially infinite variables of the same name
> can exist, shadowing each other, with values partly sharing structures.
>
> Using variable watchers I can see whether a variable value gets shadowed
> or unassigned using a set operation - but I can't know whether the
> previous value still exists, as binding of some other variable, and if
> it will be stored back into the variable.  Nor do I have access to old
> bindings and their values until the program assigns it back to the
> variable.
>
> I saw that in Bug#70938.  Manipulations of a variable can interfere in
> annoying ways.
>
> Functions are different.  You have only one dynamic binding (unless in
> the rare case of using `cl-letf', which is extremely rare).  And you
> always have access to it to undo any prior modification.

I remember Stefan M suggested to use function variables as much as possible
such as (defvar isearch-filter-predicate #'isearch-filter-visible), then
it's easy to add more filters with add-function.

But I'm not sure is it possible to do the same with a variable
getter function?  I mean that instead of (funcall isearch-filter-predicate)
such a getter function could be called (get-value isearch-filter-predicate)
to access the variable value.

bug#72229: (setq overriding-terminal-local-map nil) in isearch-done

[Juri Linkov]

close 72229 31.0.50
thanks

>> Indeed, you are right, `isearch-done' should restore the original value.
>> The existing variable `isearch--saved-overriding-local-map' can't be used,
>> so a similar variable should be added like in this patch:
>
> Thanks.  If this is deemed the right solution, please install on
> master, not on emacs-30 (as we have lived with this issue since 1995).

Now installed on master.  I need to dust off an old patch
that obsoletes overriding-terminal-local-map in isearch
by enabling buffer-local isearch-mode.  I guess it will go
to master as well.

bug#72229: (setq overriding-terminal-local-map nil) in isearch-done

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

Juri Linkov <juri@linkov.net> writes:

> But I'm not sure is it possible to do the same with a variable
> getter function?

I think it would be unavoidable to change all code places where the
variable is referenced, including those in C, to call the function value
stored in the variable instead of referencing the variable binding.


Michael.

bug#72229: (setq overriding-terminal-local-map nil) in isearch-done

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

> > But I'm not sure is it possible to do the same with a variable
> > getter function?
> 
> I think it would be unavoidable to change all code places where the
> variable is referenced, including those in C, to call the function value
> stored in the variable instead of referencing the variable binding.

I'm not knowledgeable in this area, and I'm not
sure what you're discussing (I think it's the
possibility of substituting a function for a var),
but would defining a symbol macro help?

We at least have `cl-symbol-macrolet', even if we
don't (yet) have `define-symbol-macro'.

https://lisp-docs.github.io/cl-language-reference/chap-3/d-i-dictionary/define-symbol-macro_macro

bug#72229: (setq overriding-terminal-local-map nil) in isearch-done

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

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

> I'm not knowledgeable in this area, and I'm not
> sure what you're discussing (I think it's the
> possibility of substituting a function for a var),
> but would defining a symbol macro help?
>
> We at least have `cl-symbol-macrolet', even if we
> don't (yet) have `define-symbol-macro'.
>
> https://lisp-docs.github.io/cl-language-reference/chap-3/d-i-dictionary/define-symbol-macro_macro

We would need global symbol macros.  But even these would not be able to
affect variable references from C I think.

Symbol macros are also a bit fragile in Elisp, and some completely
dislike them (Richard for example).

So, yes, this is suggesting itself but would make the problem to solve
even harder, unfortunately.



Michael.