GNU bug report logs - #3973
cleanup view-mode usage in vc-resynch-window

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 3973 in the body.
You can then email your comments to 3973 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #3 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Dan Nicolaescu <dann <at> ics.uci.edu>
To: bug-gnu-emacs <bug-gnu-emacs <at> gnu.org>
Subject: cleanup view-mode usage in vc-resynch-window
Date: Thu, 30 Jul 2009 00:44:58 -0700 (PDT)

vc-resynch-window contains this code:

             ;; TODO: Adjusting view mode might no longer be necessary
             ;; after RMS change to files.el of 1999-08-08.  Investigate
             ;; this when we install the new VC.
             (and view-read-only
                  (if (file-writable-p file)
                      (and view-mode
                           (let ((view-old-buffer-read-only nil))
                             (view-mode-exit)))
                    (and (not view-mode)
                         (not (eq (get major-mode 'mode-class) 'special))
                         (view-mode-enter))))

not sure what it's supposed to do... 
But the comment (added in Apr 2000!) seems to say that it can be removed.
Should it?

Message #8 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Dan Nicolaescu <dann <at> ics.uci.edu>
Cc: 3973 <at> debbugs.gnu.org, bug-gnu-emacs <bug-gnu-emacs <at> gnu.org>
Subject: Re: bug#3973: cleanup view-mode usage in vc-resynch-window
Date: Thu, 30 Jul 2009 17:14:20 -0400

> not sure what it's supposed to do... 
> But the comment (added in Apr 2000!) seems to say that it can be removed.
> Should it?

I don't know.  Someone needs to try it,


        Stefan

Message #16 received at 3973 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Dan Nicolaescu <dann <at> ics.uci.edu>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 3973 <at> debbugs.gnu.org
Subject: Re: bug#3973: cleanup view-mode usage in vc-resynch-window
Date: Thu, 30 Jul 2009 15:06:51 -0700 (PDT)

Stefan Monnier <monnier <at> IRO.UMontreal.CA> writes:

  > > not sure what it's supposed to do... 
  > > But the comment (added in Apr 2000!) seems to say that it can be removed.
  > > Should it?
  > 
  > I don't know.  Someone needs to try it,

I have removed that code locally for a while and didn't see any
problem. But I don't explicitly use view-mode, so...
How about remove the code and see if someone sees a problem?

Message #21 received at 3973 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dan Nicolaescu <dann <at> ics.uci.edu>
Cc: 3973 <at> debbugs.gnu.org
Subject: Re: bug#3973: cleanup view-mode usage in vc-resynch-window
Date: Thu, 30 Jul 2009 21:54:38 -0400

> I have removed that code locally for a while and didn't see any
> problem.  But I don't explicitly use view-mode, so...

Obiously the code only makes a difference when you use view-read-only,
so you'll need to set it to a non-nil value before doing any test.

> How about remove the code and see if someone sees a problem?

Not before trying out the corresponding feature, I think.


        Stefan

Message #24 received at 3973 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Dan Nicolaescu <dann <at> ics.uci.edu>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 3973 <at> debbugs.gnu.org
Subject: Re: bug#3973: cleanup view-mode usage in vc-resynch-window
Date: Thu, 30 Jul 2009 20:22:50 -0700 (PDT)

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

  > > I have removed that code locally for a while and didn't see any
  > > problem.  But I don't explicitly use view-mode, so...
  > 
  > Obiously the code only makes a difference when you use view-read-only,
  > so you'll need to set it to a non-nil value before doing any test.

[I've never done anything with view-read-only before...]
I set view-read-only to t, then played with a file under RCS.
When the files is checked in view-mode is turned on, when checking it
out it's turned off.

If the code in question is eliminated, view-mode stays turned on after
checking out.

There's some code in `after-find-file' (that gets called from
`revert-buffer') that looks like it does something very similar to the
code in question here, but `after-find-file' is called with `nomodes' t,
so that code does not trigger.

Message #29 received at 3973 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dan Nicolaescu <dann <at> ics.uci.edu>
Cc: 3973 <at> debbugs.gnu.org
Subject: Re: bug#3973: cleanup view-mode usage in vc-resynch-window
Date: Thu, 30 Jul 2009 23:43:39 -0400

>> > I have removed that code locally for a while and didn't see any
>> > problem.  But I don't explicitly use view-mode, so...
>> 
>> Obiously the code only makes a difference when you use view-read-only,
>> so you'll need to set it to a non-nil value before doing any test.

> [I've never done anything with view-read-only before...]
> I set view-read-only to t, then played with a file under RCS.
> When the files is checked in view-mode is turned on, when checking it
> out it's turned off.

> If the code in question is eliminated, view-mode stays turned on after
> checking out.

> There's some code in `after-find-file' (that gets called from
> `revert-buffer') that looks like it does something very similar to the
> code in question here, but `after-find-file' is called with `nomodes' t,
> so that code does not trigger.

So it seems the code is still needed.  Maybe we could/should replace it
with something more clear: E.g. move that code to a new function
`view-refresh' (or whatever else), which we can then call from VC.

Maybe a better option in the longer term is refine the meaning of the
`preserve-modes' arg to revert-buffer, so that some minor modes can
request to be refreshed or not (the VC state is one that would also
benefit from such a change, as can be seen in auto-revert-mode).


        Stefan

Message #32 received at 3973 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Dan Nicolaescu <dann <at> ics.uci.edu>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 3973 <at> debbugs.gnu.org
Subject: Re: bug#3973: cleanup view-mode usage in vc-resynch-window
Date: Thu, 30 Jul 2009 23:28:10 -0700 (PDT)

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

  > >> > I have removed that code locally for a while and didn't see any
  > >> > problem.  But I don't explicitly use view-mode, so...
  > >> 
  > >> Obiously the code only makes a difference when you use view-read-only,
  > >> so you'll need to set it to a non-nil value before doing any test.
  > 
  > > [I've never done anything with view-read-only before...]
  > > I set view-read-only to t, then played with a file under RCS.
  > > When the files is checked in view-mode is turned on, when checking it
  > > out it's turned off.
  > 
  > > If the code in question is eliminated, view-mode stays turned on after
  > > checking out.
  > 
  > > There's some code in `after-find-file' (that gets called from
  > > `revert-buffer') that looks like it does something very similar to the
  > > code in question here, but `after-find-file' is called with `nomodes' t,
  > > so that code does not trigger.
  > 
  > So it seems the code is still needed.  Maybe we could/should replace it
  > with something more clear: E.g. move that code to a new function
  > `view-refresh' (or whatever else), which we can then call from VC.

The function would look like this:

(defun view-maybe-enter-exit (file)
  (if (file-writable-p file)
      (and view-mode
         (let ((view-old-buffer-read-only nil))
              (view-mode-exit)))
    (and (not view-mode)
     (not (eq (get major-mode 'mode-class) 'special))
      (view-mode-enter))))

but the calling convention is strange: it needs to be called with a
buffer selected and pass the file name...  Not sure it's worth it.
At least the TODO comment can go and be replaced with a comment about
why changing the state of view-mode is needed there.

  > Maybe a better option in the longer term is refine the meaning of the
  > `preserve-modes' arg to revert-buffer, so that some minor modes can
  > request to be refreshed or not (the VC state is one that would also
  > benefit from such a change, as can be seen in auto-revert-mode).

That sounds like a good idea.  Unfortunately I am not familiar with that
code, it is quite complex and critical, and I don't think it's too
important to fix it now, so I'll punt.

Message #37 received at 3973 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dan Nicolaescu <dann <at> ics.uci.edu>
Cc: 3973 <at> debbugs.gnu.org
Subject: Re: bug#3973: cleanup view-mode usage in vc-resynch-window
Date: Fri, 31 Jul 2009 15:19:18 -0400

> The function would look like this:

> (defun view-maybe-enter-exit (file)
>   (if (file-writable-p file)
>       (and view-mode
>          (let ((view-old-buffer-read-only nil))
>               (view-mode-exit)))
>     (and (not view-mode)
>      (not (eq (get major-mode 'mode-class) 'special))
>       (view-mode-enter))))

> but the calling convention is strange: it needs to be called with a
> buffer selected and pass the file name...  Not sure it's worth it.

Why do we need to pass `file'?
Can't we (assert (equal file buffer-file-name))?

> At least the TODO comment can go and be replaced with a comment about
> why changing the state of view-mode is needed there.

Yes, please.

>> Maybe a better option in the longer term is refine the meaning of the
>> `preserve-modes' arg to revert-buffer, so that some minor modes can
>> request to be refreshed or not (the VC state is one that would also
>> benefit from such a change, as can be seen in auto-revert-mode).

> That sounds like a good idea.  Unfortunately I am not familiar with that
> code, it is quite complex and critical, and I don't think it's too
> important to fix it now, so I'll punt.

Wise choice,


        Stefan

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

From: Lars Magne Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 3973 <at> debbugs.gnu.org, Dan Nicolaescu <dann <at> ics.uci.edu>
Subject: Re: bug#3973: cleanup view-mode usage in vc-resynch-window
Date: Tue, 12 Jul 2011 22:17:39 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> At least the TODO comment can go and be replaced with a comment about
>> why changing the state of view-mode is needed there.
>
> Yes, please.

Apparently this was done, so I'm closing this report.

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

Previous Next

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