Stefan Schimanski wrote:
Hi!
I cleaned up the patches to prepare them for committing: Again there are
three, which are half depending on each other. Together the RTL behavior
is correct (a lyx2lyx patch is still missing to convert LyX 1.3's
"wrong" encoding of RTL paragraphs into the new one).
* rtlboundary.patch: it fixes the position of the cursor on RTL<->LTR
boundaries. Instead of adding the rtl parameter to cursorPos, the RTL
property is queried directly in coordOffset where it is needed. So much
cleaner and shorter than the previous attempts. (fix for
http://bugzilla.lyx.org/show_bug.cgi?id=3754: cursor movement at RTL-LTR
boundary)
Three things:
1) In Text2.cpp, in the condition "if (cur.boundary() &&
!bidi.isBoundary...", it should be:
return setCursor(cur, cur.pit(), cur.pos(), true, false);
and not, as you have:
return setCursor(cur, cur.pit(), cur.pos() + 1, true, false);
(the + 1 is wrong; see what happens at the end of a line)
2) When committing, I would commit the changes in Text2.cpp separately
from the other changes: the changes in Text2.cpp solve bug 3754, the
other changes solve issue (3) of bug 3551; and the two are independent.
3) Spacing (some lines are too long)
Attached are the two separate patches, with Text2.cpp in its corrected
(for the + 1, and shorter lines) form.
@JMarc: I incorporated your suggestions. But I kept the isRTL functions
in the Text class. This can be repaired in another patch easily, but
that would touch several files and I don't want to hide the real
contributions of the patch behind it.
+1
* rtlselection.patch: Basically the same as
http://permalink.gmane.org/gmane.editors.lyx.devel/86105 (fix
http://bugzilla.lyx.org/show_bug.cgi?id=3550: selection in bidi)
Good.
* rtlspaces.patch: There is a inconsistency between the font of spaces
at RTL<->LTR boundaries. The whole space problem is discussed in
http://permalink.gmane.org/gmane.editors.lyx.devel/80783. An
illustration of the current situation and the one after this patch are
visible at http://article.gmane.org/gmane.editors.lyx.devel/86342 in the
second picture. A discussion of the inconsistency can be found in
http://article.gmane.org/gmane.editors.lyx.devel/86425. (fixes
http://bugzilla.lyx.org/show_bug.cgi?id=3789). Dov had sent a very good
in-depth discussion and comparison to LyX 1.3 at
http://permalink.gmane.org/gmane.editors.lyx.devel/80783.
If somebody has an opinion about this issue, please speak up. I think
the version implemented by this patch is simple on the one hand, but
also implements the behavior of Latex. And I think it is not too bad
that it does not hide anything from the user or is doing magic he cannot
understand.
*I* like this, but I foresee a lot of resistance from users. Let's see
what other's say, I'll still try to ask about it on the users' list.
Meanwhile, the other patches shouldn't wait for this one --- they should
be committed ASAP.
Thanks Stefan!
Dov
Here there is the in fact simple patch:
Index: src/Bidi.cpp
===================================================================
--- src/Bidi.cpp (Revision 18626)
+++ src/Bidi.cpp (Arbeitskopie)
@@ -95,7 +95,11 @@
pos_type const body_pos = par.beginOfBody();
for (pos_type lpos = start_; lpos <= end_; ++lpos) {
- bool is_space = par.isLineSeparator(lpos);
+ bool is_space = false;
+ // We do not handle spaces around an RTL segment in a special
way anymore.
+ // Neither does LaTeX, so setting is_spacs to false makes the
view in LyX
+ // consistent with the output of LaTeX later. The old setting was:
+ // is_space = par.isLineSeparator(lpos);
pos_type const pos =
(is_space && lpos + 1 <= end_ &&
!par.isLineSeparator(lpos + 1) &&
Stefan
So all of these patches:
*) http://permalink.gmane.org/gmane.editors.lyx.devel/85956 (fix for
bug 3754: cursor movement at RTL-LTR boundary)
*) http://permalink.gmane.org/gmane.editors.lyx.devel/86192 (this fix,
for issue (3) in bug 3551 --- except note that this also includes the
above patch, they should be separated, as that one is independent of
this one)
*) http://permalink.gmane.org/gmane.editors.lyx.devel/86105 (fix for
bug 3550: selection in bidi)
are OK with me, and should be applied (in the above order).
Thanks for all your work on this, Stefan!
Dov
Dov Feldstern wrote:
Stefan Schimanski wrote:
As promised, here it is. This is the way to do it. It includes our
former patch for cursorLeft/Right to avoid boundary magic on
RTL-boundaries.
I don't understand what I'm supposed to apply this patch against.
@Dov: For the example where your patch was wrong, make a selection,
starting in an RTL paragraph, ending in a LTR paragraph. One of the
two sides will "see" the correct RTL setting by looking at the
buffer's cursor. The other one (the cursorX call for the anchor)
will also look there, but the information in the buffer's cursor has
no relation to the that position.
Stefan
I don't totally follow your explanation, but the fact is that the end
behavior is still correct (using Elazar's patch) in the case you
describe. OTOH, the selection patch from yesterday is *not* behaving
correctly.
To illustrate (with Elazar's patch for cursor movement, your patch
from yesterday for selection):
[IHG]FED
When pressing LEFT, cursor moves from after E -> after F -> Before G
(inside footnote). This is correct.
When selecting by SHIFT and pressing LEFT starting after D, selection
shows up as: "E" -> "[IHG]FE". But really, only "FE" is selected (you
can convince yourself of this by deleting). Continuing to select LEFT
again doesn't show any difference in the way the selection is, but
the cursor correctly moves to after the inset, and now everything
really is selected.
I haven't tried your new patch yet (see above), but if it's using the
same logic as the selection, then it may also be wrong... I'll try it
anyway if you'll explain what to apply it against...
Thanks!
Dov
Index: src/Text2.cpp
===================================================================
--- src/Text2.cpp (revision 18634)
+++ src/Text2.cpp (working copy)
@@ -1030,14 +1030,17 @@
// not at paragraph end?
if (cur.pos() != cur.lastpos()) {
- // if left of boundary -> just jump to right side
- if (cur.boundary())
- return setCursor(cur, cur.pit(), cur.pos(), true,
false);
-
// in front of editable inset, i.e. jump into it?
if (checkAndActivateInset(cur, true))
return false;
-
+
+ // if left of boundary -> just jump to right side
+ // but for RTL boundaries don't, because:
+ // abc|DDEEFFghi -> abcDDEEF|Fghi
+ if (cur.boundary() &&
+ !bidi.isBoundary(cur.buffer(), cur.paragraph(),
cur.pos()))
+ return setCursor(cur, cur.pit(), cur.pos(), true,
false);
+
// next position is left of boundary,
// but go to next line for special cases like space, newline,
linesep
#if 0
@@ -1062,6 +1065,11 @@
return setCursor(cur, cur.pit(), cur.pos() + 1, true,
true);
}
+ // in front of RTL boundary? Stay on this side of the boundary
because:
+ // ab|cDDEEFFghi -> abc|DDEEFFghi
+ if (bidi.isBoundary(cur.buffer(), cur.paragraph(), cur.pos() +
1))
+ return setCursor(cur, cur.pit(), cur.pos() + 1, true,
true);
+
// move right
return setCursor(cur, cur.pit(), cur.pos() + 1, true, false);
}
Index: src/Text.cpp
===================================================================
--- src/Text.cpp (revision 18634)
+++ src/Text.cpp (working copy)
@@ -1687,20 +1687,13 @@
}
// see correction above
- if (boundary_correction)
- if (getFont(buffer, par, ppos).isVisibleRightToLeft())
+ if (boundary_correction) {
+ if (isRTL(buffer, sl, boundary))
x -= singleWidth(buffer, par, ppos);
else
x += singleWidth(buffer, par, ppos);
-
- // Make sure inside an inset we always count from the left
- // edge (bidi!) -- MV
- if (sl.pos() < par.size()) {
- font = getFont(buffer, par, sl.pos());
- if (!boundary && font.isVisibleRightToLeft()
- && par.isInset(sl.pos()))
- x -= par.getInset(sl.pos())->width();
}
+
return int(x);
}
Index: src/Text3.cpp
===================================================================
--- src/Text3.cpp (revision 18634)
+++ src/Text3.cpp (working copy)
@@ -299,6 +299,20 @@
}
+bool Text::isRTL(Buffer const & buffer, CursorSlice const & sl, bool boundary)
const
+{
+ if (!sl.text())
+ return false;
+
+ int correction = 0;
+ if (boundary && sl.pos() > 0)
+ correction = -1;
+
+ Paragraph const & par = getPar(sl.pit());
+ return getFont(buffer, par, sl.pos() +
correction).isVisibleRightToLeft();
+}
+
+
void Text::dispatch(Cursor & cur, FuncRequest & cmd)
{
LYXERR(Debug::ACTION) << "Text::dispatch: cmd: " << cmd << endl;
Index: src/bufferview_funcs.cpp
===================================================================
--- src/bufferview_funcs.cpp (revision 18634)
+++ src/bufferview_funcs.cpp (working copy)
@@ -161,15 +161,34 @@
{
int x = 0;
int y = 0;
+ int lastw = 0;
- // Contribution of nested insets
- for (size_t i = 1; i != dit.depth(); ++i) {
+ // Addup ontribution of nested insets, from inside to outside,
+ // keeping the outer paragraph for a special handling below
+ for (size_t i = dit.depth() - 1; i >= 1; --i) {
CursorSlice const & sl = dit[i];
int xx = 0;
int yy = 0;
+
+ // get relative position inside sl.inset()
sl.inset().cursorPos(bv, sl, boundary && ((i+1) ==
dit.depth()), xx, yy);
+
+ // Make relative position inside of the edited inset relative
to sl.inset()
x += xx;
y += yy;
+
+ // In case of an RTL inset, the edited inset will be positioned
to the left
+ // of xx:yy
+ if (sl.text()) {
+ bool boundary_i = boundary && i + 1 == dit.depth();
+ bool rtl = sl.text()->isRTL(*bv.buffer(), sl,
boundary_i);
+ if (rtl)
+ x -= lastw;
+ }
+
+ // remember width for the case that sl.inset() is positioned in
an RTL inset
+ lastw = sl.inset().width();
+
//lyxerr << "Cursor::getPos, i: "
// << i << " x: " << xx << " y: " << y << endl;
}
@@ -180,6 +199,7 @@
BOOST_ASSERT(!pm.rows().empty());
y -= pm.rows()[0].ascent();
#if 1
+ // FIXME: document this mess
size_t rend;
if (sl.pos() > 0 && dit.depth() == 1) {
int pos = sl.pos();
@@ -195,8 +215,18 @@
for (size_t rit = 0; rit != rend; ++rit)
y += pm.rows()[rit].height();
y += pm.rows()[rend].ascent();
- x += dit.bottom().text()->cursorX(bv, dit.bottom(), boundary &&
dit.depth() == 1);
-
+
+ // Make relative position from the nested inset now bufferview absolute.
+ int xx = dit.bottom().text()->cursorX(bv, dit.bottom(), boundary &&
dit.depth() == 1);
+ x += xx;
+
+ // In the RTL case place the nested inset at the left of the cursor in
+ // the outer paragraph
+ bool boundary_1 = boundary && 1 == dit.depth();
+ bool rtl = dit.bottom().text()->isRTL(*bv.buffer(), dit.bottom(),
boundary_1);
+ if (rtl)
+ x -= lastw;
+
return Point(x, y);
}
Index: src/Text.h
===================================================================
--- src/Text.h (revision 18634)
+++ src/Text.h (working copy)
@@ -333,6 +333,8 @@
docstring getPossibleLabel(Cursor & cur) const;
/// is this paragraph right-to-left?
bool isRTL(Buffer const &, Paragraph const & par) const;
+ /// is this position in the paragraph right-to-left?
+ bool isRTL(Buffer const & buffer, CursorSlice const & sl, bool
boundary) const;
///
bool checkAndActivateInset(Cursor & cur, bool front);
