Am 02.11.2010 um 12:34 schrieb Vincent van Ravesteijn: > >>>> first step to cure the VCS load problem >>> Can I please remind you and ask you to write meaningful commit-logs. Logs >>> explaining why the change has been made, why the new design or solution or >>> implementation is better, what problem it fixed, why other solutions might >>> be wrong although it may seem not to be at first sight and so forth. >>> >>> In the example of the VCS load problem, please explain what the VCS load >>> problem is. Please explain why it is only a first step, and what still >>> needs to be done. >> The VCS load problem is (was) twofold: >> 1. the parser of the document needs the VCS state so this state (checked by >> file_found_hook) had to be done earlier. >> => first step, move it some lines up, above the readDocument() call. > > Why does the parser need to know the state ? If the state changes, do we then > reparse the file too ?
Yes. The file is reloaded after VCS operations. > I think this stumbles on a other design problem. > > If we have a file, we insert an InsetInfo about vcs, then we add the file to > a vcs, will the InsetInfo be updated ? Apparently not, apparently the file is > updated not until we parse the file again. See above. > We probably need to have some call in updateBuffer or something that will > update the InsetInfo's, and we call updateBuffer() probably somewhere after > loading the file, so we don't need to register the vc status while reading > the file. May be. My plan was to complete CVS backend implementation and the last changes did disturb it seriously. They where overdue but for my TODO list the worst time to happen. So, I where tempted to make the "hot fixes". > >> 2. in case of loading the autosave or emergency file the real file name has >> to be passed to file_found_hook. >> => the patch I posted does this. (I missed to diff src/buffer_funcs.cpp and >> - of course - I'd fix the comments). > > Well, I made two public functions of Buffer: loadLyXFile and loadThisLyXFile. > The first one takes a filename, checks for autosave/emergency files, and when > it is sure which file exactly it wants to read it calls loadThisLyXFile(). > So, after this point, the code shouldn't be adapted to the fact that LyX is > reading a different file as the user requested. > > The name of the file is already stored in d->filename, so we can just call > lyxvc.found_file_hook(d->filename). The only reason for readFile to have a > FileName parameter is that we might want to load a recovery/emergency save > file instead. This brings me to the conclusion that the FileName parameter to > loadLyXFile and loadThisLyXFile is already WRONG!. I agree. > We do not set d->filename when calling this file, so we have to make sure > that we call loadLyXFile and loadThisLyXFile with exactly the same filename > as the one we supplied when creating the buffer. If we rename the buffer we > have to explicitly call Buffer::setFileName. > > To conclude: instead of adding a second parameter to the load*LyXFIle > functions, I think we should lose all parameters because we already have this > information. The problem with lyxvc only revealed this bug. Thank you for > finding the thinko that was present in the code. Glad to hear this. > But please don't cure the symptom but criticisize and fix the (re)design. I did not have the time to follow your redesign of the buffer load operation closely. > Please correct me if I'm wrong. I'm just thinking aloud. But you cannot test it easily? I'll test your proposal... Stephan
