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

[Eli Zaretskii]

> Date: Fri, 16 Oct 2015 09:55:35 +0000
> Cc: address@hidden
> From: Alan Mackenzie <address@hidden>
> 
> > Why do you need to touch compute-motion?  I don't think it's used in
> > your scenarios.
> 
> Because it deals with screen lines and columns.  But looking more closely
> at it, it doesn't use struct it's, or anything like that, so perhaps I
> won't need to do anything to it.

AFAIR, it's only used in batch mode, so should not be relevant.

> > I'd prefer these [SAVE_IT and RESTORE_IT] to stay private to xdisp.c.
> > If you really need them elsewhere (I don't think so), there are
> > alternative solutions.
> 
> I think most of the time, I just need to assign one struct it to another.

Let's postpone discussing this until we have a working code that
doesn't break bidi.

> > > +      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.
> 
> There must be some bidi compatible order operators which express "earlier
> than" etc.  I will be needing these.

Define "earlier than".  Is that on screen, i.e. in the visual order,
or in the buffer, i.e. in the logical order, a.k.a. "the order of
buffer positions"?  These two orders are different for bidirectional
text.

> > 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.
> 
> As I said, straight assignments should do most of the time.

As long as you use functions that move the iterator, you cannot safely
assign one 'struct it' to another.  That's because the iterator object
has a (mostly invisible) companion -- the bidi cache, which must be in
sync with the contents of 'struct it'.

> > Can you describe the algorithm of this function in English?  Then
> > perhaps I could help you rewrite it in bidi-aware way.
> 
> OK, here goes!  It's a bit long winded, but that's because the function
> is little more than an assemblage of special cases.

Thanks, I will study it and see what can I propose.