Stephan Witt wrote:
> I made a new patch to implement getDiff() and use it to avoid the query for
> log message
> before checkIn() is done and the confirmation on revert when no diff is found.
thanks for your patience, i went closely through the patch now and generally
liked the approach.
> +FileName const CVS::getDiff(OperationMode opmode)
> +{
> + FileName tmpf = FileName::tempName("lyxvcout");
> + if (tmpf.empty()) {
> + LYXERR(Debug::LYXVC, "Could not generate logfile " << tmpf);
> + return tmpf;
> + }
> +
> + doVCCommand("cvs diff " + getTarget(opmode)
> + + " > " + quoteName(tmpf.toFilesystemEncoding()),
> + FileName(owner_->filePath()), false);
> + return tmpf;
> +}
the error case is suspicious. if tempName fails or "cvs diff" fails how you
detect
this on a higher stage? cmiiw but if something fails you identify it with having
empty diff, which looks wrong, even more if you release lock automatically in
such
a case...
> +int CVS::update(OperationMode opmode, FileName const tmpf)
unless you want to mix this with repoupdate why is opmode here?
> + // should a log message provided for next checkin?
> + virtual bool checkInWithMessage() { return true; }
> + // should a confirmation before revert requested?
> + virtual bool revertWithConfirmation() { return true; }
i would change naming, so its clear what the function really does.
for example isCheckinWithConfirmation...
> private:
> support::FileName file_;
> // revision number from scanMaster
> std::string version_;
> /// The user currently keeping the lock on the VC file.
> std::string locker_;
> +
> + virtual std::string const getTarget(OperationMode opmode);
> + virtual support::FileName const getDiff(OperationMode opmode);
> + virtual int edit();
> + virtual int unedit();
> + virtual int update(OperationMode opmode, support::FileName const tmpf);
> + virtual bool const hasDiff(OperationMode opmode);
> + virtual bool const hasDiff() { return hasDiff(File); }
> };
comments missing
> --- src/LyXVC.cpp (Revision 35732)
> +++ src/LyXVC.cpp (Arbeitskopie)
> @@ -163,9 +163,10 @@
> docstring empty(_("(no log message)"));
> docstring response;
> string log;
> - bool ok = Alert::askForText(response, _("LyX VC: Log Message"));
> + bool dolog = vcs->checkInWithMessage();
> + bool ok = !dolog || Alert::askForText(response, _("LyX VC: Log
> Message"));
hmm, but if !dolog then user automatically gets "cancel" message?
> if (ok) {
> - if (response.empty())
> + if (dolog && response.empty())
> response = empty;
why response=empty harms in !nodolog?
> @@ -212,8 +213,9 @@
> docstring text = bformat(_("Reverting to the stored version of the "
> "document %1$s will lose all current
> changes.\n\n"
> "Do you want to revert to the older version?"),
> file);
> - int const ret = Alert::prompt(_("Revert to stored version of
> document?"),
> - text, 0, 1, _("&Revert"), _("&Cancel"));
> + bool const doask = vcs->revertWithConfirmation();
i would need to check whats above this function in guiview but isn't possible
that we will skip
reverting in case the document is edited but not saved?
> + int const ret = doask ? Alert::prompt(_("Revert to stored version of
> document?"),
> + text, 0, 1, _("&Revert"), _("&Cancel")) : 0;
correspondingly to your previous coding style one would expect ret = doask &&
prompt ; ;)
pavel
