Stefan Schimanski wrote:
In the last patch there was a bug with the script code in math. Fixed that. So here it is again. While looking through the FINISHED_UP/DOWN stuff I am starting to think that it has no effect whatsoever. I don't create those LFUNs anymore, and still cursor handling seems ok.

Stefan


Way to go, Stefan! I would have been very hesitant about making such a big change, but you seem to have done a good job of it... :) Your patch also fixes a minor problem I still had after I "corrected" cursorLeft last week, so it must be more correct...

I checked this latest version for the RTL behavior, and it seems to work correctly (well, as correctly as it did before your change ;) ).

My only comment regarding the code itself: I would suggest separating the fix for bug 3720 from the rest of the patch here, and applying them separately. They are not logically interdependent (the fix for bug 3720 could be applied independently of the bigger change being made here), so conceptually it's better, I think, to separate them; and the practical implication is that if for some reason someone will want to revert your patch (say some major bug is discovered just before the rc1 release), then that fix will also get reverted by accident, which it needn't. Attached find the separated patches: cursor_left_inset.diff fixes bug 3720, and cursordispatch_no_corsur_left_inset.diff is all the rest (meant to be applied *after* the previous one). Together, they are exactly equivalent to your patch.

Dov
Index: src/Text2.cpp
===================================================================
--- src/Text2.cpp       (revision 18519)
+++ src/Text2.cpp       (working copy)
@@ -954,8 +954,10 @@
 {
        if (cur.selection())
                return false;
-       if (cur.pos() == cur.lastpos())
+       if (front && cur.pos() == cur.lastpos())
                return false;
+       if (!front && cur.pos() == 0)
+               return false;
        Inset * inset = front ? cur.nextInset() : cur.prevInset();
        if (!isHighlyEditableInset(inset))
                return false;
Index: src/Cursor.h
===================================================================
--- src/Cursor.h        (revision 18519)
+++ src/Cursor.h        (working copy)
@@ -121,16 +121,21 @@
        /// insert a string
        void insert(docstring const & str);
 
-       /// in pixels from left of screen
+       /// FIXME: rename to something sensible showing difference to x_target()
+       /// in pixels from left of screen, set to current position if unset
        int targetX() const;
-       /// write acess to target x position of cursor
-       int & x_target();
-       /// return target x position of cursor
+       /// set the targetX to x
+       void setTargetX(int x);
+       /// return targetX or -1 if unset
        int x_target() const;
-       /// set targetX in current position
+       /// set targetX to current position
        void setTargetX();
-       /// clear target x position of cursor
+       /// clear targetX, i.e. set it to -1
        void clearTargetX();
+       /// set offset to actual position - targetX
+       void updateTextTargetOffset();
+       /// distance between actual and targeted position during last up/down 
in text
+       int textTargetOffset() const;
 
        /// access to normalized selection anchor
        CursorSlice anchor() const;
@@ -186,11 +191,10 @@
 //private:
        /// the anchor position
        DocIterator anchor_;
-
+       
        ///
        DispatchResult disp_;
-
-
+       
 private:
        /**
         * The target x position of the cursor. This is used for when
@@ -205,6 +209,8 @@
         * shorter than x()
         */
        int x_target_;
+       /// if a x_target cannot be hit exactly in a text, put the difference 
here
+       int textTargetOffset_;
        /// do we have a selection?
        bool selection_;
        /// are we on the way to get one?
@@ -214,7 +220,13 @@
        // of a big inset spanning a whole row and computing coordinates for
        // displaying the cursor.
        bool logicalpos_;
-
+       /// x position before dispatch started
+       int beforeDispX_;
+       /// y position before dispatch started
+       int beforeDispY_;
+       /// position before dispatch started
+       size_t beforeDispDepth_;
+               
 private:
 
        //
@@ -243,10 +255,14 @@
        bool erase();
        /// return false for empty math insets
        bool backspace();
-       /// called for LFUN_UP etc
+       /// move the cursor up by sending an internal LFUN_UP
        bool up();
-       /// called for LFUN_DOWN etc
+       /// move the cursor up by sending an internal LFUN_DOWN
        bool down();
+       /// move up/down in a text inset, called for LFUN_UP/DOWN
+       bool upDownInText(bool up);
+       /// move up/down in math or any non text inset, call for LFUN_UP/DOWN
+       bool upDownInMath(bool up);
        ///
        void plainErase();
        ///
@@ -294,9 +310,6 @@
        ///
        docstring getPossibleLabel();
 
-       /// moves position somehow up or down
-       bool goUpDown(bool up);
-
        /// the name of the macro we are currently inputting
        docstring macroName();
        /// where in the curent cell does the macro name start?
Index: src/insets/InsetTabular.cpp
===================================================================
--- src/insets/InsetTabular.cpp (revision 18519)
+++ src/insets/InsetTabular.cpp (working copy)
@@ -1,5 +1,5 @@
 /**
- * \file InsetTabular.cpp
+* \file InsetTabular.cpp
  * This file is part of LyX, the document processor.
  * Licence details can be found in the file COPYING.
  *
@@ -3336,7 +3336,7 @@
                                }
                        }
                if (sl == cur.top()) {
-                       cmd = FuncRequest(LFUN_FINISHED_UP);
+                       //cmd = FuncRequest(LFUN_FINISHED_UP);
                        cur.undispatched();
                }
                break;
Index: src/insets/Inset.h
===================================================================
--- src/insets/Inset.h  (revision 18519)
+++ src/insets/Inset.h  (working copy)
@@ -80,7 +80,11 @@
        virtual InsetMath * asInsetMath() { return 0; }
        /// true for 'math' math inset, but not for e.g. mbox
        virtual bool inMathed() const { return false; }
-
+       /// is this inset based on the TextInset class?
+       virtual InsetText * asTextInset() { return 0; }
+       /// is this inset based on the TextInset class?
+       virtual InsetText const * asTextInset() const { return 0; }
+       
        /// the real dispatcher
        void dispatch(Cursor & cur, FuncRequest & cmd);
        /**
@@ -244,8 +248,6 @@
        virtual bool descendable() const { return false; }
        /// does this contain text that can be change track marked in DVI?
        virtual bool canTrackChanges() const { return false; }
-       /// is this inset based on the TextInset class?
-       virtual InsetText const * asTextInset() const { return 0; }
        /// return true if the inset should be removed automatically
        virtual bool autoDelete() const;
 
Index: src/insets/InsetText.h
===================================================================
--- src/insets/InsetText.h      (revision 18519)
+++ src/insets/InsetText.h      (working copy)
@@ -64,6 +64,8 @@
        ///
        bool canTrackChanges() const { return true; }
        ///
+       InsetText * asTextInset() { return this; }
+       ///
        InsetText const * asTextInset() const { return this; }
        ///
        int latex(Buffer const &, odocstream &, OutputParams const &) const;
Index: src/Text2.cpp
===================================================================
--- src/Text2.cpp       (revision 18519)
+++ src/Text2.cpp       (working copy)
@@ -916,7 +916,7 @@
        cur.pit() = pit;
        cur.pos() = pos;
        cur.boundary(bound);
-       cur.x_target() = x;
+       cur.setTargetX(x);
 
        // try to descend into nested insets
        Inset * inset = checkInsetHit(cur.bv(), x, y);
@@ -938,8 +938,10 @@
 
        // Make sure the cursor points to the position before
        // this inset.
-       if (inset == insetBefore)
+       if (inset == insetBefore) {
                --cur.pos();
+               cur.boundary(false);
+       }
 
        // Try to descend recursively inside the inset.
        inset = inset->editXY(cur, x, y);
@@ -978,38 +982,29 @@
        // Tell BufferView to test for FitCursor in any case!
        cur.updateFlags(Update::FitCursor);
 
+       // not at paragraph start?
        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)) {
-                       if (!cur.boundary() && 
-                           cur.textRow().pos() == cur.pos() 
-                           // FIXME: the following two conditions are copied
-                           // from cursorRight; however, isLineSeparator()
-                           // is definitely wrong here, isNewline I'm not sure
-                           // about. I'm leaving them as comments for now,
-                           // until we understand why they should or shouldn't
-                           // be here.
-                           /*&& 
-                           !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);
+               // if on right side of boundary (i.e. not at paragraph end, but 
line end)
+               // -> just jump to the left side
+               if (!cur.boundary() &&
+                               cur.lastpos() != cur.pos() &&
+                               cur.textRow().pos() == cur.pos() &&
+                               !cur.paragraph().isLineSeparator(cur.pos()) &&
+                               !cur.paragraph().isNewline(cur.pos())) {
+                       return setCursor(cur, cur.pit(), cur.pos(), true, true);
                }
-               return updateNeeded;
+               
+               // go left and try to enter inset
+               if (checkAndActivateInset(cur, false))
+                       return false;
+               
+               // normal character left
+               return setCursor(cur, cur.pit(), cur.pos() - 1, true, false);
        }
 
-       if (cur.pit() > 0) {
-               // Steps into the paragraph above
+       // move to the previous paragraph or do nothing
+       if (cur.pit() > 0)
                return setCursor(cur, cur.pit() - 1, getPar(cur.pit() - 
1).size());
-       }
        return false;
 }
 
@@ -1019,168 +1014,36 @@
        // Tell BufferView to test for FitCursor in any case!
        cur.updateFlags(Update::FitCursor);
 
+       // 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);
+                       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, true)) {
-                       if (cur.textRow().endpos() == cur.pos() + 1 &&
-                           cur.textRow().endpos() != cur.lastpos() &&
-                           !cur.paragraph().isLineSeparator(cur.pos()) &&
-                           !cur.paragraph().isNewline(cur.pos())) {
-                               cur.boundary(true);
-                       }
-                       updateNeeded |= setCursor(cur, cur.pit(), cur.pos() + 
1, true, cur.boundary());
+               // in front of editable inset, i.e. jump into it?
+               if (checkAndActivateInset(cur, true))
+                       return false;
+               
+               // next character is left of boundary, but no explicit newline 
or line sep?
+               bool willBeLeftOfBoundary = false;
+               if (cur.textRow().endpos() == cur.pos() + 1 &&
+                   cur.textRow().endpos() != cur.lastpos() &&
+                               !cur.paragraph().isLineSeparator(cur.pos()) &&
+                               !cur.paragraph().isNewline(cur.pos())) {
+                       willBeLeftOfBoundary = true;
                }
-               return updateNeeded;
+               
+               // move right
+               return setCursor(cur, cur.pit(), cur.pos() + 1, true, 
willBeLeftOfBoundary);
        }
 
+       // move to next paragraph
        if (cur.pit() != cur.lastpit())
                return setCursor(cur, cur.pit() + 1, 0);
        return false;
 }
 
 
-bool Text::cursorUp(Cursor & cur)
-{
-       // Tell BufferView to test for FitCursor in any case!
-       cur.updateFlags(Update::FitCursor);
-
-       TextMetrics const & tm = cur.bv().textMetrics(this);
-       ParagraphMetrics const & pm = tm.parMetrics(cur.pit());
-
-       int row;
-       if (cur.pos() && cur.boundary())
-               row = pm.pos2row(cur.pos()-1);
-       else
-               row = pm.pos2row(cur.pos());
-
-       int x = cur.targetX();
-       cur.setTargetX();
-       // We want to keep the x-target on subsequent up movements
-       // that cross beyond the end of short lines. Thus a special
-       // handling when the cursor is at the end of line: Use the new 
-       // x-target only if the old one was before the end of line.
-       if (cur.pos() != pm.rows()[row].endpos() 
-               || (!isWithinRtlParagraph(cur) && x < cur.targetX())
-               || (isWithinRtlParagraph(cur) && x > cur.targetX())) {
-
-               x = cur.targetX();
-       }
-
-       if (!cur.selection()) {
-               int const y = bv_funcs::getPos(cur.bv(), cur, 
cur.boundary()).y_;
-               Cursor old = cur;
-               // Go to middle of previous row. 16 found to work OK;
-               // 12 = top/bottom margin of display math
-               int const margin = 3 * InsetMathHull::displayMargin() / 2;
-               editXY(cur, x, y - pm.rows()[row].ascent() - margin);
-               cur.clearSelection();
-
-               // This happens when you move out of an inset.
-               // And to give the DEPM the possibility of doing
-               // something we must provide it with two different
-               // cursors. (Lgb)
-               Cursor dummy = cur;
-               if (dummy == old)
-                       ++dummy.pos();
-
-               cur.bv().checkDepm(dummy, old);
-               return false;
-       }
-
-       bool updateNeeded = false;
-
-       if (row > 0) {
-               updateNeeded |= setCursor(cur, cur.pit(),
-                       tm.x2pos(cur.pit(), row - 1, x));
-       } else if (cur.pit() > 0) {
-               --cur.pit();
-               //cannot use 'par' now
-               ParagraphMetrics const & pmcur = cur.bv().parMetrics(this, 
cur.pit());
-               updateNeeded |= setCursor(cur, cur.pit(),
-                       tm.x2pos(cur.pit(), pmcur.rows().size() - 1, x));
-       }
-
-       cur.x_target() = x;
-
-       return updateNeeded;
-}
-
-
-bool Text::cursorDown(Cursor & cur)
-{
-       // Tell BufferView to test for FitCursor in any case!
-       cur.updateFlags(Update::FitCursor);
-
-       TextMetrics const & tm = cur.bv().textMetrics(this);
-       ParagraphMetrics const & pm = tm.parMetrics(cur.pit());
-
-       int row;
-       if (cur.pos() && cur.boundary())
-               row = pm.pos2row(cur.pos()-1);
-       else
-               row = pm.pos2row(cur.pos());
-
-       int x = cur.targetX();
-       cur.setTargetX();
-       // We want to keep the x-target on subsequent down movements
-       // that cross beyond the end of short lines. Thus a special
-       // handling when the cursor is at the end of line: Use the new 
-       // x-target only if the old one was before the end of line.
-       if (cur.pos() != pm.rows()[row].endpos() 
-               || (!isWithinRtlParagraph(cur) && x < cur.targetX())
-               || (isWithinRtlParagraph(cur) && x > cur.targetX())) {
-
-               x = cur.targetX();
-       }
-
-       if (!cur.selection()) {
-               int const y = bv_funcs::getPos(cur.bv(), cur, 
cur.boundary()).y_;
-               Cursor old = cur;
-               // To middle of next row
-               int const margin = 3 * InsetMathHull::displayMargin() / 2;
-               editXY(cur, x, y + pm.rows()[row].descent() + margin);
-               cur.clearSelection();
-
-               // This happens when you move out of an inset.
-               // And to give the DEPM the possibility of doing
-               // something we must provide it with two different
-               // cursors. (Lgb)
-               Cursor dummy = cur;
-               if (dummy == old)
-                       ++dummy.pos();
-               
-               bool const changed = cur.bv().checkDepm(dummy, old);
-
-               // Make sure that cur gets back whatever happened to dummy(Lgb)
-               if (changed)
-                       cur = dummy;
-
-               return false;
-       }
-
-       bool updateNeeded = false;
-
-       if (row + 1 < int(pm.rows().size())) {
-               updateNeeded |= setCursor(cur, cur.pit(),
-                       tm.x2pos(cur.pit(), row + 1, x));
-       } else if (cur.pit() + 1 < int(paragraphs().size())) {
-               ++cur.pit();
-               updateNeeded |= setCursor(cur, cur.pit(),
-                       tm.x2pos(cur.pit(), 0, x));
-       }
-
-       cur.x_target() = x;
-
-       return updateNeeded;
-}
-
-
 bool Text::cursorUpParagraph(Cursor & cur)
 {
        bool updated = false;
Index: src/mathed/InsetMathNest.cpp
===================================================================
--- src/mathed/InsetMathNest.cpp        (revision 18519)
+++ src/mathed/InsetMathNest.cpp        (working copy)
@@ -542,8 +542,8 @@
                        break;
                }
                cur.selHandle(cmd.action == LFUN_UP_SELECT);
-               if (!cur.up()) {
-                       cmd = FuncRequest(LFUN_FINISHED_UP);
+               if (!cur.upDownInMath(true)) {
+                       //cmd = FuncRequest(LFUN_FINISHED_UP);
                        cur.undispatched();
                }
                // fixes bug 1598. Please check!
@@ -558,8 +558,8 @@
                        break;
                }
                cur.selHandle(cmd.action == LFUN_DOWN_SELECT);
-               if (!cur.down()) {
-                       cmd = FuncRequest(LFUN_FINISHED_DOWN);
+               if (!cur.upDownInMath(false)) {
+                       //cmd = FuncRequest(LFUN_FINISHED_DOWN);
                        cur.undispatched();
                }
                // fixes bug 1598. Please check!
Index: src/Text3.cpp
===================================================================
--- src/Text3.cpp       (revision 18519)
+++ src/Text3.cpp       (working copy)
@@ -470,13 +470,12 @@
        case LFUN_UP_SELECT:
                //lyxerr << "handle LFUN_UP[SEL]:\n" << cur << endl;
                needsUpdate |= cur.selHandle(cmd.action == LFUN_UP_SELECT);
+               needsUpdate |= cur.upDownInText(true);
 
-               needsUpdate |= cursorUp(cur);
-
                if (!needsUpdate && oldTopSlice == cur.top()
                          && cur.boundary() == oldBoundary) {
                        cur.undispatched();
-                       cmd = FuncRequest(LFUN_FINISHED_UP);
+                       //cmd = FuncRequest(LFUN_FINISHED_UP);
                }
                if (cur.selection())
                        saveSelection(cur);
@@ -486,13 +485,13 @@
        case LFUN_DOWN_SELECT:
                //lyxerr << "handle LFUN_DOWN[SEL]:\n" << cur << endl;
                needsUpdate |= cur.selHandle(cmd.action == LFUN_DOWN_SELECT);
-               needsUpdate |= cursorDown(cur);
+               needsUpdate |= cur.upDownInText(false);
 
                if (!needsUpdate && oldTopSlice == cur.top() &&
                    cur.boundary() == oldBoundary)
                {
                        cur.undispatched();
-                       cmd = FuncRequest(LFUN_FINISHED_DOWN);
+                       //cmd = FuncRequest(LFUN_FINISHED_DOWN);
                }
                if (cur.selection())
                        saveSelection(cur);
@@ -1071,7 +1070,7 @@
                        int const y = std::max(0, std::min(wh - 1, cmd.y));
 
                        setCursorFromCoordinates(cur, cmd.x, y);
-                       cur.x_target() = cmd.x;
+                       cur.setTargetX(cmd.x);
                        if (cmd.y >= wh)
                                lyx::dispatch(FuncRequest(LFUN_DOWN_SELECT));
                        else if (cmd.y < 0)
@@ -1448,12 +1447,12 @@
 
        case LFUN_FINISHED_UP:
                LYXERR(Debug::DEBUG) << "handle LFUN_FINISHED_UP:\n" << cur << 
endl;
-               cursorUp(cur);
+               cur.up();
                break;
 
        case LFUN_FINISHED_DOWN:
                LYXERR(Debug::DEBUG) << "handle LFUN_FINISHED_DOWN:\n" << cur 
<< endl;
-               cursorDown(cur);
+               cur.down();
                break;
 
        case LFUN_LAYOUT_PARAGRAPH: {
Index: src/Cursor.cpp
===================================================================
--- src/Cursor.cpp      (revision 18519)
+++ src/Cursor.cpp      (working copy)
@@ -6,12 +6,14 @@
  * \author Alejandro Aguilar Sierra
  * \author Alfredo Braunstein
  * \author André Pönitz
+ * \author Stefan Schimanski
  *
  * Full author contact details are available in file CREDITS.
  */
 
 #include <config.h>
 
+#include "Bidi.h"
 #include "BufferView.h"
 #include "bufferview_funcs.h"
 #include "Buffer.h"
@@ -262,7 +264,7 @@
 // be careful: this is called from the bv's constructor, too, so
 // bv functions are not yet available!
 Cursor::Cursor(BufferView & bv)
-       : DocIterator(), bv_(&bv), anchor_(), x_target_(-1),
+       : DocIterator(), bv_(&bv), anchor_(), x_target_(-1), 
textTargetOffset_(0),
          selection_(false), mark_(false), logicalpos_(false)
 {}
 
@@ -296,7 +298,11 @@
        fixIfBroken();
        FuncRequest cmd = cmd0;
        Cursor safe = *this;
-
+       
+       // store some values to be used inside of the handlers
+       getPos(beforeDispX_, beforeDispY_);
+       beforeDispDepth_ = depth();
+       
        for (; depth(); pop()) {
                LYXERR(Debug::DEBUG) << "Cursor::dispatch: cmd: "
                        << cmd0 << endl << *this << endl;
@@ -526,9 +532,10 @@
 }
 
 
-int & Cursor::x_target()
+void Cursor::setTargetX(int x)
 {
-       return x_target_;
+       x_target_ = x;
+       textTargetOffset_ = 0;
 }
 
 
@@ -541,10 +548,19 @@
 void Cursor::clearTargetX()
 {
        x_target_ = -1;
+       textTargetOffset_ = 0;
 }
 
 
+void Cursor::updateTextTargetOffset()
+{
+       int x;
+       int y;
+       getPos(x, y);
+       textTargetOffset_ = x - x_target_;
+}
 
+
 void Cursor::info(odocstream & os) const
 {
        for (int i = 1, n = depth(); i < n; ++i) {
@@ -662,7 +678,7 @@
 
 void Cursor::setScreenPos(int x, int y)
 {
-       x_target() = x;
+       setTargetX(x);
        bruteFind(*this, x, y, 0, bv().workWidth(), 0, bv().workHeight());
 }
 
@@ -876,11 +892,13 @@
 {
        macroModeClose();
        DocIterator save = *this;
-       if (goUpDown(true))
+       FuncRequest cmd(selection() ? LFUN_UP_SELECT : LFUN_UP, docstring());
+       this->dispatch(cmd);
+       if (disp_.dispatched())
                return true;
        setCursor(save);
        autocorrect() = false;
-       return selection();
+       return false;
 }
 
 
@@ -888,11 +906,13 @@
 {
        macroModeClose();
        DocIterator save = *this;
-       if (goUpDown(false))
+       FuncRequest cmd(selection() ? LFUN_DOWN_SELECT : LFUN_DOWN, 
docstring());
+       this->dispatch(cmd);
+       if (disp_.dispatched())
                return true;
        setCursor(save);
        autocorrect() = false;
-       return selection();
+       return false;
 }
 
 
@@ -951,17 +971,25 @@
 }
 
 
+int Cursor::textTargetOffset() const
+{
+       return textTargetOffset_;
+}
+
+
 void Cursor::setTargetX()
 {
        int x;
        int y;
        getPos(x, y);
-       x_target_ = x;
+       setTargetX(x);
 }
 
 
 bool Cursor::inMacroMode() const
 {
+       if (!inMathed())
+               return false;
        if (pos() == 0)
                return false;
        InsetMathUnknown const * p = prevAtom()->asUnknownInset();
@@ -1027,26 +1055,52 @@
 }
 
 
-bool Cursor::goUpDown(bool up)
+bool Cursor::upDownInMath(bool up)
 {
        // Be warned: The 'logic' implemented in this function is highly
        // fragile. A distance of one pixel or a '<' vs '<=' _really
        // matters. So fiddle around with it only if you think you know
        // what you are doing!
-
        int xo = 0;
        int yo = 0;
        getPos(xo, yo);
+       xo = beforeDispX_;
 
        // check if we had something else in mind, if not, this is the future
        // target
-       if (x_target() == -1)
-               x_target() = xo;
-       else
-               xo = x_target();
+       if (x_target_ == -1)
+               setTargetX(xo);
+       else if (inset().asTextInset() && xo - textTargetOffset() != 
x_target()) {
+               // In text mode inside the line (not left or right) possibly 
set a new target_x,
+               // but only if we are somewhere else than the previous 
target-offset.
+               
+               // We want to keep the x-target on subsequent up/down movements
+               // that cross beyond the end of short lines. Thus a special
+               // handling when the cursor is at the end of line: Use the new 
+               // x-target only if the old one was before the end of line
+               // or the old one was after the beginning of the line
+               bool inRTL = isWithinRtlParagraph(*this);
+               bool left;
+               bool right;
+               if (inRTL) {
+                       left = pos() == textRow().endpos();
+                       right = pos() == textRow().pos();
+               } else {
+                       left = pos() == textRow().pos();
+                       right = pos() == textRow().endpos();
+               }
+               if ((!left && !right) ||
+                               (left && !right && xo < x_target_) || 
+                               (!left && right && x_target_ < xo))
+                       setTargetX(xo);
+               else
+                       xo = targetX();
+       } else 
+               xo = targetX();
 
        // try neigbouring script insets
-       if (!selection()) {
+       Cursor old = *this;
+       if (inMathed() && !selection()) {
                // try left
                if (pos() != 0) {
                        InsetMathScript const * p = prevAtom()->asScriptInset();
@@ -1055,10 +1109,19 @@
                                push(*const_cast<InsetMathScript*>(p));
                                idx() = p->idxOfScript(up);
                                pos() = lastpos();
-                               return true;
+                               
+                               // we went in the right direction? Otherwise 
don't jump into the script
+                               int x;
+                               int y;
+                               getPos(x, y);
+                               if ((!up && y <= beforeDispY_) ||
+                                               (up && y >= beforeDispY_))
+                                       operator=(old);
+                               else
+                                       return true;
                        }
                }
-
+               
                // try right
                if (pos() != lastpos()) {
                        InsetMathScript const * p = nextAtom()->asScriptInset();
@@ -1066,62 +1129,159 @@
                                push(*const_cast<InsetMathScript*>(p));
                                idx() = p->idxOfScript(up);
                                pos() = 0;
-                               return true;
+                               
+                               // we went in the right direction? Otherwise 
don't jump into the script
+                               int x;
+                               int y;
+                               getPos(x, y);
+                               if ((!up && y <= beforeDispY_) ||
+                                               (up && y >= beforeDispY_))
+                                       operator=(old);
+                               else
+                                       return true;
                        }
                }
        }
-
-// FIXME: Switch this on for more robust movement
-#if 0
-
-       return bruteFind3(*this, xo, yo, up);
-
-#else
-       //xarray().boundingBox(xlow, xhigh, ylow, yhigh);
-       //if (up)
-       //      yhigh = yo - 4;
-       //else
-       //      ylow = yo + 4;
-       //if (bruteFind(*this, xo, yo, xlow, xhigh, ylow, yhigh)) {
-       //      lyxerr << "updown: handled by brute find in the same cell" << 
endl;
-       //      return true;
-       //}
-
-       // try to find an inset that knows better then we
-       while (true) {
-               //lyxerr << "updown: We are in " << &inset() << " idx: " << 
idx() << endl;
-               // ask inset first
-               if (inset().idxUpDown(*this, up)) {
-                       //lyxerr << "idxUpDown triggered" << endl;
-                       // try to find best position within this inset
-                       if (!selection())
-                               setCursor(bruteFind2(*this, xo, yo));
-                       return true;
-               }
-
-               // no such inset found, just take something "above"
-               if (!popLeft()) {
-                       //lyxerr << "updown: popleft failed (strange case)" << 
endl;
-                       int ylow  = up ? 0 : yo + 1;
-                       int yhigh = up ? yo - 1 : bv().workHeight();
-                       return bruteFind(*this, xo, yo, 0, bv().workWidth(), 
ylow, yhigh);
-               }
-
-               // any improvement so far?
+               
+       // try to find an inset that knows better then we,
+       if (inset().idxUpDown(*this, up)) {
+               //lyxerr << "idxUpDown triggered" << endl;
+               // try to find best position within this inset
+               if (!selection())
+                       setCursor(bruteFind2(*this, xo, yo));
+               return true;
+       }
+       
+       // any improvement going just out of inset?
+       if (popLeft() && inMathed()) {
                //lyxerr << "updown: popLeft succeeded" << endl;
                int xnew;
                int ynew;
                getPos(xnew, ynew);
-               if (up ? ynew < yo : ynew > yo)
+               if (up ? ynew < beforeDispY_ : ynew > beforeDispY_)
                        return true;
        }
-
-       // we should not come here.
-       BOOST_ASSERT(false);
-#endif
+       
+       // no success, we are probably at the document top or bottom
+       operator=(old);
+       return false;
 }
 
 
+bool Cursor::upDownInText(bool up) 
+{
+       BOOST_ASSERT(text());
+
+       // where are we?
+       int xo = 0;
+       int yo = 0;
+       getPos(xo, yo);
+       xo = beforeDispX_;
+       
+       // update the targetX - this is here before the "return false"
+       // to set a new target which can be used by InsetTexts above
+       // if we cannot move up/down inside this inset anymore
+       if (x_target_ == -1)
+               setTargetX(xo);
+       else if (xo - textTargetOffset() != x_target() && depth() == 
beforeDispDepth_) {
+               // In text mode inside the line (not left or right) possibly 
set a new target_x,
+               // but only if we are somewhere else than the previous 
target-offset.
+               
+               // We want to keep the x-target on subsequent up/down movements
+               // that cross beyond the end of short lines. Thus a special
+               // handling when the cursor is at the end of line: Use the new 
+               // x-target only if the old one was before the end of line
+               // or the old one was after the beginning of the line
+               bool inRTL = isWithinRtlParagraph(*this);
+               bool left;
+               bool right;
+               if (inRTL) {
+                       left = pos() == textRow().endpos();
+                       right = pos() == textRow().pos();
+               } else {
+                       left = pos() == textRow().pos();
+                       right = pos() == textRow().endpos();
+               }
+               if ((!left && !right) ||
+                               (left && !right && xo < x_target_) || 
+                               (!left && right && x_target_ < xo))
+                       setTargetX(xo);
+               else
+                       xo = targetX();
+       } else 
+               xo = targetX();
+               
+       // first get the current line
+       TextMetrics const & tm = bv_->textMetrics(text());
+       ParagraphMetrics const & pm = tm.parMetrics(pit());
+       int row;
+       if (pos() && boundary())
+               row = pm.pos2row(pos() - 1);
+       else
+               row = pm.pos2row(pos());
+               
+       // are we not at the start or end?
+       if (up) {
+               if (pit() == 0 && row == 0)
+                       return false;
+       } else {
+               if (pit() + 1 >= int(text()->paragraphs().size()) && 
+                               row + 1 >= int(pm.rows().size()))
+                       return false;
+       }       
+       
+       // with and without selection are handled differently
+       if (!selection()) {
+               int yo = bv_funcs::getPos(bv(), *this, boundary()).y_;
+               Cursor old = *this;
+               // To next/previous row
+               if (up)
+                       text()->editXY(*this, xo, yo - textRow().ascent() - 1);
+               else
+                       text()->editXY(*this, xo, yo + textRow().descent() + 1);
+               clearSelection();
+               
+               // This happens when you move out of an inset.
+               // And to give the DEPM the possibility of doing
+               // something we must provide it with two different
+               // cursors. (Lgb)
+               Cursor dummy = *this;
+               if (dummy == old)
+                       ++dummy.pos();
+               
+               bool const changed = bv().checkDepm(dummy, old);
+               
+               // Make sure that cur gets back whatever happened to dummy(Lgb)
+               if (changed)
+                       operator=(dummy);
+       } else {
+               // if there is a selection, we stay out of any inset, and just 
jump to the right position:
+               Cursor old = *this;
+               if (up) {
+                       if (row > 0) {
+                               top().pos() = std::min(tm.x2pos(pit(), row - 1, 
xo), top().lastpos());
+                       } else if (pit() > 0) {
+                               --pit();
+                               ParagraphMetrics const & pmcur = 
bv_->parMetrics(text(), pit());
+                               top().pos() = std::min(tm.x2pos(pit(), 
pmcur.rows().size() - 1, xo), top().lastpos());
+                       }
+               } else {
+                       if (row + 1 < int(pm.rows().size())) {
+                               top().pos() = std::min(tm.x2pos(pit(), row + 1, 
xo), top().lastpos());
+                       } else if (pit() + 1 < 
int(text()->paragraphs().size())) {
+                               ++pit();
+                               top().pos() = std::min(tm.x2pos(pit(), 0, xo), 
top().lastpos());
+                       }
+               }
+               
+               bv().checkDepm(*this, old);
+       }
+       
+       updateTextTargetOffset();
+       return true;
+}      
+
+
 void Cursor::handleFont(string const & font)
 {
        LYXERR(Debug::DEBUG) << BOOST_CURRENT_FUNCTION << ": " << font << endl;
Index: src/Text.h
===================================================================
--- src/Text.h  (revision 18519)
+++ src/Text.h  (working copy)
@@ -208,18 +208,6 @@
        /// setCursorFromCoordinates() instead of checkInsetHit().
        Inset * editXY(Cursor & cur, int x, int y);
        
-       /// Move cursor one line up.
-       /**
-        * Returns true if an update is needed after the move.
-        */
-       /// FIXME: move to TextMetrics.
-       bool cursorUp(Cursor & cur);
-       /// Move cursor one line down.
-       /**
-        * Returns true if an update is needed after the move.
-        */
-       /// FIXME: move to TextMetrics.
-       bool cursorDown(Cursor & cur);
        /// Move cursor one position left
        /**
         * Returns true if an update is needed after the move.

Reply via email to