On 5/25/07, Dov Feldstern <[EMAIL PROTECTED]> wrote:
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.
That's what Einstein said. "Documentation is more important than good
coding". Someone *MUST* set up a decent overall documentation of the
Lyx code. And as Dr. Seuss said.
"When our mother went
Down to the town for the day,
She said, "Somebody has to
Clean all this away.
Somebody,
SOMEBODY
Has to, you see."
Then she picked out two somebodies.
Sally and me."
Unfortunately I can see no cat-in-the-hat coming to the rescue in this case...
I'll start documentation in the Wiki and will spam the list upon any
significant achievement, so that you'll be able to ignore my pleas to
check it's validity. Inevitebly I'll err, and thus will produce
misdocumentation which would take hours to understand that the
documentation is plainly wrong.
That would be unfortunate, but setting such a high entery limit for
project contributors would assure us that we'll be getting only the
best of breed developers.
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.
Very true. My bad. Apologies.
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?
I don't know if there is, but it certainly doesn't seem bad. It's as
fast as comparing two pointers.
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.
Thanks for the OK. But I wonder if you didn't have a commit access
eventually, what was all this monstrous thread about?
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?
Sure thing! Let's just hope that someone will authorize it.
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);
