Dov Feldstern wrote:
Dov Feldstern wrote:
Dov Feldstern wrote:
Fixed. Everything seems to be working fine now. I would still
appreciate it if someone could review the whole patch carefully,
because this does affect general code (not just RTL).
Not everything: I just ran into a new crash which was introduced by my
patch (although this time it'll only be apparent, I think, in documents
which are truly bidi):
*) start a new document (or new paragraph). At some point, switch the
language to an RTL language. Now, the boundary is true. you can continue
typing, but as long as the cursor is at the tip, backspace will cause a
crash.
Something weird is going on at that last space, because you're
perpetually on a boundary there: if you move back with an arrow, the
first time you move back the cursor won't actually move --- all that
will change is that the boundary will become false. I think this is the
real problem: the boundary should never be "where the cursor is", which
is what's happening here. Rather, boundary is just something the cursor
sometimes "moves through". If this is the case, then in truth my patch
has actually only exposed something wrong which is already going on. ;)
Anyhow, I'll try to take a look at this tomorrow night (or if anyone
else wants to take a stab at this in the meantime...)
Dov
Okay, here's tonight's fix. Nothing fancy, I just made sure that what
was causing the crash doesn't happen, but I don't understand exactly why
it works. All I changed was to add the condition "&& pos <
cur.lastpos()" in setCursorFont.
So it seems to work, I haven't run into any crashes yet, and it solves
the crashes that we know about so far. There's still something a bit off
going on when typing in the opposite language at the tip of a paragraph
--- boundary is always true (which makes a certain amount of sense, I
guess), and the first time you move back (with LEFT in LTR or RIGHT in
RTL --- both of which call cursorLeft), it doesn't move back, but just
resets the boundary to false. Subsequent keypresses work as normal.
I would be happy for this to go on, just to get it well tested (and of
course, it does solve a crash ;) ) --- it would be better to have it
tested in beta than in a final release. I would also be *very* happy if
anyone who understand the internals of the cursor movements please
review this, and see if it makes sense. Maybe you can spot what's still
not quite right.
Again, if this doesn't go in, RTL should be turned off, because that
*will* cause crashes without this!
Thanks!
Dov
Index: src/Text2.cpp
===================================================================
--- src/Text2.cpp (revision 18314)
+++ src/Text2.cpp (working copy)
@@ -717,8 +717,12 @@
pos_type pos = cur.pos();
Paragraph & par = cur.paragraph();
- if (cur.boundary() && pos > 0)
+ if (cur.boundary() && pos > 0 && pos < cur.lastpos()) {
--pos;
+ // We may have just moved to the previous row ---
+ // we're going to be needing its bidi tables!
+ bidi.computeTables(par, cur.buffer(), cur.textRow());
+ }
if (pos > 0) {
if (pos == cur.lastpos())
@@ -904,9 +908,18 @@
return false;
if (cur.pos() == cur.lastpos())
return false;
- Inset * inset = cur.nextInset();
+ Inset * inset = front ? cur.nextInset() : cur.prevInset();
if (!isHighlyEditableInset(inset))
return false;
+ /*
+ * Apparently, when entering an inset we are expected to be positioned
+ * *before* it in the containing paragraph, regardless of the direction
+ * from which we are entering. Otherwise, cursor placement goes awry,
+ * and when we exit from the beginning, we'll be placed *after* the
+ * inset.
+ */
+ if (!front)
+ --cur.pos();
inset->edit(cur, front);
return true;
}
@@ -917,27 +930,28 @@
// Tell BufferView to test for FitCursor in any case!
cur.updateFlags(Update::FitCursor);
- if (!cur.boundary() && cur.pos() > 0 &&
- cur.textRow().pos() == cur.pos() &&
- !cur.paragraph().isLineSeparator(cur.pos()-1) &&
- !cur.paragraph().isNewline(cur.pos()-1)) {
- return setCursor(cur, cur.pit(), cur.pos(), true, true);
- }
- if (cur.pos() != 0) {
- bool updateNeeded = setCursor(cur, cur.pit(), cur.pos() - 1,
true, false);
+ if (cur.pos() > 0) {
+ if (cur.boundary())
+ return setCursor(cur, cur.pit(), cur.pos(), true,
false);
+
+ bool updateNeeded = false;
+ // If checkAndActivateInset returns true, that means that
+ // the cursor was placed inside it, so we're done
if (!checkAndActivateInset(cur, false)) {
- /** FIXME: What's this cause purpose???
- bool boundary = cur.boundary();
- if (false && !boundary &&
- bidi.isBoundary(cur.buffer(), cur.paragraph(),
cur.pos() + 1))
- updateNeeded |=
- setCursor(cur, cur.pit(), cur.pos() +
1, true, true);
- */
+ if (!cur.boundary() &&
+ cur.textRow().pos() == cur.pos() /*&&
+ !cur.paragraph().isLineSeparator(cur.pos()-1) &&
+ !cur.paragraph().isNewline(cur.pos() - 1)*/) {
+ updateNeeded |= setCursor(cur, cur.pit(),
cur.pos(),
+
true, true);
+ }
+ updateNeeded |= setCursor(cur, cur.pit(),cur.pos() - 1,
+ true,
false);
}
return updateNeeded;
}
- if (cur.pit() != 0) {
+ if (cur.pit() > 0) {
// Steps into the paragraph above
return setCursor(cur, cur.pit() - 1, getPar(cur.pit() -
1).size());
}
@@ -956,6 +970,8 @@
true, false);
bool updateNeeded = false;
+ // If checkAndActivateInset returns true, that means that
+ // the cursor was placed inside it, so we're done
if (!checkAndActivateInset(cur, true)) {
if (cur.textRow().endpos() == cur.pos() + 1 &&
cur.textRow().endpos() != cur.lastpos() &&
@@ -964,9 +980,6 @@
cur.boundary(true);
}
updateNeeded |= setCursor(cur, cur.pit(), cur.pos() +
1, true, cur.boundary());
- if (false && bidi.isBoundary(cur.buffer(),
cur.paragraph(),
- cur.pos()))
- updateNeeded |= setCursor(cur, cur.pit(),
cur.pos(), true, true);
}
return updateNeeded;
}
