bug#24104: 25.1.50; frame-or-buffer-changed-p also check file size

bug#24104: 25.1.50; frame-or-buffer-changed-p also check file size

[Tino Calancha]

Assume a buffer BUF, with size, SIZE, returning non-nil for predicate
'buffer-modified-p'; then we write some text on BUF, so that the
new size is SIZE_2 != SIZE.
In this scenario, 'frame-or-buffer-changed-p' return nil, i.e.,
the buffer state appears to not have changed.

It might be convenient to extend 'frame-or-buffer-changed-p' so that,
it also check the buffer size.  Then, for instance, Ibuffer operating
on auto mode ('ibuffer-auto-mode') would update the listing.

emacs -Q --eval="(progn (require 'ibuffer) (ibuffer))"
M-x ibuffer-auto-mode RET
M-! echo Hi Emacs! RET
;; *Shell Command Output* appear with size 10
M-! echo Could you update Ibuffer for me?
;; It seems not: maybe i should add please next time...

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 1c9ef031b2bd283c6641d5249cbfa7226b764ab1 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Fri, 29 Jul 2016 21:05:23 +0900
Subject: [PATCH] frame-or-buffer-changed-p also check file size

* src/dispnew.c (frame-or-buffer-changed-p):
Check if buffer size has changed.
Update the new file size in VARIABLE (Bug#24104).
---
  src/dispnew.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/dispnew.c b/src/dispnew.c
index 82d0b76..0218a68 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -5812,7 +5812,7 @@ DEFUN ("frame-or-buffer-changed-p", 
Fframe_or_buffer_changed_p,
  VARIABLE is a variable name whose value is either nil or a state vector
  that will be updated to contain all frames and buffers,
  aside from buffers whose names start with space,
-along with the buffers' read-only and modified flags.  This allows a fast
+along with the buffers' read-only, modified flags and buffer size.  This 
allows a fast
  check to see whether buffer menus might need to be recomputed.
  If this function returns non-nil, it updates the internal vector to 
reflect
  the current state.
@@ -5864,6 +5864,8 @@ pass nil for VARIABLE.  */)
  	goto changed;
        if (!EQ (AREF (state, idx++), Fbuffer_modified_p (buf)))
  	goto changed;
+      if (!EQ (AREF (state, idx++), BVAR (XBUFFER (buf), save_length)))
+	goto changed;
      }
    if (idx == ASIZE (state))
      goto changed;
@@ -5914,6 +5916,8 @@ pass nil for VARIABLE.  */)
        idx++;
        ASET (state, idx, Fbuffer_modified_p (buf));
        idx++;
+      ASET (state, idx, Fbuffer_size (buf));
+      idx++;
      }
    /* Fill up the vector with lambdas (always at least one).  */
    ASET (state, idx, Qlambda);
-- 
2.8.1


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;


GNU Emacs 25.1.50 (x86_64-pc-linux-gnu, GTK+ Version 3.20.6)
  of 2016-07-28
Repository revision: 4a5b6e621c68172bb69d60fe8a76932f7c779f81

bug#24104: 25.1.50; frame-or-buffer-changed-p also check file size

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Fri, 29 Jul 2016 21:10:29 +0900 (JST)
> 
> 
> Assume a buffer BUF, with size, SIZE, returning non-nil for predicate
> 'buffer-modified-p'; then we write some text on BUF, so that the
> new size is SIZE_2 != SIZE.
> In this scenario, 'frame-or-buffer-changed-p' return nil, i.e.,
> the buffer state appears to not have changed.

Because it didn't.  The buffer was changed, and it still is.  In
particular, the original change that caused buffer-modified-p to
return non-nil could also be (and normally is) a change in the size of
the buffer.

> It might be convenient to extend 'frame-or-buffer-changed-p' so that,
> it also check the buffer size.

This function is used to decide whether we need to recompute menus, so
your proposed change will cause extra recomputation of them.  I'm not
sure we want that, as recomputing menus might be expensive.

> Then, for instance, Ibuffer operating
> on auto mode ('ibuffer-auto-mode') would update the listing.

If Ibuffer has some problem, I suggest to describe that problem and
try solving it for Ibuffer alone.

Thanks.

bug#24104: 25.1.50; frame-or-buffer-changed-p also check file size

[Tino Calancha]

On Fri, 29 Jul 2016, Eli Zaretskii wrote:

> This function is used to decide whether we need to recompute menus, so
> your proposed change will cause extra recomputation of them.  I'm not
> sure we want that, as recomputing menus might be expensive.
Ok.

>> Then, for instance, Ibuffer operating
>> on auto mode ('ibuffer-auto-mode') would update the listing.
>
> If Ibuffer has some problem, I suggest to describe that problem and
> try solving it for Ibuffer alone.
Agreed.
In fact, my main target in this report is to enhance 
'ibuffer-auto-mode'.  I agree that an enhancement in Ibuffer should
not come with a penalty in other parts of Emacs.
We can close this report.
Thank you very much.

