[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: "... the window start at a meaningless point within a line."
From: |
Eli Zaretskii |
Subject: |
Re: "... the window start at a meaningless point within a line." |
Date: |
Thu, 15 Oct 2015 23:14:39 +0300 |
> Date: Thu, 15 Oct 2015 18:16:43 +0000
> Cc: address@hidden
> From: Alan Mackenzie <address@hidden>
>
> 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.
- "... the window start at a meaningless point within a line.", Alan Mackenzie, 2015/10/08
- Re: "... the window start at a meaningless point within a line.", Eli Zaretskii, 2015/10/08
- Re: "... the window start at a meaningless point within a line.", Alan Mackenzie, 2015/10/08
- Re: "... the window start at a meaningless point within a line.", Eli Zaretskii, 2015/10/08
- Re: "... the window start at a meaningless point within a line.", Alan Mackenzie, 2015/10/08
- Re: "... the window start at a meaningless point within a line.", Eli Zaretskii, 2015/10/08
- Re: "... the window start at a meaningless point within a line.", Alan Mackenzie, 2015/10/08
- Re: "... the window start at a meaningless point within a line.", Alan Mackenzie, 2015/10/15
- Re: "... the window start at a meaningless point within a line.",
Eli Zaretskii <=
- Re: "... the window start at a meaningless point within a line.", Alan Mackenzie, 2015/10/16
- Re: "... the window start at a meaningless point within a line.", Eli Zaretskii, 2015/10/16
- Re: "... the window start at a meaningless point within a line.", Alan Mackenzie, 2015/10/16
- Re: "... the window start at a meaningless point within a line.", Eli Zaretskii, 2015/10/16
- Re: "... the window start at a meaningless point within a line.", David Kastrup, 2015/10/16
- Re: "... the window start at a meaningless point within a line.", Eli Zaretskii, 2015/10/17
- Re: "... the window start at a meaningless point within a line.", David Kastrup, 2015/10/17
- Re: "... the window start at a meaningless point within a line.", Alan Mackenzie, 2015/10/16
- Re: "... the window start at a meaningless point within a line.", Eli Zaretskii, 2015/10/16
- Re: "... the window start at a meaningless point within a line.", Alan Mackenzie, 2015/10/16