On Sun, Nov 21, 2004 at 03:37:35PM +0100, [EMAIL PROTECTED] wrote: > Andre, undo continues to have problems despite your last patch.
That's bad news, especially since my next free slot on a private 'development' machine will be in two weeks' time. I don't even have a working LyX here to check. > file -> new write 3 of 4 short paragraphs, select all, delete, undo, > undo -> normally crash > > > There are some things of that patch I don't understand. > > In textUndoOrRedo 1) what does the bv.cursor has to do with anything? It should be needed to e.g. give the target for the Undo struct that will be pushed on the redo stack. > 2) You seem to assume that the undo cell must be inside the cursor path? > (you just compare them by size) I think this is needed for the scheme to work. Now that you mention it, it could well be that some code using recordUndo() is not aware of this and stores a chunk that's too small. The requirement right now is that if you are going to change some buffer contents between positions given by dit1 and dit2, you need to use recordUndo() to create some Undo struct containing paragraphs from a cell pointed to by some dit0 such that dit1 and dit2 both contain dit0 - and recordUndo() needs to be called before the change is made. I.e if dit1 is (inset1_0, idx1_0, par1_0, ... inset1_n, idx1_n, par1_n) and if dit2 is (inset2_0, idx2_0, par2_0, ... inset2_m, idx2_m, par2_m) dit0 should be something like (inset0_0, ...inset0_k) with k sucht that inset1_i == inset2_i (== inset0_i) \forall 0\leq i\leq k. Now I suppose I assumed idx1_k == idx2_k in this situation which does not have to be true for tabulars and most of mathed. Well, this could be brushed over by using k = k - 1 in this case, but this wastes some memory. Not much in the math case (cells are small there in general) but potentially quite a bit in tabulars... OTOH this might be good enough for 1.4.0 as we aren't often modifying more than one tabular cell at a time. Ok, so lets really suppose we have idx1_k == idx2_k because some kind soul put k = k - 1 in the right place or fixed this issue properly. Than the Undo struct pushed on the undo stack will contain par1_k through par2_k, and dit0 as undo.cell. undo.cur will equal dit1, even if k != n, so undo.cur will always contain undo.cell - and that's the assumption you were asking about. -- There are a few convienience functions for the common case that everything happens in the cursor paragraph or near to it, but there are probably cases missing where undo creation needs to operate not on the depth of the current cursor because the operation will change cursor depth by e.g. leaving an inset. The main problem with the current code is that separating undo.cell and undo.cur is a very recent thing and so far nobody (including me) checked that all the callers of recordUndo() are aware of this assumption. Normally, just bv.cur() is used there for both, but in a fe places, it might be bv.cur for undo.cur and for undo.cell it's the same with a slice popped off. Maybe most of the convienience functions should be dropped and we should explicitly ask for two DocumentIterators (current cursor, and 'affected cell') in recordUndo(). OTOH, we have to pass bv anyway and current cursor could be taken from there, so 'affected cell' suffices, which would basically lead to what we have now interface-wise. Well, I don't know. In any case, callers of recordUndo() need to be checked that they really pass 'affected cell' and nothing smaller. Note that there's always a way to fulfill the requirement by using the whole (single) cell of the main LyX text as undo.cell, but of course, we cannot afford to store the whole doc for each undo. > Then there is a more particular problem (the one responsible for the crash > above). In the first recordUndo call, you make an undo between > cell_dit.pit() and cur_dit.pit() but the second corresponds to the yet to > be created state, so its pit can be out of range; but I feel that if you > answer the first two I can get a better grip at the problem. > > Of course, feel free to just solve it ;-) As I said: Not much of a chance to even reproduce the problem during the next two weeks. Andre'
