Should `revert-buffer' preserve text-scaling by default?

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Should `revert-buffer' preserve text-scaling by default?

Karl Fogel-2
Currently, when you do `revert-buffer' it will discard any text scale changes done in that buffer (i.e., done via "C-x C-+" / `text-scale-adjust').  

The effect -- unexpected to me at least -- is that when I revert a buffer whose text size I have increased, the text suddenly shrinks down again (to whatever the default text scale was).

Before I "fix" this, I'd like to know if others consider it a bug or a feature.

The reason the text scale currently gets reverted is that text-scaling happens to be implemented via the minor mode `text-scale-mode'.  Since `revert-buffer' doesn't preserve modes by default, text-scaling is lost when one reverts.  There is an optional `preserve-modes' argument to `revert-buffer', but I don't think that just making wrapper that sets that argument to non-nil is a good solution here, because that would preserve *all* the modes.  From the user's point of view, the fact that text-scaling happens to be implemented via a minor mode is irrelevant and is probably not something most users are even aware of.  Text-scaling is more like a magnifying glass (or reducing glass) that one places over the text of a particular buffer.  Reverting the buffer to its original content is something that happens "under" the glass -- there's no reason reverting should change the magnification/reduction level.

We could fix this by making `revert-buffer--default' preserve `text-scale-mode' and `text-scale-amount', either unconditionally or conditionally based on a new variable `revert-buffer-preserve-text-scale'.  I would be fine with either solution; if conditional, I would opt for `revert-buffer-preserve-text-scale' being true by default.  (Note that if someone has written a custom `revert-buffer-function' to be used instead of `revert-buffer--default', then we should let their custom function decide what to do about text-scaling; it can choose whether or not to honor a `revert-buffer-preserve-text-scale' variable.)

Before I go down this road, I'd like to know if anyone thinks text-scaling is just another minor mode and should be bound together with other modes via the existing `preserve-modes' flag to `revert-buffer'.  I think the opposite: that is, I think text-scaling should be treated specially, for the reasons given above.  I just wanted to check here before writing any code.

Best regards,
-Karl

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Eli Zaretskii
> From: Karl Fogel <[hidden email]>
> Date: Fri, 29 Nov 2019 14:24:14 -0600
>
> Before I go down this road, I'd like to know if anyone thinks text-scaling is just another minor mode and should be bound together with other modes via the existing `preserve-modes' flag to `revert-buffer'.  I think the opposite: that is, I think text-scaling should be treated specially, for the reasons given above.  I just wanted to check here before writing any code.

There are other buffer-local minor modes that are of similar nature:
display-line-numbers-mode, display-fill-column-indicator-mode, and
maybe some others (e.g., what about hl-line-mode?).  So if we are
going to preserve text-scale-mode across reverting, I think we should
have a list of modes to preserve, not just exempt this one mode.

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Karl Fogel-2
On 29 Nov 2019, Eli Zaretskii wrote:
>There are other buffer-local minor modes that are of similar nature:
>display-line-numbers-mode, display-fill-column-indicator-mode, and
>maybe some others (e.g., what about hl-line-mode?).  So if we are
>going to preserve text-scale-mode across reverting, I think we should
>have a list of modes to preserve, not just exempt this one mode.

Good thinking; thanks, Eli.

So we would have a list variable `revert-buffer-preserved-modes', with a default value that includes:

  `text-scale-mode'
  `display-line-numbers-mode'
  `display-fill-column-indicator-mode'
  `hl-line-mode' (maybe?)
  ...maybe others...

Over time we can adjust the default contents of this list.

One minor implementation detail: some modes may have ancillary information they need to preserve along with the `foo-mode' boolean.  For example, `text-scale-mode' may need `text-scale-mode-amount' to be preserved: toggling `text-scale-mode' directly leaves that variable alone, so that the toggles jump between default text scale and whatever text scale one most recently set explicitly; however, `revert-buffer' causes `text-scale-mode-amount' to be reset back to 0.  I haven't dug into the code yet to see whether this was really a design decision or just an accident of implementation that can be fixed.  If it turns out that some modes need extra information preserved along with the mode variable, then the form of the proposed new list `revert-buffer-preserved-modes' might be a bit more complex than just a list of mode name symbols.  E.g., each element could be either a mode name symbol or a sublist, and in the latter case the first element of the sublist is the mode name symbol and any subsequent elements are mode-specific variables that need to be preserved.

