undo sometimes incorrectly tries to undo a larger container than is appropriate

[alex heneveld]

if you're undoing a command, "undo" will undo the largest container which ends with the command; this is not always correct behaviour, for example if our document ends halfway through a proof, or if we've made an edit which has reset the parsed container regions, then undo will try to undo the lemma, which is incorrect (and can cause prover state to become inconsistent).

one option is to look at the proof state and only undo the largest container which has been closed (processed to the point where it is discharged, e.g. lemma .... sorry).

alternatively when we do a parse we could record whether the container is closed. if we have to change the length of that container (e.g. because of an edit) then we reset the container to be open, and we don't undo it until a parse tells us its correct end location.

it is possible that i broke this (if this used to work, then it is very likely!), by adjusting the end location of containers when edits are done. however i think the changes i made were necessary. but i think what i did to break it was approximately correct.

(were we using empty children or whitespace to indicate that a container is parsed to closure? that would explain how i broke it. if so, i suggest we refactor to use a boolean "containerParsedToClosure" on a container to indicate that it is parsed to closure, rather than sticking empty containers or whitespace into the tree.)

[David Aspinall]

Thanks for the notes. The broken behaviour isn't your fault; I believe it was working reliably at one point but some refactoring of container management added an OBO error, I think. I should be fixing this myself, will investigate ASAP.

The algorithm isn't (or shouldn't be) quite as you describe it: we calculate the nearest reachable target position before the one the user requested. This doesn't always require undoing containers.

Remark: I believe that the tree (document model for script) ought to contain the parsed text including whitespace elements, ideally. Invariant: it should be correct at least as far as the processed point of the text (since the processed region is not normally edited). Therefore we can use it for undo calculations. Compared with the previous duplication of history mechanism, this loses if the user insists on editing processed text (we will require reparsing and then calculate undo according to new document contents). But current thinking is that if the user is editing the processed region they can expect to cause havoc anyway.

I've been tracking this under #24 (now changed priorities and moved milestone), so I'm marking this one as a duplicate.

[David Aspinall]

Milestone PG-Eclipse-1.0.6 deleted