bug#3973: cleanup view-mode usage in vc-resynch-window

bug#3973: cleanup view-mode usage in vc-resynch-window

[Dan Nicolaescu]

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?

bug#3973: cleanup view-mode usage in vc-resynch-window

[Stefan Monnier]

> 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

bug#3973: cleanup view-mode usage in vc-resynch-window

[Dan Nicolaescu]

Stefan Monnier <monnier@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?

bug#3973: cleanup view-mode usage in vc-resynch-window

[Dan Nicolaescu]

Stefan Monnier <monnier@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.

bug#3973: cleanup view-mode usage in vc-resynch-window

[Stefan Monnier]

>> > 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

bug#3973: cleanup view-mode usage in vc-resynch-window

[Dan Nicolaescu]

Stefan Monnier <monnier@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.

bug#3973: cleanup view-mode usage in vc-resynch-window

[Stefan Monnier]

> 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

bug#3973: cleanup view-mode usage in vc-resynch-window

[Lars Magne Ingebrigtsen]

Stefan Monnier <monnier@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/