On Mon, Oct 13, 2014 at 5:39 PM, Jean-Marc Lasgouttes <lasgout...@lyx.org> wrote: > Le 13/10/2014 14:29, Alfredo Braunstein a écrit : >> >> The fix however seems bogus to me: > > >> - cur.forwardPos(); >> + cur.top().forwardPos(); > > > Why bogus? Why do you want to revert it?
To me it seems wrong (for the reasons explained below). But maybe I don't understand it, could you explain? Was your intention to make the same change also to the other instance of cur.forwardPos() in that function? Right now it doesn't achieve what the commit message says and doesn't stop the crash... >> If you look at the top of the chunk, there is another >> forwardPos/backwardPos that could step into an inset (and it is the >> one iterated by the way). Besides, stepping into insets is needed when >> looking for changes in other cases (accept all etc). Thus IMHO 7c3d1d7 >> should be reverted. > > > I think that the functions should be rewritten from scratch :) You forgot the attachment? ;-) SeriousIy though, I totally agree. But for the moment let's fix it... If nothing else for the sake of the stable branch. >> I think that we should first search the initial pos of the change in a >> "flat" version of findNextChange as in the attachment. Note that at >> the end I still call findNextChange, but just to select the full range >> (a bit overkill). > > That makes sense, but I do not have the time/energy to make sure that the > patch is correct. I cannot understand why this thing needs to be so > complicated. IMO the exposed interface to the change tracking mechanism it's pretty basic, and this is one reason why the code must be so complicated. >> I suspect that the code could be simplified enormously by adding an >> acessor function to Paragraph that returns the range of the change a >> given pos is in. But I'm not going to do it now... > > That would be a good idea indeed. A/
