I documented the problem in the patch itself. I suspect that the reason it didn't happen in previous releases is, that the "boundary" variable was setted correctly. I don't really understand what does the " boundary variable do, but I think it means "am I just in front of a math inset?". Now for the problem. The iff condition I changed, is responsible for fixing the cursor position in insets, which are in a Hebrew paragraph. Since in Hebrew we're Rtl, one should remove from the offset of the cursor the width of the cursor we're currently in. The problem occurs, since, instead of checking whether or not we are IN a certain inset, we check whether or not there's an inset in position X of the cursor. Now when there's an inset in position 3 in the paragraph, whether the cursor is in the inset or right before the inset, it's position is 3. What we're doing is "fetch the inset in the position of the cursor where the cursor is if availible, and remove it's width from the cursor X coor", so there's no difference whether or not we're in the inset or just before the inset. I asked for information about it in the list a few days ago (how can I find which inset are we EDITING), but no one answered :-). Thanks for forcing me to further document my change! I'll update and repatch.
On 5/22/07, Dov Feldstern <[EMAIL PROTECTED]> wrote:
Elazar Leibovich wrote: > The proposed patch affects RTL documents only, and fixes partially the > bug that made the cursor "jump" to the inset's end, when it was just > before the inset. > Good job tracking this down, Elazar! I tried the patch out, and it does work for the outermost insets. Do you understand *why* it works (and as part of that, why it doesn't work for deeper insets)? I haven't looked at the patch or the surrounding code very closely at all yet (and I may not have time to until at least the weekend), but the fact that the fix is only partial seems to me to hint that perhaps there's a better way to fix this, which would cover all cases. You know --- curing the symptom instead of the root cause type thing... I may be totally wrong, though... But it would help if we could explain in words how the cursor position is being determined, and why it's going wrong in these cases. Just a few notes which I don't know if you're aware of, and which may (or may not) help you in looking into this further: *) I'm pretty sure that this problem did not exist in 1.3.X (I don't know about 1.4.X) --- but you may want to look at this code there, perhaps you can see what has changed since then... *) Note that what you're trying to fix is problem (3) in bug 3551. See the comments there, and especially look at bug 1965 and its fix, which I suspect may be related to our problem. I'm not sure, though. If you've already gone through cursorX(), I think you'll be able to judge that better than me. *) Is this at all related to the boundary (I think it probably is)? *) how (if at all) does all this related to the way the cursor behaves before plain LTR text embedded in RTL? Also, a few important general comments: *) Please svn update! r18129 was over 300 revisions ago! Since then many things may have changed --- both in the source (meaning that it becomes less and less likely that the patches will apply cleanly --- and in fact, yours doesn't), and in the behavior (meaning you may be fixing things which are already fixed, or which behave a little differently than what you're trying to fix). Given the rate of commits in LyX, you really should svn update quite regularly; and certainly before submitting a patch. Also, you want to get the benefit of your patches which have already made it into the trunk! *) This is related: please make sure that when you submit a patch, only differences which are relevant to the given patch are included. The patch which you sent includes changes to Paragraph.cpp which were meant to fix something else, and which we've already ended up fixing differently; changes to Cursor.h and Cursor.cpp which may be changes which should be made, but are not related to this specific issue --- so if you think they should be fixed, please send them to the mailing list, but separately from other patches. Note that this is not *only* to avoid confusion (though that's important, too) --- it's also to make sure that the patch's behavior is not affected by pieces of code which are not related and are not part of what everyone else is seeing... *) A note regarding the last two points: when you svn update, and also when you're playing around with different patches, you have to do it carefully. You don't want an update to interfere with your fix, and you want to easily separate the different patches from each other. One way is to manually keep track of changes before updating, or before starting to work on a different patch: svn diff into a file for saving (to make sure you have a copy of your changes should anything go wrong with an update, for example). Another tool, which mostly automates this process (and which I've only discovered in the past few weeks, but which I already don't know how one can program without), is called quilt. The best place I've found for getting started with it is here: http://www.coffeebreaks.org/blogs/wp-content/archives/talks/2005/quilt/quiltintro-s5.html Keep up the good work! Dov > > ------------------------------------------------------------------------ > > Index: Cursor.h > =================================================================== > --- Cursor.h (revision 18129) > +++ Cursor.h (working copy) > @@ -143,7 +143,7 @@ > /// get some interesting description of top position > void info(odocstream & os) const; > /// are we in math mode (2), text mode (1) or unsure (0)? > - int currentMode(); > + int currentMode() const; > /// reset cursor bottom to the beginning of the given inset > // (sort of 'chroot' environment...) > void reset(Inset &); > Index: Text.cpp > =================================================================== > --- Text.cpp (revision 18129) > +++ Text.cpp (working copy) > @@ -1658,10 +1658,21 @@ > > // Make sure inside an inset we always count from the left > // edge (bidi!) -- MV > + // lyxerr << "depth " << bv.cursor().depth() << endl; > if (sl.pos() < par.size()) { > font = getFont(buffer, par, sl.pos()); > + //This fixes the cursor position in insets in RTL > + //Paragraphs. I added an assertion to make sure we > + //are IN some kind of inset. (so far it was applied even > + //if the cursor was right before a paragraph, since the > + //position of the cursor in the paragraph is the same > + //regardless of whether or not the cursor is in the inset or not. > + //FIXME: we need to be sure somehow that we're not in front of an > + //inset, this solution don't work in nested insets (mathed in a > + //footnote. > if (!boundary && font.isVisibleRightToLeft() > - && par.isInset(sl.pos())) > + && par.isInset(sl.pos()) > + && bv.cursor().depth()>1) > x -= par.getInset(sl.pos())->width(); > } > return int(x); > Index: Paragraph.cpp > =================================================================== > --- Paragraph.cpp (revision 18129) > +++ Paragraph.cpp (working copy) > @@ -1415,10 +1415,14 @@ > Paragraph::getUChar(BufferParams const & bparams, pos_type pos) const > { > value_type c = getChar(pos); > - if (!lyxrc.rtl_support) > + if (true || !lyxrc.rtl_support) > return c; > > - value_type uc = c; > + /* This was previously used to get the paranthesis correctly in > + * RtL paragraphs. Now it seems to have no other effect than breaking > + * paste in math insets when in RtL mode. > + * To revert the change delete the "true ||" above. > + * value_type uc = c; > switch (c) { > case '(': > uc = ')'; > @@ -1448,7 +1452,7 @@ > if (uc != c && getFontSettings(bparams, pos).isRightToLeft()) > return uc; > else > - return c; > + return c;*/ > } > > > Index: Cursor.cpp > =================================================================== > --- Cursor.cpp (revision 18129) > +++ Cursor.cpp (working copy) > @@ -382,7 +382,7 @@ > } > > > -int Cursor::currentMode() > +int Cursor::currentMode() const > { > BOOST_ASSERT(!empty()); > for (int i = depth() - 1; i >= 0; --i) {
