Re: Allowing point to be outside the window?

[Eli Zaretskii <eliz@gnu.org>]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sat, 04 Dec 2021 19:18:38 +0800
> 
> I haven't figured out most of the user level command things, but here's
> a patch that allows point to be outside a window.
> 
> Some redisplay optimizations are disabled, but I hope to solve that
> soon.
> 
> WDYT?  Thanks.

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

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.

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

> @@ -16989,6 +16991,8 @@ set_cursor_from_row (struct window *w, struct glyph_row *row,
>       comes from a text property, not from an overlay.  */
>    bool string_from_text_prop = false;
>  
> +  w->cursor_visible_p = false;
> +
>    /* Don't even try doing anything if called for a mode-line or
>       header-line or tab-line row, since the rest of the code isn't
>       prepared to deal with such calamities.  */
> @@ -17575,6 +17579,7 @@ set_cursor_from_row (struct window *w, struct glyph_row *row,
>  	CHARPOS (this_line_start_pos) = 0;
>      }
>  
> +  w->cursor_visible_p = true;
>    return true;
>  }

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?

> +  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.

> + 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.

> @@ -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?

> +  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?

Thanks.