Truncating scroll runs that copy to where we copied to

Truncating scroll runs that copy to where we copied to

[YAMAMOTO Mitsuharu]

I think that `scrolling_window' needs to truncate scroll runs that
copy to where we copied to; otherwise, `assign_row (to, from)' assigns
a previously disabled bogus row in the desired matrix when we have an
overlap in the copy destination.  Such truncation can also avoid
unnecessary copy in the actual graphics operation.

Could someone double-check the code below?

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

=== modified file 'src/dispnew.c'
*** src/dispnew.c	2011-11-19 08:39:42 +0000
--- src/dispnew.c	2011-11-20 06:44:25 +0000
***************
*** 4552,4562 ****
  	    rif->clear_window_mouse_face (w);
  	    rif->scroll_run_hook (w, r);
  
! 	    /* Invalidate runs that copy from where we copied to.  */
  	    for (j = i + 1; j < nruns; ++j)
  	      {
  		struct run *p = runs[j];
  
  		if ((p->current_y >= r->desired_y
  		     && p->current_y < r->desired_y + r->height)
  		    || (p->current_y + p->height >= r->desired_y
--- 4552,4594 ----
  	    rif->clear_window_mouse_face (w);
  	    rif->scroll_run_hook (w, r);
  
! 	    /* Truncate runs that copy to where we copied to, and
! 	       invalidate runs that copy from where we copied to.  */
  	    for (j = i + 1; j < nruns; ++j)
  	      {
  		struct run *p = runs[j];
  
+ 		if (p->nrows > 0
+ 		    && p->desired_vpos < r->desired_vpos + r->nrows
+ 		    && p->desired_vpos + p->nrows > r->desired_vpos)
+ 		  {
+ 		    if (p->desired_vpos < r->desired_vpos)
+ 		      {
+ 			p->nrows = r->desired_vpos - p->desired_vpos;
+ 			p->height = r->desired_y - p->desired_y;
+ 		      }
+ 		    else
+ 		      {
+ 			int nrows_copied = (r->desired_vpos + r->nrows
+ 					    - p->desired_vpos);
+ 
+ 			if (p->nrows <= nrows_copied)
+ 			  p->nrows = 0;
+ 			else
+ 			  {
+ 			    int height_copied = (r->desired_y + r->height
+ 						 - p->desired_y);
+ 
+ 			    p->current_vpos += nrows_copied;
+ 			    p->desired_vpos += nrows_copied;
+ 			    p->nrows -= nrows_copied;
+ 			    p->current_y += height_copied;
+ 			    p->desired_y += height_copied;
+ 			    p->height -= height_copied;
+ 			  }
+ 		      }
+ 		  }
+ 
  		if ((p->current_y >= r->desired_y
  		     && p->current_y < r->desired_y + r->height)
  		    || (p->current_y + p->height >= r->desired_y

Re: Truncating scroll runs that copy to where we copied to

[Eli Zaretskii]

> Date: Sun, 20 Nov 2011 16:13:59 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> 
> I think that `scrolling_window' needs to truncate scroll runs that
> copy to where we copied to; otherwise, `assign_row (to, from)' assigns
> a previously disabled bogus row in the desired matrix when we have an
> overlap in the copy destination.  Such truncation can also avoid
> unnecessary copy in the actual graphics operation.
> 
> Could someone double-check the code below?

Could you please elaborate on the rationale?  Like, give a specific
use case with a few rows in the current and desired matrix, and show
why the current code does not DTRT?

I'm not saying you are wrong (I have my doubts about portions of that
function), just that we should at least look into a specific test
case, even if simplified one, to make sure we are fixing a real bug.

TIA

Re: Truncating scroll runs that copy to where we copied to

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 20 Nov 2011 20:23:27 +0200, Eli Zaretskii <eliz@gnu.org> said:

>> Date: Sun, 20 Nov 2011 16:13:59 +0900 From: YAMAMOTO Mitsuharu
>> <mituharu@math.s.chiba-u.ac.jp>
>> 
>> I think that `scrolling_window' needs to truncate scroll runs that
>> copy to where we copied to; otherwise, `assign_row (to, from)'
>> assigns a previously disabled bogus row in the desired matrix when
>> we have an overlap in the copy destination.  Such truncation can
>> also avoid unnecessary copy in the actual graphics operation.
>> 
>> Could someone double-check the code below?

> Could you please elaborate on the rationale?  Like, give a specific
> use case with a few rows in the current and desired matrix, and show
> why the current code does not DTRT?

> I'm not saying you are wrong (I have my doubts about portions of
> that function), just that we should at least look into a specific
> test case, even if simplified one, to make sure we are fixing a real
> bug.

1. Create a file whose contents is as follows (not including blank
   lines):

1
2
b
c
d
e
3
4
a
b
c
d
e

2. % emacs -Q
3. Resize the frame so the window occupies 10 rows.
4. Find the file created at Step 1.
   Then the first (last) displayed row becomes `1' (`b', respectively).
5. M-<
6. C-v
7. M-v

Then, the second line is displayed as `b' instead of `2'.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Truncating scroll runs that copy to where we copied to

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

On Nov 20, 2011, at 7:19 PM, YAMAMOTO Mitsuharu wrote:
>  Then, the second line is displayed as `b' instead of `2'.

Because nobody else replied:  I reproduced it easily and was shocked too see this.   Please do fix this bug before the next release.

I have, lately, seen an occasional and elusive problem with the display of header lines (or the lack thereof) and am now wondering if this could be due to the same issue.  I'll wait and see.


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 163 bytes --]

Re: Truncating scroll runs that copy to where we copied to

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 20 Nov 2011 16:13:59 +0900, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> said:

> I think that `scrolling_window' needs to truncate scroll runs that
> copy to where we copied to; otherwise, `assign_row (to, from)'
> assigns a previously disabled bogus row in the desired matrix when
> we have an overlap in the copy destination.  Such truncation can
> also avoid unnecessary copy in the actual graphics operation.

> Could someone double-check the code below?

I found a few problems with the code myself:

* The truncation should also be done when r->current_y == r->desired_y,
  because `assign_row (to, from)' is executed for this case.

* The array `runs' should be kept sorted by copied pixel lines even
  after the truncation.

* The condition for the invalidation

		if ((p->current_y >= r->desired_y
		     && p->current_y < r->desired_y + r->height)
		    || (p->current_y + p->height >= r->desired_y
			&& (p->current_y + p->height
			    < r->desired_y + r->height)))

  in the current code is slightly wrong (off-by-one in 2 places), and
  the corrected one can be simplified under the condition that `runs'
  is sorted (see the comment in the code below).

Below is a revised patch.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

=== modified file 'src/dispnew.c'
*** src/dispnew.c	2011-11-19 08:39:42 +0000
--- src/dispnew.c	2011-11-22 00:19:27 +0000
***************
*** 4551,4568 ****
  	  {
  	    rif->clear_window_mouse_face (w);
  	    rif->scroll_run_hook (w, r);
  
! 	    /* Invalidate runs that copy from where we copied to.  */
! 	    for (j = i + 1; j < nruns; ++j)
  	      {
! 		struct run *p = runs[j];
  
! 		if ((p->current_y >= r->desired_y
  		     && p->current_y < r->desired_y + r->height)
! 		    || (p->current_y + p->height >= r->desired_y
  			&& (p->current_y + p->height
! 			    < r->desired_y + r->height)))
! 		  p->nrows = 0;
  	      }
  	  }
  
--- 4551,4619 ----
  	  {
  	    rif->clear_window_mouse_face (w);
  	    rif->scroll_run_hook (w, r);
+ 	  }
+ 
+ 	/* Truncate runs that copy to where we copied to, and
+ 	   invalidate runs that copy from where we copied to.  */
+ 	for (j = nruns - 1; j > i; --j)
+ 	  {
+ 	    struct run *p = runs[j];
+ 	    int truncated_p = 0;
  
! 	    if (p->nrows > 0
! 		&& p->desired_y < r->desired_y + r->height
! 		&& p->desired_y + p->height > r->desired_y)
  	      {
! 		if (p->desired_y < r->desired_y)
! 		  {
! 		    p->nrows = r->desired_vpos - p->desired_vpos;
! 		    p->height = r->desired_y - p->desired_y;
! 		    truncated_p = 1;
! 		  }
! 		else
! 		  {
! 		    int nrows_copied = (r->desired_vpos + r->nrows
! 					- p->desired_vpos);
! 
! 		    if (p->nrows <= nrows_copied)
! 		      p->nrows = 0;
! 		    else
! 		      {
! 			int height_copied = (r->desired_y + r->height
! 					     - p->desired_y);
! 
! 			p->current_vpos += nrows_copied;
! 			p->desired_vpos += nrows_copied;
! 			p->nrows -= nrows_copied;
! 			p->current_y += height_copied;
! 			p->desired_y += height_copied;
! 			p->height -= height_copied;
! 			truncated_p = 1;
! 		      }
! 		  }
! 	      }
  
! 	    if (r->current_y != r->desired_y
! 		/* The condition below is equivalent to
! 		   ((p->current_y >= r->desired_y
  		     && p->current_y < r->desired_y + r->height)
! 		    || (p->current_y + p->height > r->desired_y
  			&& (p->current_y + p->height
! 			    <= r->desired_y + r->height)))
! 		   because we have 0 < p->height <= r->height.  */
! 		&& p->current_y < r->desired_y + r->height
! 		&& p->current_y + p->height > r->desired_y)
! 	      p->nrows = 0;
! 
! 	    /* Reorder runs by copied pixel lines if truncated.  */
! 	    if (truncated_p && p->nrows > 0)
! 	      {
! 		int k = nruns - 1;
! 
! 		while (runs[k]->nrows == 0 || runs[k]->height < p->height)
! 		  k--;
! 		memmove (runs + j, runs + j + 1, (k - j) * sizeof (*runs));
! 		runs[k] = p;
  	      }
  	  }
  

Re: Truncating scroll runs that copy to where we copied to

[Eli Zaretskii]

> From: David Reitter <david.reitter@gmail.com>
> Date: Mon, 21 Nov 2011 18:50:07 -0500
> Cc: Eli Zaretskii <eliz@gnu.org>,
>  emacs-devel@gnu.org
> 
> On Nov 20, 2011, at 7:19 PM, YAMAMOTO Mitsuharu wrote:
> >  Then, the second line is displayed as `b' instead of `2'.
> 
> Because nobody else replied:  I reproduced it easily and was shocked too see this.

I have no reasons to believe the test case will not reproduce the
problem.  The reply that is expected from me is whether the proposed
patch is correct or not, and for that it is not enough to confirm that
the test case is reproducible.  I don't have the answer yet.

FWIW, this code was not touched since Emacs 21.1 was released.  So
evidently the effects of this issue are quite subtle in practice.

> Please do fix this bug before the next release.

You can help if you explain in more detail what is wrong with the
current code.  That is what I need to assess the correctness of the
patch.  I hoped that Yamamoto-san would do that.  Failing that, I'll
need to figure it out myself, which will take more time, and thus will
have to wait a couple of days until I have that time to sit down and
look into this.  I do intend to do that, make no mistake.

> I have, lately, seen an occasional and elusive problem with the display of header lines (or the lack thereof) and am now wondering if this could be due to the same issue.  I'll wait and see.

It would be better to describe the problem you are seeing, if not a
recipe to reproduce it.

Alternatively, you could apply the patch and see if those problems
disappear.

Re: Truncating scroll runs that copy to where we copied to

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 22 Nov 2011 01:04:45 -0500, Eli Zaretskii <eliz@gnu.org> said:

> FWIW, this code was not touched since Emacs 21.1 was released.  So
> evidently the effects of this issue are quite subtle in practice.

That code had not taken effect for most cases (especially when there's
no partially visible row at the bottom) until I made the following
fix:

2011-05-21  YAMAMOTO Mitsuharu  <mituharu@math.s.chiba-u.ac.jp>

	* dispnew.c (scrolling_window): Don't exclude the case that the
	last enabled row in the desired matrix touches the bottom boundary.

=== modified file 'src/dispnew.c'
*** src/dispnew.c	2011-05-12 07:07:06 +0000
--- src/dispnew.c	2011-05-21 02:15:34 +0000
***************
*** 4330,4352 ****
  
    first_old = first_new = i;
  
!   /* Set last_new to the index + 1 of the last enabled row in the
!      desired matrix.  */
    i = first_new + 1;
!   while (i < desired_matrix->nrows - 1
! 	 && MATRIX_ROW (desired_matrix, i)->enabled_p
! 	 && MATRIX_ROW_BOTTOM_Y (MATRIX_ROW (desired_matrix, i)) <= yb)
!     ++i;
  
!   if (!MATRIX_ROW (desired_matrix, i)->enabled_p)
!     return 0;
  
    last_new = i;
  
!   /* Set last_old to the index + 1 of the last enabled row in the
!      current matrix.  We don't look at the enabled flag here because
!      we plan to reuse part of the display even if other parts are
!      disabled.  */
    i = first_old + 1;
    while (i < current_matrix->nrows - 1)
      {
--- 4330,4358 ----
  
    first_old = first_new = i;
  
!   /* Set last_new to the index + 1 of the row that reaches the
!      bottom boundary in the desired matrix.  Give up if we find a
!      disabled row before we reach the bottom boundary.  */
    i = first_new + 1;
!   while (i < desired_matrix->nrows - 1)
!     {
!       int bottom;
  
!       if (!MATRIX_ROW (desired_matrix, i)->enabled_p)
! 	return 0;
!       bottom = MATRIX_ROW_BOTTOM_Y (MATRIX_ROW (desired_matrix, i));
!       if (bottom <= yb)
! 	++i;
!       if (bottom >= yb)
! 	break;
!     }
  
    last_new = i;
  
!   /* Set last_old to the index + 1 of the row that reaches the bottom
!      boundary in the current matrix.  We don't look at the enabled
!      flag here because we plan to reuse part of the display even if
!      other parts are disabled.  */
    i = first_old + 1;
    while (i < current_matrix->nrows - 1)
      {

That explains people (including I) did not notice the problem for a
long time.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Truncating scroll runs that copy to where we copied to

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 22 Nov 2011 01:04:45 -0500, Eli Zaretskii <eliz@gnu.org> said:

> You can help if you explain in more detail what is wrong with the
> current code.  That is what I need to assess the correctness of the
> patch.  I hoped that Yamamoto-san would do that.  Failing that, I'll
> need to figure it out myself, which will take more time, and thus
> will have to wait a couple of days until I have that time to sit
> down and look into this.  I do intend to do that, make no mistake.

Actually, I told what was wrong with the current code in my first
post:

>>>>> On Sun, 20 Nov 2011 16:13:59 +0900, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> said:

> I think that `scrolling_window' needs to truncate scroll runs that
> copy to where we copied to; otherwise, `assign_row (to, from)'
> assigns a previously disabled bogus row in the desired matrix when
> we have an overlap in the copy destination.

	/* Assign matrix rows.  */
	for (j = 0; j < r->nrows; ++j)
	  {
	    struct glyph_row *from, *to;
	    int to_overlapped_p;

	    to = MATRIX_ROW (current_matrix, r->desired_vpos + j);
	    from = MATRIX_ROW (desired_matrix, r->desired_vpos + j);
	    to_overlapped_p = to->overlapped_p;
	    from->redraw_fringe_bitmaps_p = from->fringe_bitmap_periodic_p;
	    assign_row (to, from);
	    to->enabled_p = 1, from->enabled_p = 0;
	    to->overlapped_p = to_overlapped_p;
	  }

If there's an overlap (I don't mean the variable to_overlapped_p) in
the copy destinations of two runs, then `from' row in the desired
matrix becomes a bogus one that had been in the current matrix after
processing the first run (because assign_row actually does swap), and
that row is disabled by `from->enabled_p = 0'.  So, when processing
the second run, the `from' row is now the previously disabled bogus
row and it is assigned to a row in the current matrix.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Truncating scroll runs that copy to where we copied to

[Eli Zaretskii]

> Date: Tue, 22 Nov 2011 15:22:48 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: David Reitter <david.reitter@gmail.com>,
> 	emacs-devel@gnu.org
> 
> >>>>> On Tue, 22 Nov 2011 01:04:45 -0500, Eli Zaretskii <eliz@gnu.org> said:
> 
> > FWIW, this code was not touched since Emacs 21.1 was released.  So
> > evidently the effects of this issue are quite subtle in practice.
> 
> That code had not taken effect for most cases (especially when there's
> no partially visible row at the bottom) until I made the following
> fix:
> 
> 2011-05-21  YAMAMOTO Mitsuharu  <mituharu@math.s.chiba-u.ac.jp>
> 
> 	* dispnew.c (scrolling_window): Don't exclude the case that the
> 	last enabled row in the desired matrix touches the bottom boundary.

Yes, I've seen that.  What problem was it supposed to fix?  Is there a
test case?

Anyway, that was half a year ago, a long time by any measure.

Re: Truncating scroll runs that copy to where we copied to

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 22 Nov 2011 03:25:03 -0500, Eli Zaretskii <eliz@gnu.org> said:

>> 2011-05-21 YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
>> 
>> * dispnew.c (scrolling_window): Don't exclude the case that the
>> last enabled row in the desired matrix touches the bottom boundary.

> Yes, I've seen that.  What problem was it supposed to fix?  Is there
> a test case?

As I told, the problem was

>> That code had not taken effect for most cases (especially when
>> there's no partially visible row at the bottom)

A test case is the same one as what I gave in this thread: with Emacs
23.3a, which doesn't include the above change, you don't observe the
display bug, but instead you can observe that scrolling_window returns
early at `return 0;', though it can reuse the existing display
contents.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Truncating scroll runs that copy to where we copied to

[Eli Zaretskii]

> Date: Tue, 22 Nov 2011 16:26:41 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: David Reitter <david.reitter@gmail.com>,
> 	emacs-devel@gnu.org
> 
> Actually, I told what was wrong with the current code in my first
> post:

I was too stupid to glean the details from that, sorry.  The details
you provided now were the missing link, thanks.

> 	/* Assign matrix rows.  */
> 	for (j = 0; j < r->nrows; ++j)
> 	  {
> 	    struct glyph_row *from, *to;
> 	    int to_overlapped_p;
> 
> 	    to = MATRIX_ROW (current_matrix, r->desired_vpos + j);
> 	    from = MATRIX_ROW (desired_matrix, r->desired_vpos + j);
> 	    to_overlapped_p = to->overlapped_p;
> 	    from->redraw_fringe_bitmaps_p = from->fringe_bitmap_periodic_p;
> 	    assign_row (to, from);
> 	    to->enabled_p = 1, from->enabled_p = 0;
> 	    to->overlapped_p = to_overlapped_p;
> 	  }
> 
> If there's an overlap (I don't mean the variable to_overlapped_p) in
> the copy destinations of two runs, then `from' row in the desired
> matrix becomes a bogus one that had been in the current matrix after
> processing the first run (because assign_row actually does swap), and
> that row is disabled by `from->enabled_p = 0'.  So, when processing
> the second run, the `from' row is now the previously disabled bogus
> row and it is assigned to a row in the current matrix.

Are you saying that this loop was implemented under the assumption
that there's no overlap in the destinations?

Anyway, if the problem is that assign_row leaves the `from' row with
bogus glyph information (and I know it does, as I recently fixed an
assertion violation caused by that), then isn't the problem in
assign_row, to be fixed there?  Assignment as a concept does not imply
a change to the source in any way, so having a function called
assign_row that actually destroys the source means people will (and
do) introduce bugs when they use their mental model of assignment.

Alternatively, maybe we should assign only those rows that have their
enabled_p flag set?  Why would we even want to copy disabled glyph
rows?

Your changes are not here, but elsewhere, which makes me bother if we
are not dancing around the bug and sweeping the root cause under a
thick carpet.

Also, what about the unconditional setting of to->enabled_p to 1 in
the above loop, regardless of what was that flag in `from'?  Does it
look right to you?

Re: Truncating scroll runs that copy to where we copied to

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 22 Nov 2011 03:52:30 -0500, Eli Zaretskii <eliz@gnu.org> said:

> Are you saying that this loop was implemented under the assumption
> that there's no overlap in the destinations?

Of course, I don't know the intention of the original author, but
overlaps mean not only buggy behavior, but also redundant graphics
operations.  That's why I said "Such truncation can also avoid
unnecessary copy in the actual graphics operation." in my first post.

> Anyway, if the problem is that assign_row leaves the `from' row with
> bogus glyph information (and I know it does, as I recently fixed an
> assertion violation caused by that), then isn't the problem in
> assign_row, to be fixed there?  Assignment as a concept does not
> imply a change to the source in any way, so having a function called
> assign_row that actually destroys the source means people will (and
> do) introduce bugs when they use their mental model of assignment.

> Alternatively, maybe we should assign only those rows that have
> their enabled_p flag set?  Why would we even want to copy disabled
> glyph rows?

This alternative is actually what I tried first.  But I thought
truncation could also avoid redundant graphics operations as explained
above.  Actually, I thought that if I posted this alternative, then I
would receive the other-way-round comment, i.e., why not truncate
overlaps?

> Your changes are not here, but elsewhere, which makes me bother if
> we are not dancing around the bug and sweeping the root cause under
> a thick carpet.

> Also, what about the unconditional setting of to->enabled_p to 1 in
> the above loop, regardless of what was that flag in `from'?  Does it
> look right to you?

In the current code, to->enabled_p seems already be set to 1
unconditionally.  Maybe I don't understand the question.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Truncating scroll runs that copy to where we copied to

[Eli Zaretskii]

> Date: Tue, 22 Nov 2011 18:09:55 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: david.reitter@gmail.com,
> 	emacs-devel@gnu.org
> 
> > Alternatively, maybe we should assign only those rows that have
> > their enabled_p flag set?  Why would we even want to copy disabled
> > glyph rows?
> 
> This alternative is actually what I tried first.  But I thought
> truncation could also avoid redundant graphics operations as explained
> above.

If truncation indirectly prevents copying of disabled rows, then I
think at least a comment to that effect should be in the loop which
"assigns the rows".  Just looking at the loop, it is not at all
apparent that only enabled rows are being assigned.

> > Also, what about the unconditional setting of to->enabled_p to 1 in
> > the above loop, regardless of what was that flag in `from'?  Does it
> > look right to you?
> 
> In the current code, to->enabled_p seems already be set to 1
> unconditionally.  Maybe I don't understand the question.

The question was whether the current code is right when it
unconditionally sets that flag.  If the `from' row is disabled, why
should its assignee `to' row be enabled?  If, after your changes, a
disabled row is never assigned, then `to' will already have its
enabled_p flag set, by virtue of the assignment.  Either way, setting
this flag unconditionally after the call to assign_row looks bogus to
me.  (By contrast, resetting the flag in `from' looks like TRT, at
least as long as we garble it.)

Re: Truncating scroll runs that copy to where we copied to

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 22 Nov 2011 04:54:28 -0500, Eli Zaretskii <eliz@gnu.org> said:

> If truncation indirectly prevents copying of disabled rows, then I
> think at least a comment to that effect should be in the loop which
> "assigns the rows".  Just looking at the loop, it is not at all
> apparent that only enabled rows are being assigned.

Maybe we can add an assertion as well as such a comment.

>> > Also, what about the unconditional setting of to->enabled_p to 1
>> > in the above loop, regardless of what was that flag in `from'?
>> > Does it look right to you?
>> 
>> In the current code, to->enabled_p seems already be set to 1
>> unconditionally.  Maybe I don't understand the question.

> The question was whether the current code is right when it
> unconditionally sets that flag.  If the `from' row is disabled, why
> should its assignee `to' row be enabled?  If, after your changes, a
> disabled row is never assigned, then `to' will already have its
> enabled_p flag set, by virtue of the assignment.  Either way,
> setting this flag unconditionally after the call to assign_row looks
> bogus to me.  (By contrast, resetting the flag in `from' looks like
> TRT, at least as long as we garble it.)

Ah, I misunderstood that you were proposing setting to->enabled_p to 1
unconditionally.  Yes, maybe we can replace this assignment with an
assertion.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Truncating scroll runs that copy to where we copied to

[Eli Zaretskii]

> Date: Wed, 23 Nov 2011 09:41:18 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: david.reitter@gmail.com,
> 	emacs-devel@gnu.org
> 
> >>>>> On Tue, 22 Nov 2011 04:54:28 -0500, Eli Zaretskii <eliz@gnu.org> said:
> 
> > If truncation indirectly prevents copying of disabled rows, then I
> > think at least a comment to that effect should be in the loop which
> > "assigns the rows".  Just looking at the loop, it is not at all
> > apparent that only enabled rows are being assigned.
> 
> Maybe we can add an assertion as well as such a comment.

Yes, let's do that.

> > The question was whether the current code is right when it
> > unconditionally sets that flag.  If the `from' row is disabled, why
> > should its assignee `to' row be enabled?  If, after your changes, a
> > disabled row is never assigned, then `to' will already have its
> > enabled_p flag set, by virtue of the assignment.  Either way,
> > setting this flag unconditionally after the call to assign_row looks
> > bogus to me.  (By contrast, resetting the flag in `from' looks like
> > TRT, at least as long as we garble it.)
> 
> Ah, I misunderstood that you were proposing setting to->enabled_p to 1
> unconditionally.  Yes, maybe we can replace this assignment with an
> assertion.

Let's do that as well.

Otherwise, I think your changes are fine, thanks.  At least I cannot
find anything wrong with them.  Please go ahead and install them.

Re: Truncating scroll runs that copy to where we copied to

[YAMAMOTO Mitsuharu]

>>>>> On Sat, 26 Nov 2011 14:44:02 +0200, Eli Zaretskii <eliz@gnu.org> said:

> Otherwise, I think your changes are fine, thanks.  At least I cannot
> find anything wrong with them.  Please go ahead and install them.

Done.  Thanks for your comments.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp