Uninformative comment in files.el

Uninformative comment in files.el

[Stefan Monnier]

Could whoever installed the following change:

   2007-12-10  Yoni Rabkin Katzenell  <yoni-r@actcom.com>  (tiny change)
   * file.el (revert-buffer): Eliminate overlays and the mark.

replace the comment "Reset the mark and remove all overlays" which just
paraphrases the code, with a comment that actually explains why we
want/need to do that?


        Stefan

Re: Uninformative comment in files.el

[Vinicius Jose Latorre]

Stefan Monnier wrote:
> Could whoever installed the following change:
>
>    2007-12-10  Yoni Rabkin Katzenell  <yoni-r@actcom.com>  (tiny change)
>    * file.el (revert-buffer): Eliminate overlays and the mark.
>
> replace the comment "Reset the mark and remove all overlays" which just
> paraphrases the code, with a comment that actually explains why we
> want/need to do that?
>   

This is not explained in etc/TODO file.

The patch refers to this line in etc/TODO file:

    ** revert-buffer should eliminate overlays and the mark.


Does anyone know why this is necessary?

Re: Uninformative comment in files.el

[martin rudalics]

>> Could whoever installed the following change:
>>
>>    2007-12-10  Yoni Rabkin Katzenell  <yoni-r@actcom.com>  (tiny change)
>>    * file.el (revert-buffer): Eliminate overlays and the mark.
>>
>> replace the comment "Reset the mark and remove all overlays" which just
>> paraphrases the code, with a comment that actually explains why we
>> want/need to do that?
>>   
> 
> 
> This is not explained in etc/TODO file.
> 
> The patch refers to this line in etc/TODO file:
> 
>    ** revert-buffer should eliminate overlays and the mark.
> 
> 
> Does anyone know why this is necessary?

I'm afraid the patch installed is overly simplistic.  You can find out
more by looking at the thread starting with

http://lists.gnu.org/archive/html/emacs-devel/2005-11/msg01346.html

For the moment I'd strongly propose to revert the change and study the
problem in more depth.





> 
> 
> 
> _______________________________________________
> Emacs-devel mailing list
> Emacs-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/emacs-devel
> 

Re: Uninformative comment in files.el

[Vinicius Jose Latorre]

martin rudalics wrote:
>>> Could whoever installed the following change:
>>>
>>>    2007-12-10  Yoni Rabkin Katzenell  <yoni-r@actcom.com>  (tiny 
>>> change)
>>>    * file.el (revert-buffer): Eliminate overlays and the mark.
>>>
>>> replace the comment "Reset the mark and remove all overlays" which just
>>> paraphrases the code, with a comment that actually explains why we
>>> want/need to do that?
>>>   
>>
>>
>> This is not explained in etc/TODO file.
>>
>> The patch refers to this line in etc/TODO file:
>>
>>    ** revert-buffer should eliminate overlays and the mark.
>>
>>
>> Does anyone know why this is necessary?
>
> I'm afraid the patch installed is overly simplistic.  You can find out
> more by looking at the thread starting with
>
> http://lists.gnu.org/archive/html/emacs-devel/2005-11/msg01346.html
>
> For the moment I'd strongly propose to revert the change and study the
> problem in more depth.

Ok, I've read the thread.  Indeed, the problem is not so simple.

I've just undone the patch in CVS trunk.

Re: Uninformative comment in files.el

[Yoni Rabkin Katzenell]

Vinicius Jose Latorre <viniciusjl@ig.com.br> writes:

> martin rudalics wrote:
>>>> Could whoever installed the following change:
>>>>
>>>>    2007-12-10  Yoni Rabkin Katzenell  <yoni-r@actcom.com>  (tiny
>>>> change)
>>>>    * file.el (revert-buffer): Eliminate overlays and the mark.
>>>>
>>>> replace the comment "Reset the mark and remove all overlays" which just
>>>> paraphrases the code, with a comment that actually explains why we
>>>> want/need to do that?
>>>>   
>>>
>>>
>>> This is not explained in etc/TODO file.
>>>
>>> The patch refers to this line in etc/TODO file:
>>>
>>>    ** revert-buffer should eliminate overlays and the mark.
>>>
>>>
>>> Does anyone know why this is necessary?
>>
>> I'm afraid the patch installed is overly simplistic.  You can find out
>> more by looking at the thread starting with
>>
>> http://lists.gnu.org/archive/html/emacs-devel/2005-11/msg01346.html
>>
>> For the moment I'd strongly propose to revert the change and study the
>> problem in more depth.
>
> Ok, I've read the thread.  Indeed, the problem is not so simple.
>
> I've just undone the patch in CVS trunk.

