Re: "... the window start at a meaningless point within a line."

[Eli Zaretskii <eliz@gnu.org>]

> Date: Thu, 15 Oct 2015 18:16:43 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> As expected, it has been more difficult than expected.

It always is.

> I suspect I still have to modify one or two other primitives, such
> as `compute-motion'.

Why do you need to touch compute-motion?  I don't think it's used in
your scenarios.

> I have a working proof of concept in a patch below.  It is not ready for
> committing for several reasons.  One is that I haven't fully grasped what
> SAVE_IT and RESTORE_IT do with the third parameter (bidi data, I think),
> so I've just used SAVE_IT everywhere without much thought.

That argument is where the data is saved and restored.

> --- a/src/dispextern.h
> +++ b/src/dispextern.h
> @@ -2679,6 +2679,29 @@ struct it
>        inhibit_free_realized_faces = true;		\
>    } while (false)
>  
> +/* SAVE_IT and RESTORE_IT are called when we save a snapshot of the
> +   iterator state and later restore it.  This is needed because the
> +   bidi iterator on bidi.c keeps a stacked cache of its states, which
> +   is really a singleton.  When we use scratch iterator objects to
> +   move around the buffer, we can cause the bidi cache to be pushed or
> +   popped, and therefore we need to restore the cache state when we
> +   return to the original iterator.  */
> +#define SAVE_IT(ITCOPY, ITORIG, CACHE)		\
> +  do {						\
> +    if (CACHE)					\
> +      bidi_unshelve_cache (CACHE, true);	\
> +    ITCOPY = ITORIG;				\
> +    CACHE = bidi_shelve_cache ();		\
> +  } while (false)
> +
> +#define RESTORE_IT(pITORIG, pITCOPY, CACHE)	\
> +  do {						\
> +    if (pITORIG != pITCOPY)			\
> +      *(pITORIG) = *(pITCOPY);			\
> +    bidi_unshelve_cache (CACHE, false);		\
> +    CACHE = NULL;				\
> +  } while (false)
> +

I'd prefer these to stay private to xdisp.c.  If you really need them
elsewhere (I don't think so), there are alternative solutions.

> +/* Get the end of the text line on which POS is. */
> +static ptrdiff_t
> +pos_line_end_position (struct text_pos *pos)
> +{
> +  ptrdiff_t pt = PT, pt_byte = PT_BYTE;
> +  Lisp_Object eol;
> +
> +  SET_PT_BOTH (pos->charpos, pos->bytepos);
> +  eol = Fline_end_position (Qnil);
> +  SET_PT_BOTH (pt, pt_byte);
> +  return XINT (eol);
> +}

There are much easier ways to get the end of the line: e.g., scan from
POS for a newline.

> +  if (beginning < L0_phys_EOL
> +      && end >= ws)

Sorry, but this is a non-starter: you cannot compare character
positions with < and >, assuming characters are displayed on a line in
strict logical order.  These comparisons will completely break in
bidirectional text, where character positions change non-linearly with
screen coordinates.

> +      if (IT_CHARPOS (*it) < ws)
> +	{
> +	  below = false;	/* exact_it is already past `it'. */
> +	  SAVE_IT (post_exact_it, exact_it, cache);
> +	}

Same here.

> +      while (1)
> +	{
> +	  if (IT_CHARPOS (exact_it) < IT_CHARPOS (*it))
> +	    SAVE_IT (pre_exact_it, exact_it, cache);
> +	  if (!forward_to_next_display_line_start (&exact_it))
> +	    break;	/* protection against infinite looping. */
> +	  if (below
> +	      && IT_CHARPOS (exact_it) >= IT_CHARPOS (*it))
> +	    {
> +	      below = false;
> +	      SAVE_IT (post_exact_it, exact_it, cache);
> +	    }
> +	  if (IT_CHARPOS (exact_it) > end)
> +	    break;

And here.  And everywhere else.

> +	  if (IT_CHARPOS (exact_it) == IT_CHARPOS (xdisp_it))

Comparisons with == are OK.

I also suspect you don't need all those calls to SAVE_IT all over the
place.  In any case, you are leaking memory here, because there's not
a single RESTORE_IT to free the memory that SAVE_IT allocates.

Can you describe the algorithm of this function in English?  Then
perhaps I could help you rewrite it in bidi-aware way.