From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: "... the window start at a meaningless point within a line." Date: Thu, 15 Oct 2015 23:14:39 +0300 Message-ID: <834mhr99e8.fsf@gnu.org> References: <20150930204513.GA3174@acm.fritz.box> <83mvw39dp3.fsf@gnu.org> <20151001094138.GA2515@acm.fritz.box> <83h9maao7w.fsf@gnu.org> <20151001110204.GB2515@acm.fritz.box> <83egheaj9e.fsf@gnu.org> <20151015181642.GA6467@acm.fritz.box> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: ger.gmane.org 1444940140 22368 80.91.229.3 (15 Oct 2015 20:15:40 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 15 Oct 2015 20:15:40 +0000 (UTC) Cc: emacs-devel@gnu.org To: Alan Mackenzie Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Oct 15 22:15:31 2015 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Zmovv-0001P1-L6 for ged-emacs-devel@m.gmane.org; Thu, 15 Oct 2015 22:15:27 +0200 Original-Received: from localhost ([::1]:49467 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zmovv-0007ev-4A for ged-emacs-devel@m.gmane.org; Thu, 15 Oct 2015 16:15:27 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:53094) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmovF-0007XB-DM for emacs-devel@gnu.org; Thu, 15 Oct 2015 16:14:46 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZmovB-00017S-By for emacs-devel@gnu.org; Thu, 15 Oct 2015 16:14:45 -0400 Original-Received: from mtaout23.012.net.il ([80.179.55.175]:48507) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmovB-00016C-3n for emacs-devel@gnu.org; Thu, 15 Oct 2015 16:14:41 -0400 Original-Received: from conversion-daemon.a-mtaout23.012.net.il by a-mtaout23.012.net.il (HyperSendmail v2007.08) id <0NWA004002PAXZ00@a-mtaout23.012.net.il> for emacs-devel@gnu.org; Thu, 15 Oct 2015 23:14:39 +0300 (IDT) Original-Received: from HOME-C4E4A596F7 ([84.94.185.246]) by a-mtaout23.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0NWA004TP2WEXB10@a-mtaout23.012.net.il>; Thu, 15 Oct 2015 23:14:39 +0300 (IDT) In-reply-to: <20151015181642.GA6467@acm.fritz.box> X-012-Sender: halo1@inter.net.il X-detected-operating-system: by eggs.gnu.org: Solaris 10 X-Received-From: 80.179.55.175 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:191682 Archived-At: > Date: Thu, 15 Oct 2015 18:16:43 +0000 > Cc: emacs-devel@gnu.org > From: Alan Mackenzie > > 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.