Elazar Leibovich wrote:
This patch solves completely the issue of wrong display of the cursor
when it is right in front of an RTL set.
What I did is I added a better test for that. I tested whether or not
the cursor slice (which I don't understand EXACTLY what it means, it's
sort of pointer to a place in an inset) in the top of the cursor's
stack (ie, the cursor slice the cursor is in now) is equal to the
cursor slice we're getting as an input (which means the place we
should calculate the coordinate of). Only if it is not equal (ie, the
cursor is actually in the inset, and we need to calculate the
coordinates of a place outside the inset) we will apply the fix.
Dov - care to get another developer to agree and check it in?
Great, Elazar! It works, and I now understand what the problem was and
why this patch fixes it, and it makes sense.
Two comments:
1) Regarding the comments: IMO, in-code comments should explain why
something is being done, but not relate to why something is being
changed from the previous state of the code: when a developer comes and
reads the code, she doesn't care why it was *changed* (and she doesn't
necessarily know what it used to look like), she cares only why it's
doing what it's doing. Comments which explain why a change is being made
are important, but they belong to the commit log, as they relate to the
changeset, more than to the code itself.
2) I can't say about the actual method used for determining whether
we're inside the cursor. Andre', JMarc --- does this seem OK to you? Is
there a better way to achieve this?
Anyhow, I don't have commit access, so I can't commit. But the concept
of this patch has certainly got my OK, and I'd like to see it go in.
I'm attaching a patch with a revised comment, based on what I said
above, and on my understanding of the situation. Elazar, is it okay with
you?
Dov
Index: src/Text.cpp
===================================================================
--- src/Text.cpp (revision 18497)
+++ src/Text.cpp (working copy)
@@ -1696,8 +1696,25 @@
// edge (bidi!) -- MV
if (sl.pos() < par.size()) {
font = getFont(buffer, par, sl.pos());
+ // The cursor position inside an inset is expected to be
+ // relative to the inset's left edge. In RTL paragraphs,
+ // however, we currently have the position relative to the
+ // inset's *right* edge. We must explicitly correct the x
+ // position, by removing the width of the inset itself.
+ //
+ // Naturally, this correction should only take place *inside*
+ // the inset. To test whether we are actually inside the inset
+ // (and not only "at" the inset; i.e., at the position at
+ // which the inset is located in the surrounding paragraph,
+ // but not yet inside the inset), we compare the cursor slice
+ // stored at the cursor, to the cursor slice provided as a
+ // parameter. If they are equal, that means we're not in the
+ // inset which is at character sl.pos() in the pargraph, but
+ // we're right before it. Otherwise, it means that the inset
+ // was pushed onto the cursor's stack, and we must be inside.
if (!boundary && font.isVisibleRightToLeft()
- && par.isInset(sl.pos()))
+ && par.isInset(sl.pos())
+ && (&bv.cursor().top())!=&sl)
x -= par.getInset(sl.pos())->width();
}
return int(x);
