bug#20971: 24.4; occur-1 makes my buffer read-only

bug#20971: 24.4; occur-1 makes my buffer read-only

[Alex Schröder]

[-- Attachment #1.1: Type: text/plain, Size: 488 bytes --]

I'm using isearch to search for something, then I use M-s o to run an occur
for my search term. This results in *Occur* being shown and in the buffer
I'm searching to be read-only. This is confusing. I think what people
intended was to make *Occur* read-only. But occur-mode already inherits
from special-mode which already does (setq buffer-read-only t).

I therefore propose to simply remove the (setq buffer-read-only t) line
from the end of occur-1 in replace.el. See attached patch.

[-- Attachment #1.2: Type: text/html, Size: 543 bytes --]

[-- Attachment #2: fix_occur.patch --]
[-- Type: application/octet-stream, Size: 978 bytes --]

From e945aa2ebc082cfadb219c136918e5ec9270217d Mon Sep 17 00:00:00 2001
From: Alex Schroeder <alex@gnu.org>
Date: Fri, 3 Jul 2015 11:16:53 +0200
Subject: [PATCH] (occur-1): Don't set buffer-read-only.

This resulted in the buffer being searched being set read-only. This
is confusing. What people intended was to make *Occur* read-only. But
occur-mode already inherits from special-mode which already does (setq
buffer-read-only t). Therefore buffer-read-only is no longer being
set.
---
 lisp/replace.el | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lisp/replace.el b/lisp/replace.el
index 41debdd..798528e 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1501,7 +1501,6 @@ See also `multi-occur'."
               (kill-buffer occur-buf)
             (display-buffer occur-buf)
             (setq next-error-last-buffer occur-buf)
-            (setq buffer-read-only t)
             (set-buffer-modified-p nil)
             (run-hooks 'occur-hook)))))))
 
-- 
2.4.3

bug#20971: 24.4; occur-1 makes my buffer read-only

[Eli Zaretskii]

> From: Alex Schröder <kensanata@gmail.com>
> Date: Fri, 03 Jul 2015 09:21:55 +0000
> 
> I'm using isearch to search for something, then I use M-s o to run an occur for
> my search term. This results in *Occur* being shown and in the buffer I'm
> searching to be read-only.

I cannot reproduce this.  The buffer I was searching doesn't become
read-only.  Are you sure it's not one of your customizations?  Can you
see that in "emacs -Q"?

bug#20971: 24.4; occur-1 makes my buffer read-only

[Alex Schröder]

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

Indeed, the problem goes away with emacs -Q. I investigated further and
found that the culprit is display-buffer-function being set to
popwin:display-buffer. Thus, I can load my current config and evaluate
(setq display-buffer-function nil). The problem will disappear.

I still think that my patch is the right solution. The current code expects
display-buffer to behave in a certain way and Emacs gives us a ton of ways
to change that. Thus, we either need to explicitly set the current buffer,
or we can do away with trying to make *Occur* read-only. And really, who
needs that? Even if we don't set buffer-read-only, the buffer has no
self-insert-command bindings, and if you try to C-k the header, it will
still say "Text is read-only: #<buffer *Occur*>" because there is a
read-only text-property. And why shouldn't you delete matches?

Thus, as far as I can tell, my change makes the code more robust (no longer
depending on display-buffer to work in a particular way) and it has
practically no drawbacks.

[-- Attachment #2: Type: text/html, Size: 1106 bytes --]

bug#20971: 24.4; occur-1 makes my buffer read-only

[Stefan Kangas]

Alex Schröder <kensanata@gmail.com> writes:

> Indeed, the problem goes away with emacs -Q. I investigated further and found that the culprit is display-buffer-function
> being set to popwin:display-buffer. Thus, I can load my current config and evaluate (setq display-buffer-function nil). The
> problem will disappear.
>
> I still think that my patch is the right solution. The current code expects display-buffer to behave in a certain way and
> Emacs gives us a ton of ways to change that. Thus, we either need to explicitly set the current buffer, or we can do away
> with trying to make *Occur* read-only. And really, who needs that? Even if we don't set buffer-read-only, the buffer has no
> self-insert-command bindings, and if you try to C-k the header, it will still say "Text is read-only: #<buffer *Occur*>"
> because there is a read-only text-property. And why shouldn't you delete matches?
>
> Thus, as far as I can tell, my change makes the code more robust (no longer depending on display-buffer to work in a
> particular way) and it has practically no drawbacks.

I think *Occur* should definitely be read-only, so your first patch is
the wrong solution in my opinion.

On the other hand, I see no drawbacks with wrapping the relevant calls
in:

  (with-current-buffer occur-buf
       ...)

At the very least, that would make the intention of that code a bit more
clear.

But that entire part of the code is already wrapped in
(with-current-buffer occur-buf ...).  So why are we not in occur-buf
already?

Best regards,
Stefan Kangas

bug#20971: 24.4; occur-1 makes my buffer read-only

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> I think *Occur* should definitely be read-only, so your first patch is
> the wrong solution in my opinion.
>
> On the other hand, I see no drawbacks with wrapping the relevant calls
> in:
>
>   (with-current-buffer occur-buf
>        ...)
>
> At the very least, that would make the intention of that code a bit more
> clear.
>
> But that entire part of the code is already wrapped in
> (with-current-buffer occur-buf ...).  So why are we not in occur-buf
> already?

I am guessing the reporter had some misbehaving code running off of
`display-buffer' that changed the current buffer:

            (display-buffer occur-buf)
            (when occur--final-pos
              (set-window-point
               (get-buffer-window occur-buf 'all-frames)
               occur--final-pos))
            (setq next-error-last-buffer occur-buf)
            (setq buffer-read-only t)
            (set-buffer-modified-p nil)
            (run-hooks 'occur-hook)))))))

So everything after displaying the buffer runs somewhere else.  We could
defensively re-wrap the rest in a with-current-buffer, but...  I think
the bug is in the code that changed the current buffer, really.

So I'm closing this bug report.

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