Anyway, that's an implementation detail and can be figured out later.

In general, are you in favor of having a default list of preserved modes for `revert-buffer', with the above modes as the initial candidates for membership in that list?

Best regards,
-Karl

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Stefan Monnier
In reply to this post by Eli Zaretskii
> There are other buffer-local minor modes that are of similar nature:
> display-line-numbers-mode, display-fill-column-indicator-mode, and
> maybe some others (e.g., what about hl-line-mode?).  So if we are
> going to preserve text-scale-mode across reverting, I think we should
> have a list of modes to preserve, not just exempt this one mode.

There's already the `preserve-modes` argument to `revert-buffer`
for that.  For some reason we don't expose it to the end-user :-(


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Karl Fogel-2
On 29 Nov 2019, Stefan Monnier wrote:
>> There are other buffer-local minor modes that are of similar nature:
>> display-line-numbers-mode, display-fill-column-indicator-mode, and
>> maybe some others (e.g., what about hl-line-mode?).  So if we are
>> going to preserve text-scale-mode across reverting, I think we should
>> have a list of modes to preserve, not just exempt this one mode.
>
>There's already the `preserve-modes` argument to `revert-buffer`
>for that.  For some reason we don't expose it to the end-user :-(

My original post explains why I don't think that parameter is a good mechanism for these kinds of display-related behaviors.

However, the more I think about it, the more I wonder if maybe just defaulting `preserve-modes' to true might be the best solution.  Does it really make sense to *not* preserve modes by default when reverting a buffer?  I think most people just expect `revert-buffer' to revert the contents -- they're trying to discard their current changes and get back in sync with whatever's in the underlying file.  That doesn't necessarily mean they want to change Emacs's behavior in terms of how the buffer is displayed or how it reacts to manipulation.  Reversion is about content, not about interaction.