After reading that thread (which I neglected to do beforehand, sorry) I
still think that revert-buffer should remove all overlays and the mark.

My reasoning is based on the way revert-buffer is implemented, which is
to work for a simple default scenario, and to hand off any more complex
reverting to other modes via `revert-buffer-function', which replaces
revert-buffer's actions entirely if defined. For this reason, I don't
see any one fix which would solve the problem for all modes.

In any case, could you please install the docstring portion of the
patch? It is a better description of what `revert-buffer-function'
does. If needed, I can send in a patch which does only that.

-- 
   "Cut your own wood and it will warm you twice"

Re: Uninformative comment in files.el

[martin rudalics]

 > After reading that thread (which I neglected to do beforehand, sorry)

Not your fault.  The TODO item should have included a link to that
thread.

 > I still think that revert-buffer should remove all overlays and the mark.

I think your patch is correct but am not sure about a number of related
issues.  For example, is auto-reverting affected by your change and how?
Is `remove-overlays' the right function to remove all overlays in a
buffer or should we provide a simpler function that doesn't check
overlay boundaries?  Is `overlay-recenter' needed in this context?
Shall we remove overlays stored in `permanent-local' variables?  Finally
we should look into the `kill-all-local-variables' issue raised by Kim.

 > My reasoning is based on the way revert-buffer is implemented, which is
 > to work for a simple default scenario, and to hand off any more complex
 > reverting to other modes via `revert-buffer-function', which replaces
 > revert-buffer's actions entirely if defined. For this reason, I don't
 > see any one fix which would solve the problem for all modes.

In the particular case the problem was specific to a minor mode.  It
seems hardly feasible to make `revert-buffer-function' specific to minor
modes.

Re: Uninformative comment in files.el

[Yoni Rabkin Katzenell]

martin rudalics <rudalics@gmx.at> writes:

>> After reading that thread (which I neglected to do beforehand, sorry)
>
> Not your fault.  The TODO item should have included a link to that
> thread.
>
>> I still think that revert-buffer should remove all overlays and the mark.
>
> I think your patch is correct but am not sure about a number of related
> issues.  For example, is auto-reverting affected by your change and
> how?

Good point, nothing comes to mind but I'd have to check that carefully
to answer.

> Is `remove-overlays' the right function to remove all overlays in a
> buffer or should we provide a simpler function that doesn't check
> overlay boundaries?  
> Is `overlay-recenter' needed in this context?

Doing something like:

(save-excursion
  (overlay-recenter (point-max))
  (dolist (o (overlays-in (point-min) (point-max)))
    (delete-overlay o)))

... might be enough to efficiently and unconditionally nuke all the
overlays in a buffer. I have no idea if the difference between that and
the `remove-overlays' code actually matters.

As for the rest of the questions, they are so far removed from my simple
patch that I should definitely not attempt to answer unless I intend to
do some homework beforehand. They require a breadth of Emacs knowledge I
don't have at the moment.

Maybe I'll prepare a better patch after looking all that stuff up (if
nobody beats me to it).

-- 
   "Cut your own wood and it will warm you twice"

Re: Uninformative comment in files.el

[Vinicius Jose Latorre]

Yoni Rabkin Katzenell wrote:
> In any case, could you please install the docstring portion of the
> patch? It is a better description of what `revert-buffer-function'
> does. If needed, I can send in a patch which does only that.

Ok, I've just installed the docstring fix in CVS trunk.

Re: Uninformative comment in files.el

[Stefan Monnier]

> After reading that thread (which I neglected to do beforehand, sorry) I
> still think that revert-buffer should remove all overlays and the mark.

