Re: The unwarranted scrolling assumption

[Lennart Borgman]

On Sat, Jun 19, 2010 at 11:33 PM, Eli Zaretskii <address@hidden> wrote:
>> From: Lennart Borgman <address@hidden>
>> Date: Sat, 19 Jun 2010 22:38:24 +0200
>> Cc: address@hidden, David De La Harpe Golden <address@hidden>
>>
>> On Sat, Jun 19, 2010 at 10:30 PM, Juanma Barranquero <address@hidden> wrote:
>> > On Sat, Jun 19, 2010 at 22:26, Lennart Borgman
>> > <address@hidden> wrote:
>> >
>> >> This is expected. The original problem is still only addressed in my 
>> >> patch.
>> >>
>> >> And that is not applied.
>> >
>> > Then, help Eli understand why your patch is the right fix.
>>
>>
>> I have done the best I can. I do not know what to do.
>>
>> The basic assumption that clip_changed can be set in narrow_to_region
>> etc and that redisplay later can decide when it can be reset is false.
>> Redisplay does not have the necessary information since it does not
>> know exactly why clip_changed was set to 1.
>>
>> I have said that there is no reason to let narrow_to_region set
>> clip_changed (even if it really seems intuitively right to do so). It
>> is much easier to let redisplay do that instead in
>> reconsider_clip_changes because then all needed information for the
>> specific window is available.
>
> Lennart, I need _evidence_, not ideas, assumptions, and theories.


Hm, you are talking to someone who is quite theoretical in those
areas. I can see we look upon code quite differently.


> We are not trying to rewrite the display code from scratch, we are
> trying to fix a relatively minor problem in an otherwise working code.
> The way to fix minor problems is not to find a totally different way
> of doing something, but to find specific reason(s) why existing code
> doesn't do what it was supposed to do.


The involvement of the display engine is quite minor. This is merely a
logical problem with updating clip_changed.

I am only talking about replacing the logic used there. The so to say
inner parts of redisplay is not at all touch by my patch. (How could
they be that, the patch will only a few lines in its final state?)


> IOW, it doesn't matter to me in how many different ways you can set
> the clip_changed flag.  What I want to understand first and foremost
> is why the _existing_ method fails.  Until I understand that, I cannot
> reason about your suggested patch, and I therefore will not consider
> it for inclusion in Emacs.


And I am trying to explain this, but I apparently fail. And this
thread has become just too long because of our problems to
communicate. It is a bit complex, indeed, and I guess that is why we
have problems.

So just let us take it again and let me be more expressive this time.
This will allow you to more clearly see if I have missed something. I
will try to take the whole picture (with some parts I have avoided to
mention to not stir up more confusion).


First, clip_changed is currently a property of the buffer. This in
itself problematic since it supposed to be used to tell if a specific
window needs to care about clip changes. Different windows might have
been displayed at different times so one of them may have the clipping
correct while another does not have it correct. Since clip_changed
belongs to the buffer you can not distinguish those cases. So it
should maybe be moved to the windows instead.

This of course makes it impossible to update it from the narrow_to_region etc.

However I can see no problem updating it at the beginning of
reconsider_clip_changes as I suggested. If we set clip_changed to 1
there as soon as current clipping does not match the clipping recorded
at last finished redisplay we can not come from the start of a new
redisplay to a finished redisplay without setting clip_changed to 1 if
the buffer clipping has changed.

NOTE: At least not if buffer clipping can not change between the last
call to reconsider_clip_changes and a finished redisplay. If this is
not true then my suggestion will fail if clip_changed is 0 when the
buffer clipping is changed.


You can also see in the code that clip_changed is not just about
buffer clipping, look in reconsider_clip_changes. There clip_changed
is set to 1 to reflect something quite different than clipping
changes. It is set to 1 to force a redisplay. And in the same function
the code checks (for reasons currently beyond me but I do not doubt
they are correct) that window_valid_end is not nil before it resets
clip_changed to 0.

That window_valid_end is 0 is the reason resetting clip_changed to 0
is not done. It could have been 0 before the clipping change occurred
and this might mean that we could reset clip_changed (but it need not
mean that). We do not have the slightest idea at this point because we
do not have enough information.

Therefore it looks much easier to me to just set clip_changed to 1
when it seems to be needed in redisplay instead. (But please see the
NOTE above.) It also allows clipping to be really handled by window
(which is a corner case but it is nice to fix it if we can).


To make the picture complete we must also of course be aware of what
the narrowing routines does. But that is not very complicated
regarding this. For example save-restriction-restore just sets
clip_changed to 1 when restoring clipping if some clipping change has
occurred. Those routines can not actually work in any other way when
setting clip_changed since they do not know at all what has happened
with redisplay.


> Now, I asked you repeatedly to give me values of variables inside
> reconsider_clip_changes, which would explain why clip_changed is not
> reset by it.  You didn't give those values to me, for some reason that
> I cannot understand.  If you persist in your refusal to cooperate by
> showing those values, and any others that I may need for this
> investigation, I don't see any reason to continue talking to you in
> this thread.


Sorry, some misunderstanding. I think I told, but it looks like I did
send you the actual trace output. (I have been waiting for some more
serious feedback from you about the solution I suggested. There has
been very many messages here.) The output shows that it is because
window_valid_end is 0 that clip_changed is not reset:

   warning: +++reconsider: end_valid=0, b=1, ZV=1, BEGV=1

And this is how I get it:

static INLINE void
reconsider_clip_changes (w, b)
     struct window *w;
     struct buffer *b;
{
  if (b->clip_changed
      && !(!NILP (w->window_end_valid)
           && w->current_matrix->buffer == b
           && w->current_matrix->zv == BUF_ZV (b)
           && w->current_matrix->begv == BUF_BEGV (b)))
    DebPrint (("+++reconsider: end_valid=%d, b=%d, ZV=%d, BEGV=%d",
               !NILP (w->window_end_valid),
               w->current_matrix->buffer == b,
               w->current_matrix->zv == BUF_ZV (b),
               w->current_matrix->begv == BUF_BEGV (b)));

  if (b->clip_changed
           && !NILP (w->window_end_valid)
           && w->current_matrix->buffer == b
           && w->current_matrix->zv == BUF_ZV (b)
           && w->current_matrix->begv == BUF_BEGV (b))
    b->clip_changed = 0;

...