Jean-Marc Lasgouttes wrote:
I would tend to think that _some_ of the callers to cursorX need to apply Martin's test by themselves, but that the code does not belong in the method itself. However, I am just waving hands, I do not have time to look at the code long enough.
I don't see why you feel this way. I actually think that this *is* the correct place for it: as far as I understand, cursorX should return the X position at which the cursor should be placed. There is only one right answer to that question (in the case of an inset, the correct answer is apparently "at the *left* edge of the inset"), and this is where it's being determined. Part of what determines this is all kinds of corrections which need to be taken into account, and which are performed in this function; for example, a correction for big insets. One more correction which needs to be taken into account is Martin's test. The only problem is that currently, his test is implemented incorrectly for one specific edge case, which Elazar's proposed patch corrects.
Anyhow, I also looked into the question of who's calling cursorX. There are actually very few places where cursorX is called:
*) InsetText::cursorPos and InsetMathMBox::cursorPos, Martin's test is needed in both these places, so if we remove it from cursorX we'd have to add it in at least these *two* places. *) Twice in Text::drawSelection. Martin's test is right for here, too. (Anyhow, BTW, selection in RTL is broken anyhow at the moment, http://bugzilla.lyx.org/show_bug.cgi?id=3550; this is not, however, due to the fact that Martin's test is taking place in cursorX). *) coordOffset in bufferview_funcs.cpp: I don't know what this is, but I assume it is supposed to return the position at which the cursor is supposed to be, in which case Martin's test is correct.
So, if we move Martin's test out of cursorX, we're going to have to add it in 4 or 5 new places.
Finally, (a) Martin's test has been in for a while (since r10513, 10/2005), I don't know why it's only now that we're questioning whether this really belongs here or not; (b) even if it should be moved somewhere else, the proposed patch will have to go in anyway, unless the test is moved to exactly the places where it is relevant, which I highly doubt is possible --- at least not without making major changes to the RTL assumptions; (c) I don't think there's anyone who's going to spend the time now on figuring out where Martin's patch *should* go, and I don't think it would be justified for someone to spend the time on that, since as far as we know, there's nothing broken with it's placement. So it's here, for now. It has to be patched up, though, because it is implemented a tiny bit wrong. That's what Elazar's fix does. The only question is, can the fix be implemented in a cleaner way.
Dov