bug#24104: 25.1.50; frame-or-buffer-changed-p also check file size

[Stefan Kangas]

Tino Calancha <tino.calancha@gmail.com> writes:

> On Fri, 29 Jul 2016, Eli Zaretskii wrote:
>
>> This function is used to decide whether we need to recompute menus, so
>> your proposed change will cause extra recomputation of them.  I'm not
>> sure we want that, as recomputing menus might be expensive.
> Ok.
[...]
> We can close this report.

I'm consequently closing this bug report now.

Best regards,
Stefan Kangas

bug#24104: 25.1.50; frame-or-buffer-changed-p also check file size

[Drew Adams]

> > Assume a buffer BUF, with size, SIZE, returning non-nil for predicate
> > 'buffer-modified-p'; then we write some text on BUF, so that the
> > new size is SIZE_2 != SIZE.
> > In this scenario, 'frame-or-buffer-changed-p' return nil, i.e.,
> > the buffer state appears to not have changed.
> 
> Because it didn't.  The buffer was changed, and it still is.  In
> particular, the original change that caused buffer-modified-p to
> return non-nil could also be (and normally is) a change in the size of
> the buffer.

If the meaning of `frame-or-buffer-changed-p' in this scenario is,
as you suggest, intended to reflect whether or not `buffer-modified-p'
has changed (and not whether the buffer has changed), then its doc
should say so.

Whether `buffer-modified-p' has changed is something different from
whether the buffer state has changed.  If `buffer-modified-p' is
already non-nil then additional changes to the buffer will not cause
a change in `buffer-modified-p', as Tino's example illustrates.

The name `frame-or-buffer-changed-p', and its current doc, suggest
that it should return non-nil if the buffer appears to have changed.
The buffer has changed in this case, even if the value of
`buffer-modified-p' has not changed (it is still non-nil).  But
apparently the buffer does not "appear" to have changed.  That's OK,
but what's involved in this appearing is unclear.

Something should be said in the doc about what kinds of changes lead
to a change "appearing" to have occurred (or what kinds do not lead
to that).  IOW, the doc should say something about how the
"appearance" of change is judged/detected.  If read-only and
modified flags determine this (alone), then the doc should say so.

Something should be said about just what is meant by (included
in) a buffer state change and a frame state change.

If all that is meant, for buffer change, is what is said (later) in
the doc for VARIABLE, i.e., all that is checked are the (1) read-only
and (2) modified flags, then just say that up front, and not only
for VARIABLE.  That, I think, would make most of the above clear.

Except for frame state change - that part is still completely
unspecified, AFAICT.  Just what is meant by (included in) the frame
state, and how is a change in frame state detected/recorded by Emacs
("appear" to have occurred).  For buffer changes: read-only and
modified flags - OK.  What about for frame-state changes?

Finally, the doc should not talk about some "internal vector" BEFORE
that has been introduced.  Assuming that the "an internal variable"
of the last paragraph is what holds "the internal vector", it needs
to be introduced before we talk about "the internal vector".  And
the fact that the value of the internal variable is "the internal
vector" should be made explicit.

The last sentence of the doc is confusing/scary.  If a user can
call the function without passing an argument, is that different
from calling it and passing a nil argument?  If the behavior is
different then this is REALLY an exception - I've never heard of
such a thing.  What is this all about?  I'm guessing that that
last sentence should just be removed.

On the other hand, if the first part of the doc is to be believed,
VARIABLE, if present, must be a symbol that is a variable - which
excludes the symbol `nil'.  In that case it's still not clear to
me how the absence of VARIABLE is distinguished from the presence
of nil as an argument.  Perhaps that is done at the C level and
this is a real exception to the rule of &optional arguments.  If
so, that needs to be spelled out clearly.

If this is really about passing a symbol, and not its value (see
below), then, again, there is no need for the confusing last doc
sentence.  And in that case if a user _does_ pass nil as the
argument then is an error raised saying that nil is a constant and
not a variable?  (And again, how is this case distinguished from
an absence of VARIABLE?)

The doc seems to be essentially backward, and it doesn't make
explicit the connections that I think the author intended.

It should say (IIUC, but I probably do not) that the state of
frames and buffers - or of frame and buffer changes (?) is kept
in a vector that is the value of an internal variable, and which
for buffers records the read-only and modified flags (only), and
which for frames records [???].  The function returns non-nil if
this recorded state indicates that a change has occurred.  It
should then say that you can provide a different such variable
to use by passing it as optional argument VARIABLE.

The doc says that VARIABLE "is a variable name whose value is
...".  The value of a _name_ cannot be nil or a vector.
Presumably what is meant is that VARIABLE is a variable
(symbol) whose value is....  The value of the symbol's _name_
is a string (`symbol-name').

So you pass a symbol and not its value (nil or a vector), I guess.
Why is that?

In sum, this doc needs some work, I think, for it to help users
more and not confuse them more.  It confuses me, at least.

You can't make good use of a function if you don't know what its
intended behavior is.  Of course you can try to discover that by
trial and error ... but that's what we have doc for.