[Patch] Test suite for compare function

Pavel Sanda sanda at lyx.org
Tue Nov 10 11:31:38 UTC 2020


On Mon, Nov 09, 2020 at 01:03:47AM +0100, Pavel Sanda wrote:
> > > > @@ -343,7 +348,12 @@ bool GuiCompare::initialiseParams(std::string const &par)
> > > >  	if (cmd.getArg(0) == "run") {
> > > >  		oldFileCB->setEditText(toqstr(cmd.getArg(1)));
> > > >  		newFileCB->setEditText(toqstr(cmd.getArg(2)));
> > > > -		slotOK();
> > > > +        enableControls(false);
> > > > +        run(true);
> > > > +
> > > > +        compare_->wait(1000000);
> > > 
> > > On a first sight this looks fishy. We unconditionally launch run
> > > on the place we previously did not without dependency on cmd_mode.
> > > Do I miss something?
> > 
> > Previously run() was called through slotOK(). The problem is, when invoking compare from the command line, the next command in the sequence was getting executed before the compare was finished because the compare code was getting executed in a thread (and so buffer-write-as couldn't find a buffer to write, as it had not yet been created).
> > 
> > Just adding the 'wait()' call was not enough, because the 'finished' signal still happened asynchronously, so I also added the parameter to run() so that the finished signal is not connected to (and then called the finished() method manually afterwards). There may be a cleaner way to do this, I'm happy to hear suggestions. But the current approach at least didn't seem terrible to me. It probably could use a comment to explain this, though, which I'll add.
> 
> Ok, I will dig into the code later. Launching run inside initialiseParams looks strange.

So I looked at the code for the command line run at this place looks good enough.
I have another concerns though:
1) slotOK contains error() call which seems to be missed now. If you don't want it for
   commandline run it should be conditioned on cmd_mode.
2) It seems that if (!cmd_mode) you will call finished(false) twice thus wrongly
   reverting the toggles from FuncRequest(LFUN_CHANGES_OUTPUT) in finished(), right?

Small glitches: 
2) Please add explaining comment for the cmd_mode parameter to the header.
3) As said in previous mail I would set timer either to infinity (or to something
   realistically short if you insist on some timing wall).

Pavel


More information about the lyx-devel mailing list