So how about we just flip that parameter's sense and change its name to `discard-modes' (or `revert-modes', or whatever)?

That would solve the original problem that motivated me, and it might also be a better default behavior for users anyway.

(I'm happy to make the change and update all callers, if there is agreement.)

Best regards,
-Karl

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Eli Zaretskii
> From: Karl Fogel <[hidden email]>
> Cc: Eli Zaretskii <[hidden email]>,  [hidden email]
> Date: Fri, 29 Nov 2019 15:20:42 -0600
>
> However, the more I think about it, the more I wonder if maybe just
> defaulting `preserve-modes' to true might be the best solution. Does it really make sense to *not* preserve modes by default when reverting a buffer?

Well, the fact that we did that for eons is one huge evidence that it
does make sense.

But I won't object to exposing that argument via a defcustom or a
special value of prefix arg.

> So how about we just flip that parameter's sense and change its name to `discard-modes' (or `revert-modes', or whatever)?

This must be an opt-in feature, for backward compatibility reasons.

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Eli Zaretskii
In reply to this post by Karl Fogel-2
> From: Karl Fogel <[hidden email]>
> Cc: [hidden email]
> Date: Fri, 29 Nov 2019 15:02:58 -0600
>
> In general, are you in favor of having a default list of preserved modes for `revert-buffer', with the above modes as the initial candidates for membership in that list?

I'm okay with that, as an option, but let's hear what others think.
The more modes we want to preserve, the more backward incompatibility
this change will bring, so we may wish to tread with care.

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Karl Fogel-2
In reply to this post by Eli Zaretskii
On 30 Nov 2019, Eli Zaretskii wrote:
>> In general, are you in favor of having a default list of preserved
>> modes for `revert-buffer', with the above modes as the initial
>> candidates for membership in that list?
>
>I'm okay with that, as an option, but let's hear what others think.
>The more modes we want to preserve, the more backward incompatibility
>this change will bring, so we may wish to tread with care.

Agreed (see below).

>> However, the more I think about it, the more I wonder if maybe just
>> defaulting `preserve-modes' to true might be the best solution. Does
>> it really make sense to *not* preserve modes by default when
>> reverting a buffer?
>
>Well, the fact that we did that for eons is one huge evidence that it
>does make sense.
>
>But I won't object to exposing that argument via a defcustom or a
>special value of prefix arg.
>
>> So how about we just flip that parameter's sense and change its name
>to `discard-modes' (or `revert-modes', or whatever)?
>
>This must be an opt-in feature, for backward compatibility reasons.

I have to agree, despite my earlier eagerness.  Just as whoever implemented text-scaling using a minor mode probably had no thought about the interaction with `revert-buffer' given the latter's `preserve-modes' parameter, we cannot know the consequences of reversing that parameter's sense now.

Given that it would be opt-in, however, the value of implementing becomes somewhat low, since as an opt-in it would not by default solve the original bug, which is that `revert-buffer' behaves unexpectedly w.r.t. text-scaling.

So going back to the previous proposal seems like the best approach, because it allows us to change just the buggy behavior(s) and nothing else.  To reiterate: that proposal is to have a new list variable `revert-buffer-preserved-modes', whose default value includes these modes:

  `text-scale-mode'
  `display-line-numbers-mode'
  `display-fill-column-indicator-mode'
  `hl-line-mode' (maybe?)

Over time we could adjust the default contents of this list, of course.

I would also like to hear what others here think, both about the overall idea and about what modes should be in that list.

Best regards,
-Karl

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Stefan Kangas
Karl Fogel <[hidden email]> writes:

> So going back to the previous proposal seems like the best approach, because it allows us to change just the buggy behavior(s) and nothing else.  To reiterate: that proposal is to have a new list variable `revert-buffer-preserved-modes', whose default value includes these modes:
>
>   `text-scale-mode'
>   `display-line-numbers-mode'
>   `display-fill-column-indicator-mode'
>   `hl-line-mode' (maybe?)
>
> Over time we could adjust the default contents of this list, of course.
>
> I would also like to hear what others here think, both about the overall idea and about what modes should be in that list.

Since you are asking for opinions:

The bug you originally mentioned with text-scale-mode and
revert-buffer is incredibly frustrating.  I incorrectly assumed there
was a good reason for the behaviour, and never bothered looked into
it.  So thank you for doing that.

I agree that all four modes above should stay enabled on
revert-buffer, including hl-line-mode.

Best regards,
Stefan Kangas

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Juanma Barranquero

On Sun, Dec 1, 2019 at 10:48 AM Stefan Kangas <[hidden email]> wrote:

> I agree that all four modes above should stay enabled on
> revert-buffer, including hl-line-mode.

I agree, too.

Other modes to consider: visual-line-mode, so-long-mode and indent-tabs-mode.
Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Michael Albinus
In reply to this post by Stefan Kangas
Stefan Kangas <[hidden email]> writes:

>> So going back to the previous proposal seems like the best approach,
>> because it allows us to change just the buggy behavior(s) and
>> nothing else.  To reiterate: that proposal is to have a new list
>> variable `revert-buffer-preserved-modes', whose default value
>> includes these modes:
>>
>>   `text-scale-mode'
>>   `display-line-numbers-mode'
>>   `display-fill-column-indicator-mode'
>>   `hl-line-mode' (maybe?)
>>
>> Over time we could adjust the default contents of this list, of course.
>>
>> I would also like to hear what others here think, both about the overall idea and about what modes should be in that list.
>
> Since you are asking for opinions:
>
> The bug you originally mentioned with text-scale-mode and
> revert-buffer is incredibly frustrating.  I incorrectly assumed there
> was a good reason for the behaviour, and never bothered looked into
> it.  So thank you for doing that.
>
> I agree that all four modes above should stay enabled on
> revert-buffer, including hl-line-mode.

I haven't tested, but wouldn't it be sufficient if these minor modes
add the `permanent-local' property to their buffer-local variables?

> Best regards,
> Stefan Kangas

Best regards, Michael.

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Karl Fogel-2
On 01 Dec 2019, Michael Albinus wrote:
>I haven't tested, but wouldn't it be sufficient if these minor modes
>add the `permanent-local' property to their buffer-local variables?

Thank you for pointing that out (that property was unknown to me).  I'd make sure to look into that, though there might be other consequences to setting that property, so I can't say yes or no without further research.

Best regards,
-Karl

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Eli Zaretskii
In reply to this post by Juanma Barranquero
> From: Juanma Barranquero <[hidden email]>
> Date: Sun, 1 Dec 2019 11:24:27 +0100
> Cc: Karl Fogel <[hidden email]>, Eli Zaretskii <[hidden email]>,
> Stefan Monnier <[hidden email]>, Emacs developers <[hidden email]>
>
> Other modes to consider: visual-line-mode, so-long-mode and indent-tabs-mode.

IMO, at least the first two don't belong: reverting a buffer is a
valid way of getting rid of them.

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Stefan Monnier
In reply to this post by Karl Fogel-2
> bug, which is that `revert-buffer' behaves unexpectedly w.r.t. text-scaling.

FWIW, I think that this is in the eye of the beholder.

In my opinion, we should move towards an arrangement where the
revert-buffer command focuses on synchronizing the buffer's state with
the external world (e.g. text content, read-only-mode, VC state, ...)
and then have another command for resetting the modes (probably
normal-mode).

Currently, the revert-buffer's C-u is used to choose between "revert
from file or revert from the auto-save file".  Personally I never use
that (probably the most obvious reason is that I don't use auto-save
files at all), so I'd gladly change this C-u to mean "and reset the
modes".


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Juri Linkov-2
In reply to this post by Karl Fogel-2
> Before I go down this road, I'd like to know if anyone thinks text-scaling
> is just another minor mode and should be bound together with other modes
> via the existing `preserve-modes' flag to `revert-buffer'.  I think the
> opposite: that is, I think text-scaling should be treated specially, for
> the reasons given above.  I just wanted to check here before writing
> any code.

Recently I encountered a similar problem - I was reading a read-only
file buffer in View mode (with a non-nil 'view-read-only') where
SPC scrolls the page forward.  Then needed to revert the buffer,
but afterwards SPC changed its behavior to self-inserting spaces
because read-only view-mode was disabled after reverting.

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Karl Fogel-2
In reply to this post by Stefan Monnier
On 01 Dec 2019, Stefan Monnier wrote:
>FWIW, I think that this is in the eye of the beholder.
>
>In my opinion, we should move towards an arrangement where the
>revert-buffer command focuses on synchronizing the buffer's state with
>the external world (e.g. text content, read-only-mode, VC state, ...)
>and then have another command for resetting the modes (probably
>normal-mode).

I think that would be a good direction to move in.  How would that differ, in essence, from my proposal (earlier in this thread) to have `revert-buffer' not reset modes by default?  After I proposed it, Eli persuaded me that that would be too big a change and that for compatibility reasons we shouldn't do it.

I'm easily unpersuaded from that recent persuasion, though... Frankly, I think we don't have much empirical knowledge about what users want/expect from `revert-buffer' beyond reverting files contents.  The fact that it clobbers modes seems somewhat random.  I suspect the mode-clobbering was probably designed for some specific use case, and afterwards as people created new modes they were almost never thinking about whether `revert-buffer' should revert their new mode or not, because they usually weren't aware that `revert-buffer' does that at all.

The `preserve-modes' argument was added by Richard (commit 9a30563f9) probably in order to help `vc-revert-buffer1' (commit e7ffe86a9068).  I wish at that point, in 1995, we had stopped to ask ourselves why `revert-buffer' was clobbering modes in the first place.  But we didn't, and now it's 24 years and about ten zillion new modes later :-), and we have no clear idea what lossage might ensue if we change the behavior.

But if you want to move to an arrangement where `revert-buffer' focuses on synchronizing the buffer with the outside world, and people use a different command for resetting the modes, that sounds okay to me.  It would certainly resolve the one thing that I *do* feel pretty sure is a bug here, which is that reverting a buffer reverts the text-scaling too.  If someone increased the text size, they did so for purely visual purposes: it's so they can see the contents better.  Their eyesight doesn't change when they revert the buffer's contents, and clearly they know how to scale the size down again if they want to (since they scaled it up in the first place).

>Currently, the revert-buffer's C-u is used to choose between "revert
>from file or revert from the auto-save file".  Personally I never use
>that (probably the most obvious reason is that I don't use auto-save
>files at all), so I'd gladly change this C-u to mean "and reset the
>modes".

I never use the "revert from auto-save" behavior either.  I'd be happy to have C-u mean "reset the modes" instead, but remember that would mean reversing the default behavior: instead of clobbering modes by default, it would preserve modes by default and clobber only on opt-in.

Even though Eli unconvinced me of that earlier, it turns out I'm weak-minded enough to have already fallen back into thinking it's a good idea now that you suggest it.  So I'm +1 on what you say, at least until Eli posts again?

Best regards,
-Karl

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Eli Zaretskii
> From: Karl Fogel <[hidden email]>
> Cc: Eli Zaretskii <[hidden email]>,  [hidden email]
> Date: Sun, 01 Dec 2019 20:01:37 -0600
>
> Even though Eli unconvinced me of that earlier, it turns out I'm weak-minded enough to have already fallen back into thinking it's a good idea now that you suggest it.  So I'm +1 on what you say, at least until Eli posts again?

If we use a special value of the argument, not just C-u, I'm okay with
that (and I think I already said that in this discussion, didn't I?).

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Karl Fogel-2
On 02 Dec 2019, Eli Zaretskii wrote:
>> Even though Eli unconvinced me of that earlier, it turns out I'm
>> weak-minded enough to have already fallen back into thinking it's a
>> good idea now that you suggest it.  So I'm +1 on what you say, at
>> least until Eli posts again?
>
>If we use a special value of the argument, not just C-u, I'm okay with
>that (and I think I already said that in this discussion, didn't I?).

I think the first question is what the default behavior of `revert-buffer' should be -- after we decide that, we can decide what special prefix arg is used to access the non-default behavior.

Stefan proposed that the new default be to *not* reset modes, and that C-u could be reassigned to mean "and reset the modes".  Here is what he said:

>Currently, the revert-buffer's C-u is used to choose between "revert
>from file or revert from the auto-save file".  Personally I never use
>that (probably the most obvious reason is that I don't use auto-save
>files at all), so I'd gladly change this C-u to mean "and reset the
>modes".

To consider Stefan's proposal, we'd have to first decide that `revert-buffer' will no longer reset modes by default.  I had thought you were opposed to that change, on compatibility grounds.  But maybe you just meant that revert-buffer's `preserve-modes' argument should retain its current meaning when called non-interactively from Lisp, but that it would be okay for interactive `revert-buffer' to stop resetting modes by default?  In other words, the default interactive behavior could be changed to be the opposite of the default non-interactive behavior.

Was that what you were saying?

Regarding Stefan's proposal: I don't know how often people depend on the current C-u behavior of reverting from auto-save file.  But if we're changing the interactive default to not reset modes, then I think a special prefix -- different from C-u -- to access the old reset-modes behavior would be fine, partly because it would be rarely used!  Also, I presume we would have a new customizeable variable `revert-buffer-reset-modes' (defaulting to nil) that people who have a permanent preference could set.

Best regards,
-Karl

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Eli Zaretskii
> From: Karl Fogel <[hidden email]>
> Cc: [hidden email],  [hidden email]
> Date: Mon, 02 Dec 2019 11:19:08 -0600
>
> Stefan proposed that the new default be to *not* reset modes, and that C-u could be reassigned to mean "and reset the modes".  Here is what he said:
>
> >Currently, the revert-buffer's C-u is used to choose between "revert
> >from file or revert from the auto-save file".  Personally I never use
> >that (probably the most obvious reason is that I don't use auto-save
> >files at all), so I'd gladly change this C-u to mean "and reset the
> >modes".

I meant it the other way around.

If we want to consider changing the default, IMO it would be better to
add a defcustom that will start at nil (i.e. the default stays as it
was before), and will in the future become non-nil if that's what
people want.

Reply | Threaded
Open this post in threaded view
|

Re: Should `revert-buffer' preserve text-scaling by default?

Karl Fogel-2
On 02 Dec 2019, Eli Zaretskii wrote:
>If we want to consider changing the default, IMO it would be better to
>add a defcustom that will start at nil (i.e. the default stays as it
>was before), and will in the future become non-nil if that's what
>people want.

Well, that would be progress, at least.  I would definitely set it :-).

What is the process by which we might eventually decide to change the default to non-nil (that is, to *not* resetting modes)?

In other words, how will the mere passage of time affect our willingness to change the default interactive behavior?  Is it that the existence of the variable will make it easy for us to ask the question (six months from now or whenever) "Hey, folks, did you know about this new defcustom and did you consciously decide whether to set it?"  We can easily ask that question before implementing the variable, though.  If the idea is we would post to some forum(s) and ask, then let's just do those posts now and ask whether people would like `revert-buffer' to stop resetting modes by default.

(I'm not sure which forums those would be, other than this one.  There's no "emacs-users" mailing list listed at https://savannah.gnu.org/mail/?group=emacs.)

I understand the general principle of proceeding cautiously.  My question is how introducing this defcustom is a step along any particular road -- how it is part of "proceeding"?  Maybe by people reading the NEWS entry about the new defcustom?  In that case, maybe we should solicit input directly from the NEWS entry, so we have more information to work with when we're later considering whether to change its default value.

Once we decide what we're doing here, I'm happy to implement it, by the way.

Best regards,
-Karl

12