Re: Allowing point to be outside the window?

[Po Lu <luangruo@yahoo.com>]

Eli Zaretskii <eliz@gnu.org> writes:

> This "allows point to be outside a window" in what situations?

If you move point out of the visible area of the window (such as with
`goto-char') or if you set the window start but do not force it.

> In general, it's hard to provide useful feedback without understanding
> the main ideas of the changes.  And you didn't add any comments to
> help in that.  So I'm left to wonder whether some changes I don't
> think I understand are just my misunderstanding of your ideas, or your
> misunderstanding of the code.

I will work on that, sorry.

> So just a few random thoughts I had while reading the changes:

> Shouldn't set_cursor_from_row be disabled/bypassed in this mode?
>
> And I don't understand the dance with the cursor_visible_p flag: why
> do you reset it at entry and set back before exiting?  Isn't the value
> of w->cursor enough to tell whether the cursor position was computed
> or not?

The idea is that if `set_cursor_from_row' returns false (say if the
cursor was not found to be in the row), w->cursor_visible will also be
set to false.

I don't understand why set_cursor_from_row should be disabled though:
isn't it responsible for placing the cursor on the display (and as such,
isn't it required for the cursor to be displayed when point is _inside_
the window?).

>> +  if (!keep_point_visible)
>> +    goto maybe_try_window;
>> +
>>   try_to_scroll:

> I don't understand this change.  Are you forcibly trying to avoid
> calling try_scrolling?  If so, why?  That function is only loosely
> related to scrolling commands; in fact, if you type C-v in "emacs -Q",
> you will never see it called.  This function is an optimization of a
> window's redisplay, and that is needed even in this new mode you are
> implementing.

Doesn't `try_scrolling' try to constrain point inside if it is too near
the end of the window?  (And isn't the optimization that tries to decide
whether or not to blit the display if appropriate `scrolling_window' in
dispextern.c?)

>> + maybe_try_window:
>> +
>> +  /* Set the window start position here explicitly if it is outside
>> +     the accessible portion of the buffer.  */
>> +
>> +  if (CHARPOS (startp) < BEGV
>> +      || CHARPOS (startp) > ZV)
>> +    {
>> +      if (CHARPOS (startp) < BEGV)
>> +	set_marker_both (w->start, Qnil, BEGV, BEGV_BYTE);
>> +      else
>> +	set_marker_both (w->start, Qnil, ZV, ZV_BYTE);
>> +
>> +      SET_TEXT_POS_FROM_MARKER (startp, w->start);
>> +
>> +      /* Run scroll hooks.  */
>> +      startp = run_window_scroll_functions (window, startp);
>> +    }
>> +
>> +  /* We invoke try_window and try_window_reusing_current_matrix below,
>> +     and they manipulate the bidi cache.  Save and restore the cache
>> +     state of our iterator, so we could continue using it after that.  */
>> +  itdata = bidi_shelve_cache ();

> This and the rest of this hunk is a copy of code we have elsewhere,
> and I'm not sure I understand why you needed that.

I think I can keep it underneath the recenter label, thanks.

>> @@ -20875,6 +20956,8 @@ #define GIVE_UP(X) return 0
>>  	  row = row_containing_pos (w, PT, r0, NULL, 0);
>>  	  if (row)
>>  	    set_cursor_from_row (w, row, current_matrix, 0, 0, 0, 0);
>> +	  else
>> +	    w->cursor_visible_p = false;
>>  	  return 1;
>>  	}
>>      }
>> @@ -20915,6 +20998,8 @@ #define GIVE_UP(X) return 0
>>  	  row = row_containing_pos (w, PT, r0, NULL, 0);
>>  	  if (row)
>>  	    set_cursor_from_row (w, row, current_matrix, 0, 0, 0, 0);
>> +	  else
>> +	    w->cursor_visible_p = false;
>>  	  return 2;
>>  	}
>>      }
>> @@ -21152,6 +21237,8 @@ #define GIVE_UP(X) return 0
>>  				    last_unchanged_at_beg_row + 1, 0);
>>  	  if (row)
>>  	    set_cursor_from_row (w, row, w->current_matrix, 0, 0, 0, 0);
>> +	  else
>> +	    w->cursor_visible_p = false;
>>  	}
>>  
>>        /* Start from first_unchanged_at_end_row looking for PT.  */
>> @@ -21162,6 +21249,8 @@ #define GIVE_UP(X) return 0
>>  	  if (row)
>>  	    set_cursor_from_row (w, row, w->current_matrix, delta,
>>  				 delta_bytes, dy, dvpos);
>> +	  else
>> +	    w->cursor_visible_p = false;
>>  	}

> Are these really needed? why?

I think that's not needed anymore.  I will delete it.

>> +  if (w->cursor_visible_p)
>> +    {
>> +      int this_scroll_margin = window_scroll_margin (w, MARGIN_IN_PIXELS);
>> +      int cursor_height = MATRIX_ROW (w->desired_matrix, w->cursor.vpos)->height;

> The condition above doesn't check the keep_point_visible flag, why?

w->cursor_visible_p should always be true if keep_point_visible is not
nil by the time we get there.

Thanks.