I agree with the confusion remark. What I should've done is to iff it with (lyxrc.bidiVisual) condition. I just wanted remarks on my implementation. Except, we might decide that the visual approach is the default one As of your multiline inset problem - this does not exist, since mathed insets (and I've implemented this ONLY for mathed insets) cannot span multiple lines. BTW, your log2vis function is a smart way to go, but it wouldn't work for the Mathed Inset AFAIK, it's not treated like regular text.
On 5/6/07, Dov Feldstern <[EMAIL PROTECTED]> wrote:
Hi Elazar! If I understand correctly, this patch is intended to provide "visual mode" cursor movement? So a few comments: 1) I find it a little confusing to have two different patches going around (the one I suggested yesterday, and this one), which are intended to solve the same issue (Bidi cursor movement), but using two different approaches (visual mode / logical mode). So firstly, I would suggest opening a new bug (actually, feature request) in bugzilla, something like "Add support for visual mode bidi cursor movement" (and perhaps create a link to 3551 and say that this is an alternative approach to solving the same problem), and then we can say that a patch is intended to solve 3551 or XXXX -- just to help clarify what each one is about. 2) If we add support for visual mode, we should add it as a user preference, not *instead of* logical mode, which is the current mode. So any patches should take this into account. In other words, we're going to need some preference variable "bidi_cursor_mode" which can be either "visual" or "logical", and then based on it's value, decide how to interpret the arrow keys. Figuring out how this should best be done will require some thinking. Also, ultimately we'll also need to add this option to the preferences, which I have no idea how to do --- but that can wait for now. If it's easier, for starters you may want to ignore this, and just create patches as if visual mode were the only mode we need to support. Later on we can think of how to integrate it. But (a) it would probably be good to keep in the back of your head the fact that we'll need to do this in the end, it may affect the implementation a bit; and (b) it should just be clear, that none of the patches for visual mode should go in until a means for allowing the user to choose between the modes is available. I know that I'm not being totally fair, since *I'm* providing patches without taking all of this into account. But I have the advantage of making patches to the existing approach, whereas you're suggesting a new approach. ;) But seriously, I'd be happy to see an option for visual mode as well, and I'll try to help out if I can. 3) Now, to the suggested patch itself: I think that this is not the correct approach. If you're going for visual mode, then you invariably have to take into account presentation issues --- such as the width of the screen, etc. For example, right now what you're doing is to move the cursor, upon entry into an inset, to the *end* of the inset. But that's not really the right thing to do: what if the cursor is five lines long (let alone five paragraphs) --- suddenly the cursor will move five lines down, and then start moving up as you move along? And of course, when you change the width of the window, or the screen font you're using, then suddenly the five lines become seven... This is *not* what visual mode means. What visual mode really means, from the user experience perspective, is this: when the user presses the LEFT key, the cursor will move to the position which on screen is to the left of the current position, regardless of the language. When the end of the line is reached, move down one line, etc. Therefore, I would suggest the following approach (which actually would be quite simple to implement, I think): use the functions vis2log and log2vis from Bidi.cpp. These translate a visual position to a logical one and vice versa. They already take into account everything I mentioned above: line breaks, etc., so they're exactly what's needed here. So now it would work something like this: when the LEFT key is pressed, you want to move from the current visual position x to visual position x+1 (or x-1, I'm not sure. In fact, I think this may depend on the paragraph's language). So to figure out where the cursor really needs to go, all you do is place it at vis2log(x+1). There are still some issue to work out, but basically, I definitely think that this is the way to go. Good luck! Dov Elazar Leibovich wrote: > Please try this patch for second approach. Please make sure I'm not > breaking anything. > See discussion here http://bugzilla.lyx.org/show_bug.cgi?id=3551 > > > ------------------------------------------------------------------------ > > Index: Text2.cpp > =================================================================== > --- Text2.cpp (revision 18129) > +++ Text2.cpp (working copy) > @@ -915,8 +915,12 @@ > > bool Text::cursorLeft(Cursor & cur) > { > + bool direction = false; > + if (isRTL(*cur.bv().buffer(), cur.paragraph())) > + direction = !direction; > // Tell BufferView to test for FitCursor in any case! > cur.updateFlags(Update::FitCursor); > + > > if (!cur.boundary() && cur.pos() > 0 && > cur.textRow().pos() == cur.pos() && > @@ -926,7 +930,7 @@ > } > if (cur.pos() != 0) { > bool updateNeeded = setCursor(cur, cur.pit(), cur.pos() - 1, true, false); > - if (!checkAndActivateInset(cur, false)) { > + if (!checkAndActivateInset(cur, direction)) { > /** FIXME: What's this cause purpose??? > bool boundary = cur.boundary(); > if (false && !boundary && > @@ -949,6 +953,9 @@ > bool Text::cursorRight(Cursor & cur) > { > // Tell BufferView to test for FitCursor in any case! > + bool direction = true; > + if (isRTL(*cur.bv().buffer(), cur.paragraph())) > + direction = !direction; > cur.updateFlags(Update::FitCursor); > > if (cur.pos() != cur.lastpos()) { > @@ -957,7 +964,7 @@ > true, false); > > bool updateNeeded = false; > - if (!checkAndActivateInset(cur, true)) { > + if (!checkAndActivateInset(cur, direction)) { > if (cur.textRow().endpos() == cur.pos() + 1 && > cur.textRow().endpos() != cur.lastpos() && > !cur.paragraph().isLineSeparator(cur.pos()) && > Index: Cursor.cpp > =================================================================== > --- Cursor.cpp (revision 18129) > +++ Cursor.cpp (working copy) > @@ -360,10 +360,14 @@ > { > BOOST_ASSERT(!empty()); > //lyxerr << "Leaving inset to the left" << endl; > + const pos_type lp = (depth() > 1) ? (*this)[depth() - 2].lastpos() : 0; > inset().notifyCursorLeaves(*this); > if (depth() == 1) > return false; > pop(); > + if (depth() == 1 && isRTL()) { > + pos() += lastpos() - lp + 1; > + } > return true; > } > > @@ -376,8 +380,11 @@ > inset().notifyCursorLeaves(*this); > if (depth() == 1) > return false; > + > pop(); > - pos() += lastpos() - lp + 1; > + if (!(depth() == 1 && isRTL())) { > + pos() += lastpos() - lp + 1; > + } > return true; > } >