I disagree.  Anything that's not a minor mode will suffer from similar
problems and setting revert-buffer-function is not an option for them.
Your change will cause problems for the other category of minor-mode
like packages: those that need their overlays to survive a revert-buffer.

Blindly removing all overlays is drastic, and even more so if the only
way to prevent it is to set revert-buffer-function.

If a package needs to delete its hooks upon revert (e.g. cua-mode), then
it can use `before-revert-hook' and `after-revert-function'.


        Stefan


PS: For what it's worth, I believe that those two hooks should be
applied regardless of revert-buffer-function.

Re: Uninformative comment in files.el

[Yoni Rabkin Katzenell]

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

>> After reading that thread (which I neglected to do beforehand, sorry) I
>> still think that revert-buffer should remove all overlays and the mark.
>
> I disagree.  Anything that's not a minor mode will suffer from similar
> problems and setting revert-buffer-function is not an option for them.
> Your change will cause problems for the other category of minor-mode
> like packages: those that need their overlays to survive a revert-buffer.

Maybe what confuses me is that it is called `revert-buffer' and not
something like `revert-to-file-contents'. I'd expect the former to
revert the buffer to a pristine state, but the latter to only change the
contents as they relate to the associated file.

I'll recapitulate what I wrote in a separate response: I'll wait until
it is clear what you guys want to do with `revert-buffer' (if anything),
then I'll see if I can help implement that. Currently I'm both unclear
as to the purpose of the function, and obviously ignorant of the
subtleties.

-- 
   "Cut your own wood and it will warm you twice"

Re: Uninformative comment in files.el

[Richard Stallman]

    Your change will cause problems for the other category of minor-mode
    like packages: those that need their overlays to survive a revert-buffer.

I don't think that's true.  Any package that depends on overlays to
correctly survive reverting won't work anyway.  Reverting the buffer
will move the overlay bounds out, at least to cover all the changed
text in the file.  That would break them.

That is why I think they should be removed.

    Blindly removing all overlays is drastic,

It is better than letting them become garbage
and get in the way.

Re: Uninformative comment in files.el

[Stefan Monnier]

>     Your change will cause problems for the other category of minor-mode
>     like packages: those that need their overlays to survive a revert-buffer.

> I don't think that's true.  Any package that depends on overlays to
> correctly survive reverting won't work anyway.  Reverting the buffer
> will move the overlay bounds out, at least to cover all the changed
> text in the file.  That would break them.

Maybe it would.  Or maybe the package knows how to deal with that (it
wants the overlay to cover the whole buffer, for instance, and sets the
marker's advance-after property appropriately) or it just needs to keep
the overlay-buffer info to know in which buffer is the overlay.


        Stefan

Re: Uninformative comment in files.el

[martin rudalics]

 >>I don't think that's true.  Any package that depends on overlays to
 >>correctly survive reverting won't work anyway.  Reverting the buffer
 >>will move the overlay bounds out, at least to cover all the changed
 >>text in the file.  That would break them.
 >
 >
 > Maybe it would.  Or maybe the package knows how to deal with that (it
 > wants the overlay to cover the whole buffer, for instance, and sets the
 > marker's advance-after property appropriately) or it just needs to keep
 > the overlay-buffer info to know in which buffer is the overlay.

We could introduce another overlay property, `keep-when-revert-buffer'
if removing overlays shall be the default, `remove-when-revert-buffer'
otherwise.

Re: Uninformative comment in files.el

[David Kastrup]

martin rudalics <rudalics@gmx.at> writes:

>>>I don't think that's true.  Any package that depends on overlays to
>>>correctly survive reverting won't work anyway.  Reverting the buffer
>>>will move the overlay bounds out, at least to cover all the changed
>>>text in the file.  That would break them.
>>
>>
>> Maybe it would.  Or maybe the package knows how to deal with that (it
>> wants the overlay to cover the whole buffer, for instance, and sets the
>> marker's advance-after property appropriately) or it just needs to keep
>> the overlay-buffer info to know in which buffer is the overlay.
>
> We could introduce another overlay property, `keep-when-revert-buffer'
> if removing overlays shall be the default, `remove-when-revert-buffer'
> otherwise.

Well, it may need to be reseated.  So maybe it might be smarter to have
something like a property 'revert-buffer-functions (defaults to
delete-overlay but could be set to nil instead or to other values).  I
am fuzzy about when to call it and with which arguments.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Uninformative comment in files.el

[Richard Stallman]

    Well, it may need to be reseated.  So maybe it might be smarter to have
    something like a property 'revert-buffer-functions (defaults to
    delete-overlay but could be set to nil instead or to other values).

That is starting to sound possibly useful.  However, I still think
that the code to delete the overlays should be reinstalled, either
with or without this added facility.

Re: Uninformative comment in files.el

[Richard Stallman]

    Maybe it would.  Or maybe the package knows how to deal with that (it
    wants the overlay to cover the whole buffer, for instance, and sets the
    marker's advance-after property appropriately) or it just needs to keep
    the overlay-buffer info to know in which buffer is the overlay.

Those are pretty silly ways to use an overlay.  I won't say it is inconceivable
that any package does this, but I think it is not sufficient reason
to forego fixing this problem.

Re: Uninformative comment in files.el

[Stephen J. Turnbull]

Richard Stallman writes:

 >     Or maybe the package [...]  wants the overlay to cover the
 >     whole buffer, for instance,
 > 
 > [That is a] pretty silly [way] to use an overlay.  I won't say it
 > is inconceivable that any package does this, but I think it is not
 > sufficient reason to forego fixing this problem.

Well, one (and possibly more) whole-buffer overlay(s) is a consequence
of an obvious strategy to implement a top-down parser.  Ie, wrap each
non-terminal parsed with an overlay to carry annotations as you return
from recursive levels.  At least, it was obvious to me when I did it.

Re: Uninformative comment in files.el

[Richard Stallman]

    Well, one (and possibly more) whole-buffer overlay(s) is a consequence
    of an obvious strategy to implement a top-down parser.  Ie, wrap each
    non-terminal parsed with an overlay to carry annotations as you return
    from recursive levels.  At least, it was obvious to me when I did it.

Maybe you're right, but even though one of the many overlays in this
would cover the whole buffer, it would also make other overlays,
smaller than the whole buffer, which would get messed up by reverting.

Re: Uninformative comment in files.el

[Richard Stallman]

    > > http://lists.gnu.org/archive/html/emacs-devel/2005-11/msg01346.html
    > >
    > > For the moment I'd strongly propose to revert the change and study the
    > > problem in more depth.

The start of that thread shows the kind of problem that we
will fix by making revert-buffer delete overlays.

The rest of that thread shows other proposed solutions which are not
right.  There is no valid argument against making revert-buffer delete
overlays.  None of these messages actually shows any problem with that
change.  They do show some problems which that change, alone, won't
solve; but those cases have a problem now.

I don't see anything that needs to be studied.
I think you should reinstall the patch.

Re: Uninformative comment in files.el

[Stefan Monnier]

> I don't see anything that needs to be studied.
> I think you should reinstall the patch.

But then please remove the useless paraphrasing comment and add a useful
comment that explains why overlays should be removed.  And why the mark
should also be manipulated.


        Stefan

Re: Uninformative comment in files.el

[Richard Stallman]

    But then please remove the useless paraphrasing comment and add a useful
    comment that explains why overlays should be removed.  And why the mark
    should also be manipulated.

Sure.

The issue with the mark is basically the same: it often won't point at
the right place any more.  But maybe that's not as important as it is
with overlays, since there's always only one mark.  So if the mark is
not at the right place, you can ignore it.

Thus, I guess it is better not to alter anything about the mark.

Re: Uninformative comment in files.el

[Stefan Monnier]

> Thus, I guess it is better not to alter anything about the mark.

Agreed.  It should be deactivated, of course, but the
buffer-modification should take care of that.


        Stefan

Re: Uninformative comment in files.el

[Richard Stallman]

I think that any markers and overlays that were in the buffer before
you reverted will point at the wrong place after the revert.  It is
better to eliminate them.  If some Lisp program wanted them, it will
have to regenerate them anyway if they are to be right.

I did once add code to preserve markers in the unchanged text at the
beginning and end of the file.  But I think this stopped working with
Mule.