* bug#21091: 25.0.50; `isearch-done' called before `isearch-update' raises wrong-type-arg error
@ 2015-07-19 3:57 Drew Adams
2016-04-30 21:27 ` Lars Ingebrigtsen
2016-09-04 3:45 ` npostavs
0 siblings, 2 replies; 6+ messages in thread
From: Drew Adams @ 2015-07-19 3:57 UTC (permalink / raw)
To: 21091
Dunno whether I'll be able to convince you that this is a bug, but here
goes.
Recently someone added `isearch--current-buffer' to isearch.el.
This is initially nil. It is given a string value (buffer name) only in
`isearch-update'. But it is called in `isearch-done' and expected to
have a string value there. If it does not, a wrong-type-arg error is
raised.
I have code that defines some Isearch commands that each start out by
calling (isearch-done) - they can be called at top level or from
`isearch-mode-map'. Here is the beginning of one:
(defun foo (arg)
(interactive "P")
(bar 'isearch-forward arg))
(defun bar (arg)
(isearch-done)
(setq isearch-success t
isearch-adjusted t)
(let* ((enable-recursive-minibuffers t)
...)
...
(setq isearch-filter-predicate ...)
...)
(funcall search-fn))
When called at top level, `isearch--current-buffer' is nil, and the
wrong-type arg error is raised.
I can "fix" the problem that was introduced by wrapping the
`isearch-done' call in `ignore-errors'. But I think it would be better
for isearch.el not to assume that `isearch-done' is called after
`isearch-update'. I don't think there is a reason why the two need to
be coupled in that way. Adding variable `isearch--current-buffer' in
the way it was done makes the isearch.el code more fragile than it needs
to be, I think.
Anyway, please consider somehow ensuring that `isearch-done' does not
care whether `isearch--current-buffer' has a string value.
Perhaps one possibility would be something like this - dunno.
(when isearch--current-buffer
(with-current-buffer isearch--current-buffer
(setq isearch--current-buffer nil)
(setq cursor-sensor-inhibit (delq 'isearch cursor-sensor-inhibit))))
Or maybe even:
(when (and isearch--current-buffer
(get-buffer isearch--current-buffer))
...)
In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
of 2015-07-03 on LEG570
Bzr revision: 2b848fadd51e805b2f46da64c5958ea7f009048a
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
`configure --host=i686-pc-mingw32 --enable-checking=yes,glyphs'
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#21091: 25.0.50; `isearch-done' called before `isearch-update' raises wrong-type-arg error
2015-07-19 3:57 bug#21091: 25.0.50; `isearch-done' called before `isearch-update' raises wrong-type-arg error Drew Adams
@ 2016-04-30 21:27 ` Lars Ingebrigtsen
2016-09-04 0:08 ` Drew Adams
2016-09-04 3:45 ` npostavs
1 sibling, 1 reply; 6+ messages in thread
From: Lars Ingebrigtsen @ 2016-04-30 21:27 UTC (permalink / raw)
To: Drew Adams; +Cc: 21091
Drew Adams <drew.adams@oracle.com> writes:
> Dunno whether I'll be able to convince you that this is a bug, but here
> goes.
>
> Recently someone added `isearch--current-buffer' to isearch.el.
>
> This is initially nil. It is given a string value (buffer name) only in
> `isearch-update'. But it is called in `isearch-done' and expected to
> have a string value there. If it does not, a wrong-type-arg error is
> raised.
If I understand you correctly, I don't think this is a bug. If somebody
else disagrees, please reopen this bug report.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#21091: 25.0.50; `isearch-done' called before `isearch-update' raises wrong-type-arg error
2016-04-30 21:27 ` Lars Ingebrigtsen
@ 2016-09-04 0:08 ` Drew Adams
0 siblings, 0 replies; 6+ messages in thread
From: Drew Adams @ 2016-09-04 0:08 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 21091
> > Recently someone added `isearch--current-buffer' to isearch.el.
> >
> > This is initially nil. It is given a string value (buffer name) only in
> > `isearch-update'. But it is called in `isearch-done' and expected to
> > have a string value there. If it does not, a wrong-type-arg error is
> > raised...
> >
> > When called at top level, `isearch--current-buffer' is nil, and the
> > wrong-type arg error is raised.
> >
> > I can "fix" the problem that was introduced by wrapping the
> > `isearch-done' call in `ignore-errors'. But I think it would be better
> > for isearch.el not to assume that `isearch-done' is called after
> > `isearch-update'. I don't think there is a reason why the two need to
> > be coupled in that way. Adding variable `isearch--current-buffer' in
> > the way it was done makes the isearch.el code more fragile than it needs
> > to be, I think.
> >
> > Anyway, please consider somehow ensuring that `isearch-done' does not
> > care whether `isearch--current-buffer' has a string value.
>
> If I understand you correctly, I don't think this is a bug. If somebody
> else disagrees, please reopen this bug report.
No reason given? Why do you think it is not a bug? Why do you not
think that "adding variable `isearch--current-buffer' in the way it
was done makes the isearch.el code more fragile than it needs to be"?
Why should the code assume that `isearch-done' is called only after
`isearch-update'? There is nothing inherent in `isearch-done' that
suggests that.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#21091: 25.0.50; `isearch-done' called before `isearch-update' raises wrong-type-arg error
2015-07-19 3:57 bug#21091: 25.0.50; `isearch-done' called before `isearch-update' raises wrong-type-arg error Drew Adams
2016-04-30 21:27 ` Lars Ingebrigtsen
@ 2016-09-04 3:45 ` npostavs
2016-09-04 5:43 ` Drew Adams
1 sibling, 1 reply; 6+ messages in thread
From: npostavs @ 2016-09-04 3:45 UTC (permalink / raw)
To: Drew Adams; +Cc: 21091
[-- Attachment #1: Type: text/plain, Size: 327 bytes --]
tags 21091 patch
quit
Drew Adams <drew.adams@oracle.com> writes:
>
> Anyway, please consider somehow ensuring that `isearch-done' does not
> care whether `isearch--current-buffer' has a string value.
Make sense to me, I suggest the following (isearch-update checks for a
buffer value, so I went with that to be consistent):
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1356 bytes --]
From 5d7697546800ad3494df1d06d24e12f2fe987350 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 3 Sep 2016 23:38:35 -0400
Subject: [PATCH v1] Don't require isearch-update before isearch-done
It is useful to be able to call `isearch-done' unconditionally to
ensure a non-isearching state.
* lisp/isearch.el (isearch-done): Check that `isearch--current-buffer'
is a live buffer before using it (Bug #21091).
---
lisp/isearch.el | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/lisp/isearch.el b/lisp/isearch.el
index b50379a..39ed8af 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1045,9 +1045,10 @@ isearch-done
(remove-hook 'mouse-leave-buffer-hook 'isearch-done)
(remove-hook 'kbd-macro-termination-hook 'isearch-done)
(setq isearch-lazy-highlight-start nil)
- (with-current-buffer isearch--current-buffer
- (setq isearch--current-buffer nil)
- (setq cursor-sensor-inhibit (delq 'isearch cursor-sensor-inhibit)))
+ (when (buffer-live-p isearch--current-buffer)
+ (with-current-buffer isearch--current-buffer
+ (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.
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#21091: 25.0.50; `isearch-done' called before `isearch-update' raises wrong-type-arg error
2016-09-04 3:45 ` npostavs
@ 2016-09-04 5:43 ` Drew Adams
2016-09-04 20:47 ` npostavs
0 siblings, 1 reply; 6+ messages in thread
From: Drew Adams @ 2016-09-04 5:43 UTC (permalink / raw)
To: npostavs; +Cc: 21091
> > Anyway, please consider somehow ensuring that `isearch-done' does not
> > care whether `isearch--current-buffer' has a string value.
>
> Make sense to me, I suggest the following (isearch-update checks for a
> buffer value, so I went with that to be consistent):
Haven't tested, but it looks good to me. I think that's all that's
needed. Thanks for doing this.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#21091: 25.0.50; `isearch-done' called before `isearch-update' raises wrong-type-arg error
2016-09-04 5:43 ` Drew Adams
@ 2016-09-04 20:47 ` npostavs
0 siblings, 0 replies; 6+ messages in thread
From: npostavs @ 2016-09-04 20:47 UTC (permalink / raw)
To: Drew Adams; +Cc: 21091
[-- Attachment #1: Type: text/plain, Size: 647 bytes --]
tags 21091 - notabug
found 21091 25.1
quit
Drew Adams <drew.adams@oracle.com> writes:
>> > Anyway, please consider somehow ensuring that `isearch-done' does not
>> > care whether `isearch--current-buffer' has a string value.
>>
>> Make sense to me, I suggest the following (isearch-update checks for a
>> buffer value, so I went with that to be consistent):
>
> Haven't tested, but it looks good to me. I think that's all that's
> needed. Thanks for doing this.
Speaking of testing, I think this sort thing would benefit from a
regression test, so I added that to the patch. I will to master push in
a few days if there are no objections.
[-- Attachment #2: patch v2 --]
[-- Type: text/plain, Size: 2028 bytes --]
From 2f00da3a255a2fb46ce4819a3153e04d9d9d59c9 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 3 Sep 2016 23:38:35 -0400
Subject: [PATCH v2] Don't require isearch-update before isearch-done
It is useful to be able to call `isearch-done' unconditionally to
ensure a non-isearching state.
* lisp/isearch.el (isearch-done): Check that `isearch--current-buffer'
is a live buffer before using it (Bug #21091).
* test/lisp/isearch-tests.el (isearch--test-done): Test it.
---
lisp/isearch.el | 7 ++++---
test/lisp/isearch-tests.el | 8 ++++++++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/lisp/isearch.el b/lisp/isearch.el
index b50379a..39ed8af 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1045,9 +1045,10 @@ isearch-done
(remove-hook 'mouse-leave-buffer-hook 'isearch-done)
(remove-hook 'kbd-macro-termination-hook 'isearch-done)
(setq isearch-lazy-highlight-start nil)
- (with-current-buffer isearch--current-buffer
- (setq isearch--current-buffer nil)
- (setq cursor-sensor-inhibit (delq 'isearch cursor-sensor-inhibit)))
+ (when (buffer-live-p isearch--current-buffer)
+ (with-current-buffer isearch--current-buffer
+ (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.
diff --git a/test/lisp/isearch-tests.el b/test/lisp/isearch-tests.el
index 48c3424..52f312d 100644
--- a/test/lisp/isearch-tests.el
+++ b/test/lisp/isearch-tests.el
@@ -28,5 +28,13 @@
(isearch-update)
(should (equal isearch--current-buffer (current-buffer)))))
+(ert-deftest isearch--test-done ()
+ ;; Normal operation.
+ (isearch-update)
+ (isearch-done)
+ (should-not isearch--current-buffer)
+ ;; Bug #21091: let `isearch-done' work without `isearch-update'.
+ (isearch-done))
+
(provide 'isearch-tests)
;;; isearch-tests.el ends here
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-04 20:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-19 3:57 bug#21091: 25.0.50; `isearch-done' called before `isearch-update' raises wrong-type-arg error Drew Adams
2016-04-30 21:27 ` Lars Ingebrigtsen
2016-09-04 0:08 ` Drew Adams
2016-09-04 3:45 ` npostavs
2016-09-04 5:43 ` Drew Adams
2016-09-04 20:47 ` npostavs
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).