Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 22.10.2010 um 01:07 schrieb Pavel Sanda:

 Stephan Witt wrote:
 If the principle way is ok I can do the same for the RCS and SVN backend 
 too.
 
 One question regarding the View log button of repoUpdate: 
 here on Mac the dialog to display the log is unusable. 
 It is blocked by the next confirmation dialog. 
 Is this platform specific or on all platforms?
 
 no, this glitch is on all platforms (iirc resize did work, but the button 
 was
 silent).
 
 On Mac it's blocked completely.
 
 i carry the idea of trying to show that dialog as nonmodal. it maybe
 oneliner somewhere, but i never really get to solve it...
 
 Ok.
 
 i tried hopeless experiment to set the log window modal for this usecases
 but it does not work. dispatch returns from show-dialog lfun and does not
 wait for completion and i guess it would need deeper surgery or completely
 new dialog to arrange this.

A new dialog similar to display the VC log...

 maybe there is another way how to trigger the dialog which would wait,
 but my curiousity is exhausted now ;)

I had the same idea already. Luckily I didn't try it.
The other idea I had was the special dialog combining the buttons and the
presentation of the diff.

Stephan

PS. I'll answer your other mail later. Thanks for it already.

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 22.10.2010 um 02:11 schrieb Pavel Sanda:

 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...

Your right. I'll try to come up with a better solution.
BTW, I copied the tmpf allocation code sequence.
I think the debug message does not need to print the empty tmpf.

But I've found another way to solve the same goal - using cvs status.
Checking for the diff is not good enough.

While doing some stress test with checkIn() I found the error message when 
merge is needed
Something's wrong with cvs commit not acceptable. Then I tried to change that 
and failed to
solve it because of the stderr output of the VC command is lost silently. So I 
came to the
idea to implement getStatus() and use the result accordingly.

The result would be one out of
* uptodate
* locally modified
* needs checkout
* needs merge
* no cvs file

The checkInWithMessage() implementation would be then
{ return getStatus() == LocallyModified; }
And the checkIn() would do a switch on the getStatus() and
raise more descriptive error messages according the status.

 
 +int CVS::update(OperationMode opmode, FileName const tmpf)
 
 unless you want to mix this with repoupdate why is opmode here?

Yes. That's the plan.

 +// 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...

Ok.

 
 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

Ok.

 
 --- 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?

You mean the user should have the option to cancel the operation if the file is 
up-to-date?
Seems reasonable.

 
  if (ok) {
 -if (response.empty())
 +if (dolog  response.empty())
  response = empty;
 
 why response=empty harms in !nodolog?

It doesn't harm. It wastes CPU cycles to set response when !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?

Yes, so it is. Then reverting should be guarded with revertEnabled() too?

 
 +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 ;  ;)

I don't think so. The first is bool and the second is int.
But I can change the style to be more verbose if you like.
E. g.

if 

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 22.10.2010 um 15:28 schrieb Stephan Witt:

 Am 22.10.2010 um 02:11 schrieb Pavel Sanda:
 
 Stephan Witt wrote:
 
 @@ -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?
 
 Yes, so it is. Then reverting should be guarded with revertEnabled() too?

The simpler solution would be to check owner_-isClean() in 
CVS::revertWithConfirmation()...

Stephan

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 22.10.2010 um 15:28 schrieb Stephan Witt:

 Am 22.10.2010 um 02:11 schrieb Pavel Sanda:
 
 Stephan Witt wrote:
 While doing some stress test with checkIn() I found the error message when 
 merge is needed
 Something's wrong with cvs commit not acceptable. Then I tried to change 
 that and failed to
 solve it because of the stderr output of the VC command is lost silently. So 
 I came to the
 idea to implement getStatus() and use the result accordingly.
 
 The result would be one out of
 * uptodate
 * locally modified
 * needs checkout
 * needs merge
 * no cvs file
 
 The checkInWithMessage() implementation would be then
 { return getStatus() == LocallyModified; }
 And the checkIn() would do a switch on the getStatus() and
 raise more descriptive error messages according the status.

Unfortunately this doesn't solve all possible cases.
If the checkout is not the head version you cannot commit any changes.
But the cvs status output isn't aware of that - it's locally modified only.
In this case the error message of the commit is needed to tell the user what 
happened.
Hmm, isn't it possible to get both stdout and stderr from doVCcommandCall() 
into some result?

 
 But I can change the style to be more verbose if you like.
 E. g.
 
   if (vcs-checkInWithMessage()) {
   log = vcs-checkIn(to_utf8(empty));
   // Reserve empty string for cancel button
   if (log.empty())
   log = to_utf8(empty);
   } else if (Alert::askForText(response, _(LyX VC: Log Message))) {
   if (response.empty())
   response = empty;
   log = vcs-checkIn(to_utf8(response));
   // Reserve empty string for cancel button
   if (log.empty())
   log = to_utf8(empty);
   } else {
   LYXERR(Debug::LYXVC, LyXVC: user cancelled);
   }

Sorry, I meant if (!vcs-checkInWithMessage()) { ...

Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
  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...
 
 Your right. I'll try to come up with a better solution.
 BTW, I copied the tmpf allocation code sequence.
 I think the debug message does not need to print the empty tmpf.
 
 But I've found another way to solve the same goal - using cvs status.
 Checking for the diff is not good enough.
 
 While doing some stress test with checkIn() I found the error message when 
 merge is needed
 Something's wrong with cvs commit not acceptable. Then I tried to change 
 that and failed to
 solve it because of the stderr output of the VC command is lost silently. So 
 I came to the
 idea to implement getStatus() and use the result accordingly.
 
 The result would be one out of
 * uptodate
 * locally modified
 * needs checkout
 * needs merge
 * no cvs file
 
 The checkInWithMessage() implementation would be then
 { return getStatus() == LocallyModified; }
 And the checkIn() would do a switch on the getStatus() and
 raise more descriptive error messages according the status.

sorry is starts to go beyond my head, cf end of the mail...

  --- 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?
 
 You mean the user should have the option to cancel the operation if the file 
 is up-to-date?
 Seems reasonable.

i meant that there is currently else part in code below (not seen in patch) 
which
triggers cancel message, just in case no diff was found.

 if (ok) {
  -  if (response.empty())
  +  if (dolog  response.empty())
 response = empty;
  
  why response=empty harms in !nodolog?
 
 It doesn't harm. It wastes CPU cycles to set response when !nodolog.

the problem is how can various rcs/svn/cvs version react on empty message
as parameter. compared to this few cpu cycles are negligible.

  @@ -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?
 
 Yes, so it is. Then reverting should be guarded with revertEnabled() too?

either ask for isClean or not touch revert at all

  +  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 ;  ;)
 
 I don't think so. The first is bool and the second is int.
 But I can change the style to be more verbose if you like.

not let it as it is. it was kind of joke.

 BTW, I have the problem that returning strings as result code is strange.
 And SVN::checkOut() implementation returns the a non empty string on error
 when the temporary log file name cannot be generated.

yes its ugly hack and i did it because there was no reasonable way how to pass
return info to user at the moment i implemented (this should be better now).

there are many things which can be improved inside revision control code
implementation however i want recall our original settlement - do some changes
inside CVS but dont touch API at this stage of development. i dont have time to
reimplement things and test all possible usescases again before public release
of 2.0.

you wanted to fix that bug that we ask for log even if only lock (or something
like that in cvs) is to be relased, ok, but please dont go beyond that.

pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 22.10.2010 um 01:07 schrieb Pavel Sanda:

> Stephan Witt wrote:
 If the principle way is ok I can do the same for the RCS and SVN backend 
 too.
 
 One question regarding the "View log" button of repoUpdate: 
 here on Mac the dialog to display the log is unusable. 
 It is blocked by the next confirmation dialog. 
 Is this platform specific or on all platforms?
>>> 
>>> no, this glitch is on all platforms (iirc resize did work, but the button 
>>> was
>>> silent).
>> 
>> On Mac it's blocked completely.
>> 
>>> i carry the idea of trying to show that dialog as nonmodal. it maybe
>>> oneliner somewhere, but i never really get to solve it...
>> 
>> Ok.
> 
> i tried hopeless experiment to set the log window modal for this usecases
> but it does not work. dispatch returns from show-dialog lfun and does not
> wait for completion and i guess it would need deeper surgery or completely
> new dialog to arrange this.

A new dialog similar to display the VC log...

> maybe there is another way how to trigger the dialog which would wait,
> but my curiousity is exhausted now ;)

I had the same idea already. Luckily I didn't try it.
The other idea I had was the special dialog combining the buttons and the
presentation of the diff.

Stephan

PS. I'll answer your other mail later. Thanks for it already.

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 22.10.2010 um 02:11 schrieb Pavel Sanda:

> 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...

Your right. I'll try to come up with a better solution.
BTW, I copied the tmpf allocation code sequence.
I think the debug message does not need to print the empty tmpf.

But I've found another way to solve the same goal - using "cvs status".
Checking for the diff is not good enough.

While doing some "stress test" with checkIn() I found the error message when 
merge is needed
"Something's wrong with cvs commit" not acceptable. Then I tried to change that 
and failed to
solve it because of the stderr output of the VC command is lost silently. So I 
came to the
idea to implement getStatus() and use the result accordingly.

The result would be one out of
* uptodate
* locally modified
* needs checkout
* needs merge
* no cvs file

The checkInWithMessage() implementation would be then
{ return getStatus() == LocallyModified; }
And the checkIn() would do a switch on the getStatus() and
raise more descriptive error messages according the status.

> 
>> +int CVS::update(OperationMode opmode, FileName const tmpf)
> 
> unless you want to mix this with repoupdate why is opmode here?

Yes. That's the plan.

>> +// 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...

Ok.

> 
>> 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

Ok.

> 
>> --- 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?

You mean the user should have the option to cancel the operation if the file is 
up-to-date?
Seems reasonable.

> 
>>  if (ok) {
>> -if (response.empty())
>> +if (dolog && response.empty())
>>  response = empty;
> 
> why response=empty harms in !nodolog?

It doesn't harm. It wastes CPU cycles to set response when !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, _(""), _(""));
>> +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?

Yes, so it is. Then reverting should be guarded with revertEnabled() too?

> 
>> +int const ret = doask ? Alert::prompt(_("Revert to stored version of 
>> document?"),
>> +text, 0, 1, _(""), _("")) : 0;
> 
> correspondingly to your previous coding style one would expect 

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 22.10.2010 um 15:28 schrieb Stephan Witt:

> Am 22.10.2010 um 02:11 schrieb Pavel Sanda:
> 
>> Stephan Witt wrote:
>> 
>>> @@ -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, _(""), _(""));
>>> +   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?
> 
> Yes, so it is. Then reverting should be guarded with revertEnabled() too?

The simpler solution would be to check owner_->isClean() in 
CVS::revertWithConfirmation()...

Stephan

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 22.10.2010 um 15:28 schrieb Stephan Witt:

> Am 22.10.2010 um 02:11 schrieb Pavel Sanda:
> 
>> Stephan Witt wrote:
> While doing some "stress test" with checkIn() I found the error message when 
> merge is needed
> "Something's wrong with cvs commit" not acceptable. Then I tried to change 
> that and failed to
> solve it because of the stderr output of the VC command is lost silently. So 
> I came to the
> idea to implement getStatus() and use the result accordingly.
> 
> The result would be one out of
> * uptodate
> * locally modified
> * needs checkout
> * needs merge
> * no cvs file
> 
> The checkInWithMessage() implementation would be then
> { return getStatus() == LocallyModified; }
> And the checkIn() would do a switch on the getStatus() and
> raise more descriptive error messages according the status.

Unfortunately this doesn't solve all possible cases.
If the checkout is not the head version you cannot commit any changes.
But the "cvs status" output isn't aware of that - it's locally modified only.
In this case the error message of the commit is needed to tell the user what 
happened.
Hmm, isn't it possible to get both stdout and stderr from doVCcommandCall() 
into some result?

> 
> But I can change the style to be more verbose if you like.
> E. g.
> 
>   if (vcs->checkInWithMessage()) {
>   log = vcs->checkIn(to_utf8(empty));
>   // Reserve empty string for cancel button
>   if (log.empty())
>   log = to_utf8(empty);
>   } else if (Alert::askForText(response, _("LyX VC: Log Message"))) {
>   if (response.empty())
>   response = empty;
>   log = vcs->checkIn(to_utf8(response));
>   // Reserve empty string for cancel button
>   if (log.empty())
>   log = to_utf8(empty);
>   } else {
>   LYXERR(Debug::LYXVC, "LyXVC: user cancelled");
>   }

Sorry, I meant "if (!vcs->checkInWithMessage()) {" ...

Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
> > 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...
> 
> Your right. I'll try to come up with a better solution.
> BTW, I copied the tmpf allocation code sequence.
> I think the debug message does not need to print the empty tmpf.
> 
> But I've found another way to solve the same goal - using "cvs status".
> Checking for the diff is not good enough.
> 
> While doing some "stress test" with checkIn() I found the error message when 
> merge is needed
> "Something's wrong with cvs commit" not acceptable. Then I tried to change 
> that and failed to
> solve it because of the stderr output of the VC command is lost silently. So 
> I came to the
> idea to implement getStatus() and use the result accordingly.
> 
> The result would be one out of
> * uptodate
> * locally modified
> * needs checkout
> * needs merge
> * no cvs file
> 
> The checkInWithMessage() implementation would be then
> { return getStatus() == LocallyModified; }
> And the checkIn() would do a switch on the getStatus() and
> raise more descriptive error messages according the status.

sorry is starts to go beyond my head, cf end of the mail...

> >> --- 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?
> 
> You mean the user should have the option to cancel the operation if the file 
> is up-to-date?
> Seems reasonable.

i meant that there is currently else part in code below (not seen in patch) 
which
triggers cancel message, just in case no diff was found.

> >>if (ok) {
> >> -  if (response.empty())
> >> +  if (dolog && response.empty())
> >>response = empty;
> > 
> > why response=empty harms in !nodolog?
> 
> It doesn't harm. It wastes CPU cycles to set response when !nodolog.

the problem is how can various rcs/svn/cvs version react on empty message
as parameter. compared to this few cpu cycles are negligible.

> >> @@ -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, _(""), _(""));
> >> +  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?
> 
> Yes, so it is. Then reverting should be guarded with revertEnabled() too?

either ask for isClean or not touch revert at all

> >> +  int const ret = doask ? Alert::prompt(_("Revert to stored version of 
> >> document?"),
> >> +  text, 0, 1, _(""), _("")) : 0;
> > 
> > correspondingly to your previous coding style one would expect ret = doask 
> > && prompt ;  ;)
> 
> I don't think so. The first is bool and the second is int.
> But I can change the style to be more verbose if you like.

not let it as it is. it was kind of joke.

> BTW, I have the problem that returning strings as result code is strange.
> And SVN::checkOut() implementation returns the a non empty string on error
> when the temporary log file name cannot be generated.

yes its ugly hack and i did it because there was no reasonable way how to pass
return info to user at the moment i implemented (this should be better now).

there are many things which can be improved inside revision control code
implementation however i want recall our original settlement - do some changes
inside CVS but dont touch API at this stage of development. i dont have time to
reimplement things and test all possible usescases again before public release
of 2.0.

you wanted to fix that bug that we ask for log even if only lock (or something
like that in cvs) is to be relased, ok, but please dont go beyond that.

pavel

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
  If the principle way is ok I can do the same for the RCS and SVN backend 
  too.
  
  One question regarding the View log button of repoUpdate: 
  here on Mac the dialog to display the log is unusable. 
  It is blocked by the next confirmation dialog. 
  Is this platform specific or on all platforms?
  
  no, this glitch is on all platforms (iirc resize did work, but the button 
  was
  silent).
 
 On Mac it's blocked completely.
 
  i carry the idea of trying to show that dialog as nonmodal. it maybe
  oneliner somewhere, but i never really get to solve it...
 
 Ok.

i tried hopeless experiment to set the log window modal for this usecases
but it does not work. dispatch returns from show-dialog lfun and does not
wait for completion and i guess it would need deeper surgery or completely
new dialog to arrange this.

maybe there is another way how to trigger the dialog which would wait,
but my curiousity is exhausted now ;)

pavel
diff --git a/src/VCBackend.cpp b/src/VCBackend.cpp
index 15b6033..9a6ff9d 100644
--- a/src/VCBackend.cpp
+++ b/src/VCBackend.cpp
@@ -824,7 +824,7 @@ string SVN::repoUpdate()
int ret = frontend::Alert::prompt(_(Changes detected),
text, 0, 1, _(Yes), _(No), _(View Log 
...));
if (ret == 2 ) {
-   dispatch(FuncRequest(LFUN_DIALOG_SHOW, file  + 
tmpf.absFileName()));
+   dispatch(FuncRequest(LFUN_DIALOG_SHOW, vclogmodal  + 
tmpf.absFileName()));
ret = frontend::Alert::prompt(_(Changes detected),
text, 0, 1, _(Yes), _(No));
hideDialogs(file, 0);
diff --git a/src/frontends/qt4/GuiLog.cpp b/src/frontends/qt4/GuiLog.cpp
index e5a8a96..d3c99a9 100644
--- a/src/frontends/qt4/GuiLog.cpp
+++ b/src/frontends/qt4/GuiLog.cpp
@@ -218,6 +218,7 @@ bool GuiLog::initialiseParams(string const  data)
 
logTypeCO-setEnabled(logtype == latex);
logTypeCO-clear();
+   setModal(false);

FileName log(logfile);

@@ -238,9 +239,11 @@ bool GuiLog::initialiseParams(string const  data)
} else if (logtype == lyx2lyx) {
type_ = Lyx2lyxLog;
logTypeCO-addItem(qt_(LyX2LyX), toqstr(logtype));
-   } else if (logtype == vc) {
+   } else if (logtype == vc || logtype == vcn) {
type_ = VCLog;
logTypeCO-addItem(qt_(Version Control), toqstr(logtype));
+   if (logtype == vcn )
+   setModal(true);
} else
return false;
 
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index e24ec97..8ed0770 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -1575,7 +1575,7 @@ bool GuiView::getStatus(FuncRequest const  cmd, 
FuncStatus  flag)
enable = 
FileName(doc_buffer-logName()).isReadableFile();
else if (name == spellchecker)
enable = theSpellChecker()  !doc_buffer-isReadonly();
-   else if (name == vclog)
+   else if (prefixIs(name, vclog))
enable = doc_buffer-lyxvc().inUse();
break;
}
@@ -3191,7 +3191,10 @@ void GuiView::dispatch(FuncRequest const  cmd, 
DispatchResult  dr)
}
data += Lexer::quoteString(logfile);
showDialog(log, data);
-   } else if (name == vclog) {
+   } else if (prefixIs(name, vclog)) {
+   string type = vc;
+   if (name == vclogmodal)
+   type = vcn;
string const data = vc  +

Lexer::quoteString(doc_buffer-lyxvc().getLogFile());
showDialog(log, data);

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

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

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
> >> If the principle way is ok I can do the same for the RCS and SVN backend 
> >> too.
> >> 
> >> One question regarding the "View log" button of repoUpdate: 
> >> here on Mac the dialog to display the log is unusable. 
> >> It is blocked by the next confirmation dialog. 
> >> Is this platform specific or on all platforms?
> > 
> > no, this glitch is on all platforms (iirc resize did work, but the button 
> > was
> > silent).
> 
> On Mac it's blocked completely.
> 
> > i carry the idea of trying to show that dialog as nonmodal. it maybe
> > oneliner somewhere, but i never really get to solve it...
> 
> Ok.

i tried hopeless experiment to set the log window modal for this usecases
but it does not work. dispatch returns from show-dialog lfun and does not
wait for completion and i guess it would need deeper surgery or completely
new dialog to arrange this.

maybe there is another way how to trigger the dialog which would wait,
but my curiousity is exhausted now ;)

pavel
diff --git a/src/VCBackend.cpp b/src/VCBackend.cpp
index 15b6033..9a6ff9d 100644
--- a/src/VCBackend.cpp
+++ b/src/VCBackend.cpp
@@ -824,7 +824,7 @@ string SVN::repoUpdate()
int ret = frontend::Alert::prompt(_("Changes detected"),
text, 0, 1, _(""), _(""), _("View  
..."));
if (ret == 2 ) {
-   dispatch(FuncRequest(LFUN_DIALOG_SHOW, "file " + 
tmpf.absFileName()));
+   dispatch(FuncRequest(LFUN_DIALOG_SHOW, "vclogmodal " + 
tmpf.absFileName()));
ret = frontend::Alert::prompt(_("Changes detected"),
text, 0, 1, _(""), _(""));
hideDialogs("file", 0);
diff --git a/src/frontends/qt4/GuiLog.cpp b/src/frontends/qt4/GuiLog.cpp
index e5a8a96..d3c99a9 100644
--- a/src/frontends/qt4/GuiLog.cpp
+++ b/src/frontends/qt4/GuiLog.cpp
@@ -218,6 +218,7 @@ bool GuiLog::initialiseParams(string const & data)
 
logTypeCO->setEnabled(logtype == "latex");
logTypeCO->clear();
+   setModal(false);

FileName log(logfile);

@@ -238,9 +239,11 @@ bool GuiLog::initialiseParams(string const & data)
} else if (logtype == "lyx2lyx") {
type_ = Lyx2lyxLog;
logTypeCO->addItem(qt_("LyX2LyX"), toqstr(logtype));
-   } else if (logtype == "vc") {
+   } else if (logtype == "vc" || logtype == "vcn") {
type_ = VCLog;
logTypeCO->addItem(qt_("Version Control"), toqstr(logtype));
+   if (logtype == "vcn" )
+   setModal(true);
} else
return false;
 
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index e24ec97..8ed0770 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -1575,7 +1575,7 @@ bool GuiView::getStatus(FuncRequest const & cmd, 
FuncStatus & flag)
enable = 
FileName(doc_buffer->logName()).isReadableFile();
else if (name == "spellchecker")
enable = theSpellChecker() && !doc_buffer->isReadonly();
-   else if (name == "vclog")
+   else if (prefixIs(name, "vclog"))
enable = doc_buffer->lyxvc().inUse();
break;
}
@@ -3191,7 +3191,10 @@ void GuiView::dispatch(FuncRequest const & cmd, 
DispatchResult & dr)
}
data += Lexer::quoteString(logfile);
showDialog("log", data);
-   } else if (name == "vclog") {
+   } else if (prefixIs(name, "vclog")) {
+   string type = "vc";
+   if (name == "vclogmodal")
+   type = "vcn";
string const data = "vc " +

Lexer::quoteString(doc_buffer->lyxvc().getLogFile());
showDialog("log", data);

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

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, _(""), _(""));
> + 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, _(""), _("")) : 0;

correspondingly to your previous coding style one would expect ret = doask && 
prompt ;  ;)


pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 19.10.2010 um 03:34 schrieb Pavel Sanda:

 Stephan Witt wrote:
 Pavel, can you have a look please?
 
 hopefully towards the end of this week. actually it would be really helpful
 if we can proceed in usual incremental manner - different patches for 
 different
 issues, its much harder to keep your track if everything is muddled into
 one big patch, ie patch for CVS related stuff and fixing other bugs on the 
 top of it.
 
 quick peek into diff - doVCCommand change looks ok (maybe call the param 
 reportError?).
 why 
 +   // to be sure test readonly state again...
   

 +   FileName fn(owner_-absFileName());   
   

 +   fn.refresh(); 
   

 +   if (!fn.isReadOnly())
 is not inside checkoutenabled?

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.

Next patch follows later...

Stephan

Index: src/VCBackend.cpp
===
--- src/VCBackend.cpp   (Revision 35732)
+++ src/VCBackend.cpp   (Arbeitskopie)
@@ -47,7 +47,7 @@
 }
 
 
-int VCS::doVCCommand(string const  cmd, FileName const  path)
+int VCS::doVCCommand(string const  cmd, FileName const  path, bool 
reportError)
 {
if (owner_)
owner_-setBusy(true);
@@ -56,7 +56,7 @@
 
if (owner_)
owner_-setBusy(false);
-   if (ret)
+   if (ret  reportError)
frontend::Alert::error(_(Revision control error.),
bformat(_(Some problem occured while running the 
command:\n
  '%1$s'.),
@@ -432,12 +432,82 @@
 }
 
 
+string const CVS::getTarget(OperationMode opmode)
+{
+   switch(opmode) {
+   case Directory:
+   return quoteName(owner_-filePath());
+   case File:
+   return quoteName(onlyFileName(owner_-absFileName()));
+   }
+   return string();
+}
+
+
+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;
+}
+
+
+bool const CVS::hasDiff(OperationMode opmode)
+{
+   FileName tmpf = getDiff(opmode);
+   docstring res = tmpf.fileContents(UTF-8);
+   tmpf.removeFile();
+   return !res.empty();
+}
+
+
+int CVS::edit()
+{
+   vcstatus = LOCKED;
+   return doVCCommand(cvs -q edit 
+   + quoteName(onlyFileName(owner_-absFileName())),
+   FileName(owner_-filePath()));
+}
+
+
+int CVS::unedit()
+{
+   vcstatus = UNLOCKED;
+   return doVCCommand(cvs -q unedit 
+   + quoteName(onlyFileName(owner_-absFileName())),
+   FileName(owner_-filePath()));
+}
+
+
+int CVS::update(OperationMode opmode, FileName const tmpf)
+{
+   string const redirection = tmpf.empty() ? 
+   :+ quoteName(tmpf.toFilesystemEncoding());
+
+   return doVCCommand(cvs -q update 
+   + getTarget(opmode) + redirection,
+   FileName(owner_-filePath()));
+}
+
+
 string CVS::checkIn(string const  msg)
 {
-   int ret = doVCCommand(cvs -q commit -m \ + msg + \ 
-   + quoteName(onlyFileName(owner_-absFileName())),
+   if (!hasDiff()) {
+   unedit();
+   return CVS: Proceeded;
+   }
+
+   int rc = doVCCommand(cvs -q commit -m \ + msg + \ 
+   + quoteName(onlyFileName(owner_-absFileName())),
FileName(owner_-filePath()));
-   return ret ? string() : CVS: Proceeded;
+   return rc ? string() : CVS: Proceeded;
 }
 
 
@@ -453,22 +523,20 @@
if (!checkOutEnabled())
return string();
 
-   int ret = doVCCommand(cvs -q edit 
- + 
quoteName(onlyFileName(owner_-absFileName())),
- FileName(owner_-filePath()));
+   int ret = edit();
if (ret)
return string();
 
-   ret = doVCCommand(cvs update 
- + 
quoteName(onlyFileName(owner_-absFileName())),
- FileName(owner_-filePath()));
+ 

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 19.10.2010 um 03:34 schrieb Pavel Sanda:

> Stephan Witt wrote:
>> Pavel, can you have a look please?
> 
> hopefully towards the end of this week. actually it would be really helpful
> if we can proceed in usual incremental manner - different patches for 
> different
> issues, its much harder to keep your track if everything is muddled into
> one big patch, ie patch for CVS related stuff and fixing other bugs on the 
> top of it.
> 
> quick peek into diff - doVCCommand change looks ok (maybe call the param 
> reportError?).
> why 
> +   // to be sure test readonly state again...
>   
>
> +   FileName fn(owner_->absFileName());   
>   
>
> +   fn.refresh(); 
>   
>
> +   if (!fn.isReadOnly())
> is not inside checkoutenabled?

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.

Next patch follows later...

Stephan

Index: src/VCBackend.cpp
===
--- src/VCBackend.cpp   (Revision 35732)
+++ src/VCBackend.cpp   (Arbeitskopie)
@@ -47,7 +47,7 @@
 }
 
 
-int VCS::doVCCommand(string const & cmd, FileName const & path)
+int VCS::doVCCommand(string const & cmd, FileName const & path, bool 
reportError)
 {
if (owner_)
owner_->setBusy(true);
@@ -56,7 +56,7 @@
 
if (owner_)
owner_->setBusy(false);
-   if (ret)
+   if (ret && reportError)
frontend::Alert::error(_("Revision control error."),
bformat(_("Some problem occured while running the 
command:\n"
  "'%1$s'."),
@@ -432,12 +432,82 @@
 }
 
 
+string const CVS::getTarget(OperationMode opmode)
+{
+   switch(opmode) {
+   case Directory:
+   return quoteName(owner_->filePath());
+   case File:
+   return quoteName(onlyFileName(owner_->absFileName()));
+   }
+   return string();
+}
+
+
+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;
+}
+
+
+bool const CVS::hasDiff(OperationMode opmode)
+{
+   FileName tmpf = getDiff(opmode);
+   docstring res = tmpf.fileContents("UTF-8");
+   tmpf.removeFile();
+   return !res.empty();
+}
+
+
+int CVS::edit()
+{
+   vcstatus = LOCKED;
+   return doVCCommand("cvs -q edit "
+   + quoteName(onlyFileName(owner_->absFileName())),
+   FileName(owner_->filePath()));
+}
+
+
+int CVS::unedit()
+{
+   vcstatus = UNLOCKED;
+   return doVCCommand("cvs -q unedit "
+   + quoteName(onlyFileName(owner_->absFileName())),
+   FileName(owner_->filePath()));
+}
+
+
+int CVS::update(OperationMode opmode, FileName const tmpf)
+{
+   string const redirection = tmpf.empty() ? ""
+   : " > " + quoteName(tmpf.toFilesystemEncoding());
+
+   return doVCCommand("cvs -q update "
+   + getTarget(opmode) + redirection,
+   FileName(owner_->filePath()));
+}
+
+
 string CVS::checkIn(string const & msg)
 {
-   int ret = doVCCommand("cvs -q commit -m \"" + msg + "\" "
-   + quoteName(onlyFileName(owner_->absFileName())),
+   if (!hasDiff()) {
+   unedit();
+   return "CVS: Proceeded";
+   }
+
+   int rc = doVCCommand("cvs -q commit -m \"" + msg + "\" "
+   + quoteName(onlyFileName(owner_->absFileName())),
FileName(owner_->filePath()));
-   return ret ? string() : "CVS: Proceeded";
+   return rc ? string() : "CVS: Proceeded";
 }
 
 
@@ -453,22 +523,20 @@
if (!checkOutEnabled())
return string();
 
-   int ret = doVCCommand("cvs -q edit "
- + 
quoteName(onlyFileName(owner_->absFileName())),
- FileName(owner_->filePath()));
+   int ret = edit();
if (ret)
return string();
 
-   ret = doVCCommand("cvs update "
- + 

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
 Am 19.10.2010 um 03:34 schrieb Pavel Sanda:
 
  Stephan Witt wrote:
  Pavel, can you have a look please?
  
  hopefully towards the end of this week. actually it would be really helpful
  if we can proceed in usual incremental manner - different patches for 
  different
  issues, its much harder to keep your track if everything is muddled into
  one big patch, ie patch for CVS related stuff and fixing other bugs on the 
  top of it.
 
 I don't know how to do that. I have only one LyX source tree in compilable 
 state.
 I'll try to make two patches out of it.

do you know git? :) http://wiki.lyx.org/Devel/Git
pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 19.10.2010 um 15:10 schrieb Pavel Sanda:

 Stephan Witt wrote:
 Am 19.10.2010 um 03:34 schrieb Pavel Sanda:
 
 Stephan Witt wrote:
 Pavel, can you have a look please?
 
 hopefully towards the end of this week. actually it would be really helpful
 if we can proceed in usual incremental manner - different patches for 
 different
 issues, its much harder to keep your track if everything is muddled into
 one big patch, ie patch for CVS related stuff and fixing other bugs on the 
 top of it.
 
 I don't know how to do that. I have only one LyX source tree in compilable 
 state.
 I'll try to make two patches out of it.
 
 do you know git? :) http://wiki.lyx.org/Devel/Git

No, I only heard of it.


Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
> Am 19.10.2010 um 03:34 schrieb Pavel Sanda:
> 
> > Stephan Witt wrote:
> >> Pavel, can you have a look please?
> > 
> > hopefully towards the end of this week. actually it would be really helpful
> > if we can proceed in usual incremental manner - different patches for 
> > different
> > issues, its much harder to keep your track if everything is muddled into
> > one big patch, ie patch for CVS related stuff and fixing other bugs on the 
> > top of it.
> 
> I don't know how to do that. I have only one LyX source tree in compilable 
> state.
> I'll try to make two patches out of it.

do you know git? :) http://wiki.lyx.org/Devel/Git
pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 19.10.2010 um 15:10 schrieb Pavel Sanda:

> Stephan Witt wrote:
>> Am 19.10.2010 um 03:34 schrieb Pavel Sanda:
>> 
>>> Stephan Witt wrote:
 Pavel, can you have a look please?
>>> 
>>> hopefully towards the end of this week. actually it would be really helpful
>>> if we can proceed in usual incremental manner - different patches for 
>>> different
>>> issues, its much harder to keep your track if everything is muddled into
>>> one big patch, ie patch for CVS related stuff and fixing other bugs on the 
>>> top of it.
>> 
>> I don't know how to do that. I have only one LyX source tree in compilable 
>> state.
>> I'll try to make two patches out of it.
> 
> do you know git? :) http://wiki.lyx.org/Devel/Git

No, I only heard of it.


Stephan

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 17.10.2010 um 21:00 schrieb Stephan Witt:

 Am 15.10.2010 um 21:28 schrieb Stephan Witt:
 
 Am 15.10.2010 um 18:00 schrieb Pavel Sanda:
 
 Pavel Sanda wrote:
 this is already bug #chrrm (bugzilla is down). 
 
 #6396 
 
 Ok, thanks.
 
 An attempt to attack it (together with repoUpdate implementation) is 
 attached...

Pavel, can you have a look please?
If the principle way is ok I can do the same for the RCS and SVN backend too.

One question regarding the View log button of repoUpdate: 
here on Mac the dialog to display the log is unusable. 
It is blocked by the next confirmation dialog. 
Is this platform specific or on all platforms?

Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
 Pavel, can you have a look please?

hopefully towards the end of this week. actually it would be really helpful
if we can proceed in usual incremental manner - different patches for different
issues, its much harder to keep your track if everything is muddled into
one big patch, ie patch for CVS related stuff and fixing other bugs on the top 
of it.

quick peek into diff - doVCCommand change looks ok (maybe call the param 
reportError?).
why 
+   // to be sure test readonly state again...  

   
+   FileName fn(owner_-absFileName()); 

   
+   fn.refresh();   

   
+   if (!fn.isReadOnly())
is not inside checkoutenabled?

just fast screening, i really need to go sleep now, sorry ;)

 If the principle way is ok I can do the same for the RCS and SVN backend too.
 
 One question regarding the View log button of repoUpdate: 
 here on Mac the dialog to display the log is unusable. 
 It is blocked by the next confirmation dialog. 
 Is this platform specific or on all platforms?

no, this glitch is on all platforms (iirc resize did work, but the button was
silent). i carry the idea of trying to show that dialog as nonmodal. it maybe
oneliner somewhere, but i never really get to solve it...

pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 19.10.2010 um 03:34 schrieb Pavel Sanda:

 Stephan Witt wrote:
 Pavel, can you have a look please?
 
 hopefully towards the end of this week. actually it would be really helpful
 if we can proceed in usual incremental manner - different patches for 
 different
 issues, its much harder to keep your track if everything is muddled into
 one big patch, ie patch for CVS related stuff and fixing other bugs on the 
 top of it.

I don't know how to do that. I have only one LyX source tree in compilable 
state.
I'll try to make two patches out of it.

 
 quick peek into diff - doVCCommand change looks ok (maybe call the param 
 reportError?).
 why 
 +   // to be sure test readonly state again...
   

 +   FileName fn(owner_-absFileName());   
   

 +   fn.refresh(); 
   

 +   if (!fn.isReadOnly())
 is not inside checkoutenabled?

I thought checkoutEnabled() is called too often.
 
 
 just fast screening, i really need to go sleep now, sorry ;)

Thank you for answering me.

 If the principle way is ok I can do the same for the RCS and SVN backend too.
 
 One question regarding the View log button of repoUpdate: 
 here on Mac the dialog to display the log is unusable. 
 It is blocked by the next confirmation dialog. 
 Is this platform specific or on all platforms?
 
 no, this glitch is on all platforms (iirc resize did work, but the button was
 silent).

On Mac it's blocked completely.

 i carry the idea of trying to show that dialog as nonmodal. it maybe
 oneliner somewhere, but i never really get to solve it...

Ok.

Stephan

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 17.10.2010 um 21:00 schrieb Stephan Witt:

> Am 15.10.2010 um 21:28 schrieb Stephan Witt:
> 
>> Am 15.10.2010 um 18:00 schrieb Pavel Sanda:
>> 
>>> Pavel Sanda wrote:
 this is already bug #chrrm (bugzilla is down). 
>>> 
>>> #6396 
>> 
>> Ok, thanks.
> 
> An attempt to attack it (together with repoUpdate implementation) is 
> attached...

Pavel, can you have a look please?
If the principle way is ok I can do the same for the RCS and SVN backend too.

One question regarding the "View log" button of repoUpdate: 
here on Mac the dialog to display the log is unusable. 
It is blocked by the next confirmation dialog. 
Is this platform specific or on all platforms?

Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
> Pavel, can you have a look please?

hopefully towards the end of this week. actually it would be really helpful
if we can proceed in usual incremental manner - different patches for different
issues, its much harder to keep your track if everything is muddled into
one big patch, ie patch for CVS related stuff and fixing other bugs on the top 
of it.

quick peek into diff - doVCCommand change looks ok (maybe call the param 
reportError?).
why 
+   // to be sure test readonly state again...  

   
+   FileName fn(owner_->absFileName()); 

   
+   fn.refresh();   

   
+   if (!fn.isReadOnly())
is not inside checkoutenabled?

just fast screening, i really need to go sleep now, sorry ;)

> If the principle way is ok I can do the same for the RCS and SVN backend too.
> 
> One question regarding the "View log" button of repoUpdate: 
> here on Mac the dialog to display the log is unusable. 
> It is blocked by the next confirmation dialog. 
> Is this platform specific or on all platforms?

no, this glitch is on all platforms (iirc resize did work, but the button was
silent). i carry the idea of trying to show that dialog as nonmodal. it maybe
oneliner somewhere, but i never really get to solve it...

pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 19.10.2010 um 03:34 schrieb Pavel Sanda:

> Stephan Witt wrote:
>> Pavel, can you have a look please?
> 
> hopefully towards the end of this week. actually it would be really helpful
> if we can proceed in usual incremental manner - different patches for 
> different
> issues, its much harder to keep your track if everything is muddled into
> one big patch, ie patch for CVS related stuff and fixing other bugs on the 
> top of it.

I don't know how to do that. I have only one LyX source tree in compilable 
state.
I'll try to make two patches out of it.

> 
> quick peek into diff - doVCCommand change looks ok (maybe call the param 
> reportError?).
> why 
> +   // to be sure test readonly state again...
>   
>
> +   FileName fn(owner_->absFileName());   
>   
>
> +   fn.refresh(); 
>   
>
> +   if (!fn.isReadOnly())
> is not inside checkoutenabled?

I thought checkoutEnabled() is called too often.
 
> 
> just fast screening, i really need to go sleep now, sorry ;)

Thank you for answering me.

>> If the principle way is ok I can do the same for the RCS and SVN backend too.
>> 
>> One question regarding the "View log" button of repoUpdate: 
>> here on Mac the dialog to display the log is unusable. 
>> It is blocked by the next confirmation dialog. 
>> Is this platform specific or on all platforms?
> 
> no, this glitch is on all platforms (iirc resize did work, but the button was
> silent).

On Mac it's blocked completely.

> i carry the idea of trying to show that dialog as nonmodal. it maybe
> oneliner somewhere, but i never really get to solve it...

Ok.

Stephan

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 15.10.2010 um 21:28 schrieb Stephan Witt:

 Am 15.10.2010 um 18:00 schrieb Pavel Sanda:
 
 Pavel Sanda wrote:
 this is already bug #chrrm (bugzilla is down). 
 
 #6396 
 
 Ok, thanks.

An attempt to attack it (together with repoUpdate implementation) is attached...

I tried to make the implementation for CVS as example with minimal intrusion 
upon the VC code base.
That's why I added the two functions checkInWithMessage() and 
revertWithConfirmation().
The more ambitious solution would be to add the diff query operation for all VC 
backends and act on the outcome.
That would be a somewhat bigger change than the proposed patch.

The change to doVCCommand() is needed because of cvs diff unfortunately exits 
with 1 if a diff is detected.
(As documented in cvs man page.)

I'm not sure if the revert() for unchanged files should happen without 
confirmation or with a modified question. 

Stephan

Index: src/VCBackend.cpp
===
--- src/VCBackend.cpp   (Revision 35671)
+++ src/VCBackend.cpp   (Arbeitskopie)
@@ -47,7 +47,7 @@
 }
 
 
-int VCS::doVCCommand(string const  cmd, FileName const  path)
+int VCS::doVCCommand(string const  cmd, FileName const  path, bool 
reportStatus)
 {
if (owner_)
owner_-setBusy(true);
@@ -56,7 +56,7 @@
 
if (owner_)
owner_-setBusy(false);
-   if (ret)
+   if (ret  reportStatus)
frontend::Alert::error(_(Revision control error.),
bformat(_(Some problem occured while running the 
command:\n
  '%1$s'.),
@@ -402,21 +402,14 @@
 
//sm[4]; // options
//sm[5]; // tag or tagdate
-   // FIXME: must double check file is stattable/existing
-   time_t mod = file_.lastModified();
-   string mod_date = rtrim(asctime(gmtime(mod)), \n);
-   LYXERR(Debug::LYXVC, Date in Entries: `  file_date
-'\nModification date of file: `  
mod_date  '\'');
-   //FIXME this whole locking bussiness is not working 
under cvs and the machinery
-   // conforms to the ci usage, not cvs.
-   if (file_date == mod_date) {
-   locker_ = Unlocked;
-   vcstatus = UNLOCKED;
+   if (file_.isReadableFile()) {
+   time_t mod = file_.lastModified();
+   string mod_date = rtrim(asctime(gmtime(mod)), 
\n);
+   LYXERR(Debug::LYXVC, Date in Entries: `  
file_date
+'\nModification date of file: `  
mod_date  '\'');
+   vcstatus = file_.isReadOnly() ? UNLOCKED : 
LOCKED;
} else {
-   // Here we should also do some more checking
-   // to see if there are conflicts or not.
-   locker_ = Locked;
-   vcstatus = LOCKED;
+   vcstatus = NOLOCKING;
}
break;
}
@@ -432,12 +425,81 @@
 }
 
 
+string const CVS::getTarget(OperationMode opmode)
+{
+   switch(opmode) {
+   case Directory:
+   return quoteName(owner_-filePath());
+   case File:
+   return quoteName(onlyFileName(owner_-absFileName()));
+   }
+   return string();
+}
+
+
+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;
+}
+
+
+bool const CVS::hasDiff(OperationMode opmode)
+{
+   FileName tmpf = getDiff(opmode);
+   docstring res = tmpf.fileContents(UTF-8);
+   return !res.empty();
+}
+
+
+int CVS::edit()
+{
+   vcstatus = LOCKED;
+   return doVCCommand(cvs -q edit 
+   + quoteName(onlyFileName(owner_-absFileName())),
+   FileName(owner_-filePath()));
+}
+
+
+int CVS::unedit()
+{
+   vcstatus = UNLOCKED;
+   return doVCCommand(cvs -q unedit 
+   + quoteName(onlyFileName(owner_-absFileName())),
+   FileName(owner_-filePath()));
+}
+
+
+int CVS::update(OperationMode opmode, FileName const tmpf)
+{
+   string const redirection = tmpf.empty() ? 
+   :+ quoteName(tmpf.toFilesystemEncoding());
+
+   return doVCCommand(cvs -q update 
+   + getTarget(opmode) + redirection,
+   

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 15.10.2010 um 21:28 schrieb Stephan Witt:

> Am 15.10.2010 um 18:00 schrieb Pavel Sanda:
> 
>> Pavel Sanda wrote:
>>> this is already bug #chrrm (bugzilla is down). 
>> 
>> #6396 
> 
> Ok, thanks.

An attempt to attack it (together with repoUpdate implementation) is attached...

I tried to make the implementation for CVS as example with minimal intrusion 
upon the VC code base.
That's why I added the two functions checkInWithMessage() and 
revertWithConfirmation().
The more ambitious solution would be to add the diff query operation for all VC 
backends and act on the outcome.
That would be a somewhat bigger change than the proposed patch.

The change to doVCCommand() is needed because of "cvs diff" unfortunately exits 
with 1 if a diff is detected.
(As documented in cvs man page.)

I'm not sure if the revert() for unchanged files should happen without 
confirmation or with a modified question. 

Stephan

Index: src/VCBackend.cpp
===
--- src/VCBackend.cpp   (Revision 35671)
+++ src/VCBackend.cpp   (Arbeitskopie)
@@ -47,7 +47,7 @@
 }
 
 
-int VCS::doVCCommand(string const & cmd, FileName const & path)
+int VCS::doVCCommand(string const & cmd, FileName const & path, bool 
reportStatus)
 {
if (owner_)
owner_->setBusy(true);
@@ -56,7 +56,7 @@
 
if (owner_)
owner_->setBusy(false);
-   if (ret)
+   if (ret && reportStatus)
frontend::Alert::error(_("Revision control error."),
bformat(_("Some problem occured while running the 
command:\n"
  "'%1$s'."),
@@ -402,21 +402,14 @@
 
//sm[4]; // options
//sm[5]; // tag or tagdate
-   // FIXME: must double check file is stattable/existing
-   time_t mod = file_.lastModified();
-   string mod_date = rtrim(asctime(gmtime()), "\n");
-   LYXERR(Debug::LYXVC, "Date in Entries: `" << file_date
-   << "'\nModification date of file: `" << 
mod_date << '\'');
-   //FIXME this whole locking bussiness is not working 
under cvs and the machinery
-   // conforms to the ci usage, not cvs.
-   if (file_date == mod_date) {
-   locker_ = "Unlocked";
-   vcstatus = UNLOCKED;
+   if (file_.isReadableFile()) {
+   time_t mod = file_.lastModified();
+   string mod_date = rtrim(asctime(gmtime()), 
"\n");
+   LYXERR(Debug::LYXVC, "Date in Entries: `" << 
file_date
+   << "'\nModification date of file: `" << 
mod_date << '\'');
+   vcstatus = file_.isReadOnly() ? UNLOCKED : 
LOCKED;
} else {
-   // Here we should also do some more checking
-   // to see if there are conflicts or not.
-   locker_ = "Locked";
-   vcstatus = LOCKED;
+   vcstatus = NOLOCKING;
}
break;
}
@@ -432,12 +425,81 @@
 }
 
 
+string const CVS::getTarget(OperationMode opmode)
+{
+   switch(opmode) {
+   case Directory:
+   return quoteName(owner_->filePath());
+   case File:
+   return quoteName(onlyFileName(owner_->absFileName()));
+   }
+   return string();
+}
+
+
+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;
+}
+
+
+bool const CVS::hasDiff(OperationMode opmode)
+{
+   FileName tmpf = getDiff(opmode);
+   docstring res = tmpf.fileContents("UTF-8");
+   return !res.empty();
+}
+
+
+int CVS::edit()
+{
+   vcstatus = LOCKED;
+   return doVCCommand("cvs -q edit "
+   + quoteName(onlyFileName(owner_->absFileName())),
+   FileName(owner_->filePath()));
+}
+
+
+int CVS::unedit()
+{
+   vcstatus = UNLOCKED;
+   return doVCCommand("cvs -q unedit "
+   + quoteName(onlyFileName(owner_->absFileName())),
+   FileName(owner_->filePath()));
+}
+
+
+int CVS::update(OperationMode opmode, FileName const tmpf)
+{
+   string const redirection = tmpf.empty() ? ""
+   : " > " + quoteName(tmpf.toFilesystemEncoding());
+
+   return doVCCommand("cvs -q update "
+ 

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
 The owner_ variable points to the buffer, of course. 
 Most of the time VC implementation uses the filename of the buffer.
 But I cannot see what sense the file_ variable of the VC instance has.

intuitively the meaning is clear - when possible use owner. when buffer
might/does not exist use filename. there are two cases i can imagine
where buffer does not exist - calling scanMaster before loading the
file and when we reload the document. reload has been refactorized
during 2.0svn development and iirc buffer structure is not destructed now.
whether scanMaster is actually called before buffer creation i would
need to check. otherwise i wouldnt see need for file_.

 One can drop it - or use it consequently - I'd say.
  
  i would need to dig into the code if you want some
  specific info.
 
 If possible... that would be nice.

i quickly went through sources and saw also few usage of file_ outside
scanMaster in svn routines which looks inconsitent and owner_ would be
more appropriate.

pavel

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Pavel Sanda wrote:
 i quickly went through sources and saw also few usage of file_ outside
 scanMaster in svn routines which looks inconsitent and owner_ would be
 more appropriate.

no as i see it again they are part of scanMaster so the question really is 
whether
this routine can be called without existing buffer...

pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 15.10.2010 um 15:08 schrieb Pavel Sanda:

 Pavel Sanda wrote:
 i quickly went through sources and saw also few usage of file_ outside
 scanMaster in svn routines which looks inconsitent and owner_ would be
 more appropriate.
 
 no as i see it again they are part of scanMaster so the question really is 
 whether
 this routine can be called without existing buffer...

AFAICS not. scanMaster() gets called from constructors only.
The constructors are in LyXVC::registrer() which gets called from buffer
and LyXVC::file_found_hook() - called when buffer file is readable.

So I think file_ is used as internal state to transfer the constructor
parameter to scanMaster. Since it's not really broken - I'll refrain
from fixing it.

But in general owner_-fileName() should be used instead of file_.

Another question: when the working copy has no diff to repository
the check in operation isn't sensible. But it's expensive to test
this. So I'd like to test it before prompting for the log message
and then doing the noop check in. Does this sound sensible?
The proper solution would be to refactor the diff operation and
use it for that and for repoUpdate in the check for it and prompt
when a real change was made.
If you're fine with that I'll prepare a patch otherwise I have to
copy the diff test code to CVS::checkIn.

Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
 But in general owner_-fileName() should be used instead of file_.

yes

 Another question: when the working copy has no diff to repository
 the check in operation isn't sensible. But it's expensive to test
 this. So I'd like to test it before prompting for the log message
 and then doing the noop check in. Does this sound sensible?

this is already bug #chrrm (bugzilla is down). i was about to fix it few months
back but there was some catch (hard to remember, maybe because checkin can
release locks only or something like that), so i postponed it ;)

i agree this should be fixed but dont have time recheck that all other rcs/svn
usecases are not affected, so this testing would be up to you.

pavel

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Pavel Sanda wrote:
 this is already bug #chrrm (bugzilla is down). 

#6396 

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 15.10.2010 um 18:00 schrieb Pavel Sanda:

 Pavel Sanda wrote:
 this is already bug #chrrm (bugzilla is down). 
 
 #6396 

Ok, thanks.

Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
> The owner_ variable "points" to the buffer, of course. 
> Most of the time VC implementation uses the filename of the buffer.
> But I cannot see what sense the file_ variable of the VC instance has.

intuitively the meaning is clear - when possible use owner. when buffer
might/does not exist use filename. there are two cases i can imagine
where buffer does not exist - calling scanMaster before loading the
file and when we reload the document. reload has been refactorized
during 2.0svn development and iirc buffer structure is not destructed now.
whether scanMaster is actually called before buffer creation i would
need to check. otherwise i wouldnt see need for file_.

> One can drop it - or use it consequently - I'd say.
>  
> > i would need to dig into the code if you want some
> > specific info.
> 
> If possible... that would be nice.

i quickly went through sources and saw also few usage of file_ outside
scanMaster in svn routines which looks inconsitent and owner_ would be
more appropriate.

pavel

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Pavel Sanda wrote:
> i quickly went through sources and saw also few usage of file_ outside
> scanMaster in svn routines which looks inconsitent and owner_ would be
> more appropriate.

no as i see it again they are part of scanMaster so the question really is 
whether
this routine can be called without existing buffer...

pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 15.10.2010 um 15:08 schrieb Pavel Sanda:

> Pavel Sanda wrote:
>> i quickly went through sources and saw also few usage of file_ outside
>> scanMaster in svn routines which looks inconsitent and owner_ would be
>> more appropriate.
> 
> no as i see it again they are part of scanMaster so the question really is 
> whether
> this routine can be called without existing buffer...

AFAICS not. scanMaster() gets called from constructors only.
The constructors are in LyXVC::registrer() which gets called from buffer
and LyXVC::file_found_hook() - called when buffer file is readable.

So I think file_ is used as internal state to transfer the constructor
parameter to scanMaster. Since it's not really broken - I'll refrain
from fixing it.

But in general owner_->fileName() should be used instead of file_.

Another question: when the working copy has no diff to repository
the check in operation isn't sensible. But it's expensive to test
this. So I'd like to test it before prompting for the log message
and then doing the noop check in. Does this sound sensible?
The proper solution would be to refactor the diff operation and
use it for that and for repoUpdate in the check for it and prompt
when a real change was made.
If you're fine with that I'll prepare a patch otherwise I have to
copy the diff test code to CVS::checkIn.

Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
> But in general owner_->fileName() should be used instead of file_.

yes

> Another question: when the working copy has no diff to repository
> the check in operation isn't sensible. But it's expensive to test
> this. So I'd like to test it before prompting for the log message
> and then doing the noop check in. Does this sound sensible?

this is already bug #chrrm (bugzilla is down). i was about to fix it few months
back but there was some catch (hard to remember, maybe because checkin can
release locks only or something like that), so i postponed it ;)

i agree this should be fixed but dont have time recheck that all other rcs/svn
usecases are not affected, so this testing would be up to you.

pavel

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Pavel Sanda wrote:
> this is already bug #chrrm (bugzilla is down). 

#6396 

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 15.10.2010 um 18:00 schrieb Pavel Sanda:

> Pavel Sanda wrote:
>> this is already bug #chrrm (bugzilla is down). 
> 
> #6396 

Ok, thanks.

Stephan

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 08.10.2010 um 16:37 schrieb Stephan Witt:

 Am 08.10.2010 um 16:19 schrieb Pavel Sanda:
 
 Stephan Witt wrote:
 but i still dont like we dont care about this consitently. the solution 
 would be to push
 this conde into lyxvc.
 
 I wouldn't vote against such move. :-)
 
 can you move it please?
 
 Yes, I do it when working on the backport of my changes.

I have a general question about the VCBackend code.
The CVS and SVN classes have a file_ member variable.
Am I right that this is the reference to the owner_ file?
Most of the time the file of the owner_ is used and sometimes
it's file_... Is it useful to have both of them?
What should be preferred if it is useful?

 
 please can you update our additional manual with the current state of art
 for CVS and also suggest typical lyx use cases for cvs regime? mean cvs
 user has probaly no idea about cvs edit and so on.
 
 also add some note into revision control item in newinlyx20 wiki...
 
 This I can do. I'm on vacation next two weeks. Some time should be 
 available...

Currently I have less time then I expected.
But it's not forgotten.

Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
  Yes, I do it when working on the backport of my changes.
 
 I have a general question about the VCBackend code.
 The CVS and SVN classes have a file_ member variable.
 Am I right that this is the reference to the owner_ file?
 Most of the time the file of the owner_ is used and sometimes
 it's file_... Is it useful to have both of them?
 What should be preferred if it is useful?

owner_ is buffer - something opened in lyx; file_ is just filename
which exist independently whether the file is opened, closed, reloaded
whatsoever... i would need to dig into the code if you want some
specific info.

pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 14.10.2010 um 22:43 schrieb Pavel Sanda:

 Stephan Witt wrote:
 Yes, I do it when working on the backport of my changes.
 
 I have a general question about the VCBackend code.
 The CVS and SVN classes have a file_ member variable.
 Am I right that this is the reference to the owner_ file?
 Most of the time the file of the owner_ is used and sometimes
 it's file_... Is it useful to have both of them?
 What should be preferred if it is useful?
 
 owner_ is buffer - something opened in lyx; file_ is just filename
 which exist independently whether the file is opened, closed, reloaded
 whatsoever...

The owner_ variable points to the buffer, of course. 
Most of the time VC implementation uses the filename of the buffer.
But I cannot see what sense the file_ variable of the VC instance has.
One can drop it - or use it consequently - I'd say.
 
 i would need to dig into the code if you want some
 specific info.

If possible... that would be nice.

Stephan

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 08.10.2010 um 16:37 schrieb Stephan Witt:

> Am 08.10.2010 um 16:19 schrieb Pavel Sanda:
> 
>> Stephan Witt wrote:
 but i still dont like we dont care about this consitently. the solution 
 would be to push
 this conde into lyxvc.
>>> 
>>> I wouldn't vote against such move. :-)
>> 
>> can you move it please?
> 
> Yes, I do it when working on the backport of my changes.

I have a general question about the VCBackend code.
The CVS and SVN classes have a file_ member variable.
Am I right that this is the reference to the owner_ file?
Most of the time the file of the owner_ is used and sometimes
it's file_... Is it useful to have both of them?
What should be preferred if it is useful?

>> 
 please can you update our additional manual with the current state of art
 for CVS and also suggest typical lyx use cases for cvs regime? mean cvs
 user has probaly no idea about cvs edit and so on.
 
 also add some note into revision control item in newinlyx20 wiki...
>>> 
>>> This I can do. I'm on vacation next two weeks. Some time should be 
>>> available...

Currently I have less time then I expected.
But it's not forgotten.

Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
> > Yes, I do it when working on the backport of my changes.
> 
> I have a general question about the VCBackend code.
> The CVS and SVN classes have a file_ member variable.
> Am I right that this is the reference to the owner_ file?
> Most of the time the file of the owner_ is used and sometimes
> it's file_... Is it useful to have both of them?
> What should be preferred if it is useful?

owner_ is buffer - something opened in lyx; file_ is just filename
which exist independently whether the file is opened, closed, reloaded
whatsoever... i would need to dig into the code if you want some
specific info.

pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 14.10.2010 um 22:43 schrieb Pavel Sanda:

> Stephan Witt wrote:
>>> Yes, I do it when working on the backport of my changes.
>> 
>> I have a general question about the VCBackend code.
>> The CVS and SVN classes have a file_ member variable.
>> Am I right that this is the reference to the owner_ file?
>> Most of the time the file of the owner_ is used and sometimes
>> it's file_... Is it useful to have both of them?
>> What should be preferred if it is useful?
> 
> owner_ is buffer - something opened in lyx; file_ is just filename
> which exist independently whether the file is opened, closed, reloaded
> whatsoever...

The owner_ variable "points" to the buffer, of course. 
Most of the time VC implementation uses the filename of the buffer.
But I cannot see what sense the file_ variable of the VC instance has.
One can drop it - or use it consequently - I'd say.
 
> i would need to dig into the code if you want some
> specific info.

If possible... that would be nice.

Stephan

Re: r35575 - lyx-devel/trunk/src

[Abdelrazak Younes]

On 08/10/2010 15:58, Edwin Leuven wrote:

Pavel Sandasa...@lyx.org  wrote:
   

this makes big sense and was already proposed by Abdel.
 

i always like abdel's ideas...
   


Thanks ;-)

The advantage of git (or any other distributed vcs) is of course that 
you don't need a server. The full history of the lyx file as well as the 
embedded objects would be within the git-lyx document itself. A bit like 
Microsoft embedded versioning but much more powerful.
On Windows we could package git.exe with the installer easily, like we 
do for python.




i have no time for this project though.
 

pity

could we add this to

http://www.lyx.org/Donate

and announce it on lyx-users?

i could make a first contribution to get it going...
   


I also really have no time for LyX development, unfortunately, but I 
would help any interested developer. Vincent had already expressed some 
interested in the lyx embedded format and this project would nicely 
complement his previous compare project :-)


Vincent, what about your thesis? Hope you managed to complete it 
successfully.


Cheers,
Abdel.

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Abdelrazak Younes wrote:
 On Windows we could package git.exe with the installer easily, like we do 
 for python.

maybe even include some subset in our sources as we do with boost... 
we shouldn't be dependent on the development of git tools to be able
to read our own files...

pavel

Re: r35575 - lyx-devel/trunk/src

[Abdelrazak Younes]

On 08/10/2010 15:58, Edwin Leuven wrote:

Pavel Sanda  wrote:
   

this makes big sense and was already proposed by Abdel.
 

i always like abdel's ideas...
   


Thanks ;-)

The advantage of git (or any other distributed vcs) is of course that 
you don't need a server. The full history of the lyx file as well as the 
embedded objects would be within the git-lyx document itself. A bit like 
Microsoft embedded versioning but much more powerful.
On Windows we could package git.exe with the installer easily, like we 
do for python.




i have no time for this project though.
 

pity

could we add this to

http://www.lyx.org/Donate

and announce it on lyx-users?

i could make a first contribution to get it going...
   


I also really have no time for LyX development, unfortunately, but I 
would help any interested developer. Vincent had already expressed some 
interested in the lyx embedded format and this project would nicely 
complement his previous compare project :-)


Vincent, what about your thesis? Hope you managed to complete it 
successfully.


Cheers,
Abdel.

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Abdelrazak Younes wrote:
> On Windows we could package git.exe with the installer easily, like we do 
> for python.

maybe even include some subset in our sources as we do with boost... 
we shouldn't be dependent on the development of git tools to be able
to read our own files...

pavel

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

sw...@lyx.org wrote:
 Author: switt
 Date: Fri Oct  8 07:40:16 2010
 New Revision: 35575
 URL: http://www.lyx.org/trac/changeset/35575
 
 Log:
 add implementation for CVS::checkOut

few comments...

  
  
  string CVS::checkOut()
  {
 - // cvs update or perhaps for cvs this should be a noop
 - // we need to detect conflict (eg C in output)
 - // before we can do this.
 - lyxerr  Sorry, not implemented.  endl;
 - return string();
 + // to be sure we test it again...
 + if (!checkOutEnabled())
 + return string();

why we are not sure here? we should be consistent in all routines.

 +
 + int ret = doVCCommand(cvs -q edit 
 +   + 
 quoteName(onlyFileName(owner_-absFileName())),
 +   FileName(owner_-filePath()));

i would like to understand the usecase here.
shouldn'be there some unedit in the commit case? 
this mimicks locking in order to avoid conflicts?

pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 08.10.2010 um 14:06 schrieb Pavel Sanda:

 sw...@lyx.org wrote:
 Author: switt
 Date: Fri Oct  8 07:40:16 2010
 New Revision: 35575
 URL: http://www.lyx.org/trac/changeset/35575
 
 Log:
 add implementation for CVS::checkOut
 
 few comments...
 
 
 
 string CVS::checkOut()
 {
 -// cvs update or perhaps for cvs this should be a noop
 -// we need to detect conflict (eg C in output)
 -// before we can do this.
 -lyxerr  Sorry, not implemented.  endl;
 -return string();
 +// to be sure we test it again...
 +if (!checkOutEnabled())
 +return string();
 
 why we are not sure here? we should be consistent in all routines.

Because it is not sure if the checkout is enabled when doing the checkout.
You test the enabled state when the menu is used. 
You cannot know the time difference from opening the menu and the begin of the 
checkout.
If somehow there is an checkout done already in the meantime you are in 
trouble.
That's what I want to avoid.

 +
 +int ret = doVCCommand(cvs -q edit 
 +  + 
 quoteName(onlyFileName(owner_-absFileName())),
 +  FileName(owner_-filePath()));
 
 i would like to understand the usecase here.
 shouldn'be there some unedit in the commit case?

No, the commit restores the readonly state.

But you can use cvs unedit to drop your local uncommited changes.
This I didn't want to implement in revert() because I didn't want to assume the
average user is working like me. But perhaps it as a good alternative. I have to
think about that.

 this mimicks locking in order to avoid conflicts?

Not in our case. You may implement that on the server side.
We only want to avoid the change of some file unintentionally.

As a bonus you can query the list of the current editors with cvs editors.
It's possible to edit some file by multiple editors and to commit the changes.
But the second user has to use cvs update before cvs commit.

That's the case anyway. And is currently not supported with LyX.


Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
  +  // to be sure we test it again...
  +  if (!checkOutEnabled())
  +  return string();
  
  why we are not sure here? we should be consistent in all routines.
 
 Because it is not sure if the checkout is enabled when doing the checkout.
 You test the enabled state when the menu is used. 
 You cannot know the time difference from opening the menu and the begin of 
 the checkout.
 If somehow there is an checkout done already in the meantime you are in 
 trouble.
 That's what I want to avoid.

aha, you meant some 3rd party script playing tricks in the mean time, right?
but i still dont like we dont care about this consitently. the solution would 
be to push
this conde into lyxvc.

  i would like to understand the usecase here.
  shouldn'be there some unedit in the commit case?
 
 No, the commit restores the readonly state.
 
 But you can use cvs unedit to drop your local uncommited changes.
 This I didn't want to implement in revert() because I didn't want to assume 
 the
 average user is working like me. But perhaps it as a good alternative. I have 
 to
 think about that.
 
  this mimicks locking in order to avoid conflicts?
 
 Not in our case.

hmm. what about to add some routine to detect conflict like in svn case?

 As a bonus you can query the list of the current editors with cvs editors.
 It's possible to edit some file by multiple editors and to commit the changes.
 But the second user has to use cvs update before cvs commit.
 
 That's the case anyway. And is currently not supported with LyX.

please can you update our additional manual with the current state of art
for CVS and also suggest typical lyx use cases for cvs regime? mean cvs
user has probaly no idea about cvs edit and so on.

also add some note into revision control item in newinlyx20 wiki...

pavel

Re: r35575 - lyx-devel/trunk/src

[Edwin Leuven]

it would be nice to have the possibility to store the zipped svn
repository of the lyx document itself in the lyx file. it would be
activated with a store history item in the menu just like we have
compressed now.

when opening this document lyx would unzip the repository in the
tempdir, and commit eventual changes by spanning an svn process that
commits on a save. closing the document zips the stuff and copies it
back to the original .lyxsvn

this way non-power users can have the benefits of svn -- namely access
to the revision history of their document + the interface to look at
the changes and eventually roll back -- without going through the
trouble of understanding svn (it's just open/save/close) and setting
up an svn server.

it would also have the advantage of easily carrying around the history
of your document in a single file

don't know whether this makes sense, but i would use it...

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Edwin Leuven wrote:
 don't know whether this makes sense, but i would use it...

this makes big sense and was already proposed by Abdel.
the only difference was to use git for it.
it would also solve embedded format feature...

i have no time for this project though.

pavel

Re: r35575 - lyx-devel/trunk/src

[Edwin Leuven]

Pavel Sanda sa...@lyx.org wrote:
 this makes big sense and was already proposed by Abdel.

i always like abdel's ideas...

 i have no time for this project though.

pity

could we add this to

http://www.lyx.org/Donate

and announce it on lyx-users?

i could make a first contribution to get it going...

ed.

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 08.10.2010 um 15:35 schrieb Pavel Sanda:

 Stephan Witt wrote:
 +  // to be sure we test it again...
 +  if (!checkOutEnabled())
 +  return string();
 
 why we are not sure here? we should be consistent in all routines.
 
 Because it is not sure if the checkout is enabled when doing the checkout.
 You test the enabled state when the menu is used. 
 You cannot know the time difference from opening the menu and the begin of 
 the checkout.
 If somehow there is an checkout done already in the meantime you are in 
 trouble.
 That's what I want to avoid.
 
 aha, you meant some 3rd party script playing tricks in the mean time, right?

I only want to be sure.
It may be a network drive and another user is doing the same, or whatever.
If the checkout is enabled at this point the additional test doesn't hurt.
If it is not... so it's better to avoid the checkout.
And this is the operation with the potential for data loss.

But to work reliable the test in checkOutEnabled() should test the file 
readonly state
and not the LyX buffer state.

 but i still dont like we dont care about this consitently. the solution would 
 be to push
 this conde into lyxvc.

I wouldn't vote against such move. :-)

 i would like to understand the usecase here.
 shouldn'be there some unedit in the commit case?
 
 No, the commit restores the readonly state.
 
 But you can use cvs unedit to drop your local uncommited changes.
 This I didn't want to implement in revert() because I didn't want to assume 
 the
 average user is working like me. But perhaps it as a good alternative. I 
 have to
 think about that.
 
 this mimicks locking in order to avoid conflicts?
 
 Not in our case.
 
 hmm. what about to add some routine to detect conflict like in svn case?

Where is the conflict?

The conflict is when more than one user is changing the *same part* of the 
document.
I don't know if e. g. an user adding an subsection to a document automatically 
changes
the whole document. I don't think so, but didn't test it.

The conflict is detected later by cvs on cvs update.

I would propose to make the refresh from repository incorporating the changes 
working.
When a conflict arises the user has to solve that manually.
E. g. saving the own version as copy and refresh from repository discarding the 
current
changes after this step.

 As a bonus you can query the list of the current editors with cvs editors.
 It's possible to edit some file by multiple editors and to commit the 
 changes.
 But the second user has to use cvs update before cvs commit.
 
 That's the case anyway. And is currently not supported with LyX.

The solution for that I've outlined above.

 please can you update our additional manual with the current state of art
 for CVS and also suggest typical lyx use cases for cvs regime? mean cvs
 user has probaly no idea about cvs edit and so on.
 
 also add some note into revision control item in newinlyx20 wiki...

This I can do. I'm on vacation next two weeks. Some time should be available...

Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Edwin Leuven wrote:
  i have no time for this project though.
 
 pity

btw for single files only, this sort of works already when you use RCS...

 could we add this to
 
 http://www.lyx.org/Donate
 
 and announce it on lyx-users?
 

i think before we offer something we should know who is responsible for coding
it once we collect the money. but once somebody acks the task we can.

 i could make a first contribution to get it going...

you also could be the one who code it... :)

pavel

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
  but i still dont like we dont care about this consitently. the solution 
  would be to push
  this conde into lyxvc.
 
 I wouldn't vote against such move. :-)

can you move it please?

 Where is the conflict?
 
 The conflict is when more than one user is changing the *same part* of the 
 document.
...
 The conflict is detected later by cvs on cvs update.

yes. i meant something like what we currently do with svn:

cvs update  file.log
if contains(file.log,^C )
dialog(Error when updating from repository, You have to manually 
resolve the conflicts NOW!)

but its up to you. the current state is better than it was.

  please can you update our additional manual with the current state of art
  for CVS and also suggest typical lyx use cases for cvs regime? mean cvs
  user has probaly no idea about cvs edit and so on.
  
  also add some note into revision control item in newinlyx20 wiki...
 
 This I can do. I'm on vacation next two weeks. Some time should be 
 available...

thanks.
pavel

Re: r35575 - lyx-devel/trunk/src

[Edwin Leuven]

Pavel Sanda sa...@lyx.org wrote:
 btw for single files only, this sort of works already when you use RCS...

i wouldn't give me a single file with the history, and i would need to
use svn. right?

 i think before we offer something we should know who is responsible for coding
 it once we collect the money. but once somebody acks the task we can.

ok, let's see whether someone is tempted

 i could make a first contribution to get it going...

 you also could be the one who code it... :)

unfortunately i have more money than time...

ed.

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Edwin Leuven wrote:
 Pavel Sanda sa...@lyx.org wrote:
  btw for single files only, this sort of works already when you use RCS...
 
 i wouldn't give me a single file with the history, and i would need to
 use svn. right?

no. you need to have gnu rcs installed (http://www.gnu.org/software/rcs/).
then next by your own file.lyx there is created file.lyx,v with the
full history. this is nicely connected with lyx gui, so you can use
update/commit, in 2.0 even compare with history.

pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 08.10.2010 um 16:19 schrieb Pavel Sanda:

 Stephan Witt wrote:
 but i still dont like we dont care about this consitently. the solution 
 would be to push
 this conde into lyxvc.
 
 I wouldn't vote against such move. :-)
 
 can you move it please?

Yes, I do it when working on the backport of my changes.
I didn't do that first because of you said the API should remain stable.

 Where is the conflict?
 
 The conflict is when more than one user is changing the *same part* of the 
 document.
 ...
 The conflict is detected later by cvs on cvs update.
 
 yes. i meant something like what we currently do with svn:
 
 cvs update  file.log
 if contains(file.log,^C )
   dialog(Error when updating from repository, You have to manually 
 resolve the conflicts NOW!)
 
 but its up to you. the current state is better than it was.

Yes, thanks. I read the SVN solution already and was thinking of doing 
something similar...
I think it would make a big difference for collaborative work.

 
 please can you update our additional manual with the current state of art
 for CVS and also suggest typical lyx use cases for cvs regime? mean cvs
 user has probaly no idea about cvs edit and so on.
 
 also add some note into revision control item in newinlyx20 wiki...
 
 This I can do. I'm on vacation next two weeks. Some time should be 
 available...
 
 thanks.
 pavel

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

sw...@lyx.org wrote:
> Author: switt
> Date: Fri Oct  8 07:40:16 2010
> New Revision: 35575
> URL: http://www.lyx.org/trac/changeset/35575
> 
> Log:
> add implementation for CVS::checkOut

few comments...

>  
>  
>  string CVS::checkOut()
>  {
> - // cvs update or perhaps for cvs this should be a noop
> - // we need to detect conflict (eg "C" in output)
> - // before we can do this.
> - lyxerr << "Sorry, not implemented." << endl;
> - return string();
> + // to be sure we test it again...
> + if (!checkOutEnabled())
> + return string();

why we are not sure here? we should be consistent in all routines.

> +
> + int ret = doVCCommand("cvs -q edit "
> +   + 
> quoteName(onlyFileName(owner_->absFileName())),
> +   FileName(owner_->filePath()));

i would like to understand the usecase here.
shouldn'be there some unedit in the commit case? 
this mimicks locking in order to avoid conflicts?

pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 08.10.2010 um 14:06 schrieb Pavel Sanda:

> sw...@lyx.org wrote:
>> Author: switt
>> Date: Fri Oct  8 07:40:16 2010
>> New Revision: 35575
>> URL: http://www.lyx.org/trac/changeset/35575
>> 
>> Log:
>> add implementation for CVS::checkOut
> 
> few comments...
> 
>> 
>> 
>> string CVS::checkOut()
>> {
>> -// cvs update or perhaps for cvs this should be a noop
>> -// we need to detect conflict (eg "C" in output)
>> -// before we can do this.
>> -lyxerr << "Sorry, not implemented." << endl;
>> -return string();
>> +// to be sure we test it again...
>> +if (!checkOutEnabled())
>> +return string();
> 
> why we are not sure here? we should be consistent in all routines.

Because it is not sure if the checkout is enabled when doing the checkout.
You test the enabled state when the menu is used. 
You cannot know the time difference from opening the menu and the begin of the 
checkout.
If somehow there is an "checkout" done already in the meantime you are in 
trouble.
That's what I want to avoid.

>> +
>> +int ret = doVCCommand("cvs -q edit "
>> +  + 
>> quoteName(onlyFileName(owner_->absFileName())),
>> +  FileName(owner_->filePath()));
> 
> i would like to understand the usecase here.
> shouldn'be there some unedit in the commit case?

No, the commit restores the readonly state.

But you can use cvs unedit to drop your local uncommited changes.
This I didn't want to implement in revert() because I didn't want to assume the
average user is working like me. But perhaps it as a good alternative. I have to
think about that.

> this mimicks locking in order to avoid conflicts?

Not in our case. You may implement that on the server side.
We only want to avoid the change of some file unintentionally.

As a bonus you can query the list of the current editors with "cvs editors".
It's possible to edit some file by multiple editors and to commit the changes.
But the second user has to use "cvs update" before "cvs commit".

That's the case anyway. And is currently not supported with LyX.


Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
> >> +  // to be sure we test it again...
> >> +  if (!checkOutEnabled())
> >> +  return string();
> > 
> > why we are not sure here? we should be consistent in all routines.
> 
> Because it is not sure if the checkout is enabled when doing the checkout.
> You test the enabled state when the menu is used. 
> You cannot know the time difference from opening the menu and the begin of 
> the checkout.
> If somehow there is an "checkout" done already in the meantime you are in 
> trouble.
> That's what I want to avoid.

aha, you meant some 3rd party script playing tricks in the mean time, right?
but i still dont like we dont care about this consitently. the solution would 
be to push
this conde into lyxvc.

> > i would like to understand the usecase here.
> > shouldn'be there some unedit in the commit case?
> 
> No, the commit restores the readonly state.
> 
> But you can use cvs unedit to drop your local uncommited changes.
> This I didn't want to implement in revert() because I didn't want to assume 
> the
> average user is working like me. But perhaps it as a good alternative. I have 
> to
> think about that.
> 
> > this mimicks locking in order to avoid conflicts?
> 
> Not in our case.

hmm. what about to add some routine to detect conflict like in svn case?

> As a bonus you can query the list of the current editors with "cvs editors".
> It's possible to edit some file by multiple editors and to commit the changes.
> But the second user has to use "cvs update" before "cvs commit".
> 
> That's the case anyway. And is currently not supported with LyX.

please can you update our additional manual with the current state of art
for CVS and also suggest typical lyx use cases for cvs regime? mean cvs
user has probaly no idea about cvs edit and so on.

also add some note into revision control item in newinlyx20 wiki...

pavel

Re: r35575 - lyx-devel/trunk/src

[Edwin Leuven]

it would be nice to have the possibility to store the zipped svn
repository of the lyx document itself in the lyx file. it would be
activated with a "store history" item in the menu just like we have
"compressed" now.

when opening this document lyx would unzip the repository in the
tempdir, and commit eventual changes by spanning an svn process that
commits on a save. closing the document zips the stuff and copies it
back to the original .lyxsvn

this way non-power users can have the benefits of svn -- namely access
to the revision history of their document + the interface to look at
the changes and eventually roll back -- without going through the
trouble of understanding svn (it's just open/save/close) and setting
up an svn server.

it would also have the advantage of easily carrying around the history
of your document in a single file

don't know whether this makes sense, but i would use it...

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Edwin Leuven wrote:
> don't know whether this makes sense, but i would use it...

this makes big sense and was already proposed by Abdel.
the only difference was to use git for it.
it would also solve embedded format feature...

i have no time for this project though.

pavel

Re: r35575 - lyx-devel/trunk/src

[Edwin Leuven]

Pavel Sanda  wrote:
> this makes big sense and was already proposed by Abdel.

i always like abdel's ideas...

> i have no time for this project though.

pity

could we add this to

http://www.lyx.org/Donate

and announce it on lyx-users?

i could make a first contribution to get it going...

ed.

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 08.10.2010 um 15:35 schrieb Pavel Sanda:

> Stephan Witt wrote:
 +  // to be sure we test it again...
 +  if (!checkOutEnabled())
 +  return string();
>>> 
>>> why we are not sure here? we should be consistent in all routines.
>> 
>> Because it is not sure if the checkout is enabled when doing the checkout.
>> You test the enabled state when the menu is used. 
>> You cannot know the time difference from opening the menu and the begin of 
>> the checkout.
>> If somehow there is an "checkout" done already in the meantime you are in 
>> trouble.
>> That's what I want to avoid.
> 
> aha, you meant some 3rd party script playing tricks in the mean time, right?

I only want to be sure.
It may be a network drive and another user is doing the same, or whatever.
If the checkout is enabled at this point the additional test doesn't hurt.
If it is not... so it's better to avoid the checkout.
And this is the operation with the potential for data loss.

But to work reliable the test in checkOutEnabled() should test the file 
readonly state
and not the LyX buffer state.

> but i still dont like we dont care about this consitently. the solution would 
> be to push
> this conde into lyxvc.

I wouldn't vote against such move. :-)

>>> i would like to understand the usecase here.
>>> shouldn'be there some unedit in the commit case?
>> 
>> No, the commit restores the readonly state.
>> 
>> But you can use cvs unedit to drop your local uncommited changes.
>> This I didn't want to implement in revert() because I didn't want to assume 
>> the
>> average user is working like me. But perhaps it as a good alternative. I 
>> have to
>> think about that.
>> 
>>> this mimicks locking in order to avoid conflicts?
>> 
>> Not in our case.
> 
> hmm. what about to add some routine to detect conflict like in svn case?

Where is the conflict?

The conflict is when more than one user is changing the *same part* of the 
document.
I don't know if e. g. an user adding an subsection to a document automatically 
changes
the whole document. I don't think so, but didn't test it.

The conflict is detected later by cvs on "cvs update".

I would propose to make the refresh from repository incorporating the changes 
working.
When a conflict arises the user has to solve that manually.
E. g. saving the own version as copy and refresh from repository discarding the 
current
changes after this step.

>> As a bonus you can query the list of the current editors with "cvs editors".
>> It's possible to edit some file by multiple editors and to commit the 
>> changes.
>> But the second user has to use "cvs update" before "cvs commit".
>> 
>> That's the case anyway. And is currently not supported with LyX.

The solution for that I've outlined above.

> please can you update our additional manual with the current state of art
> for CVS and also suggest typical lyx use cases for cvs regime? mean cvs
> user has probaly no idea about cvs edit and so on.
> 
> also add some note into revision control item in newinlyx20 wiki...

This I can do. I'm on vacation next two weeks. Some time should be available...

Stephan

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Edwin Leuven wrote:
> > i have no time for this project though.
> 
> pity

btw for single files only, this sort of works already when you use RCS...

> could we add this to
> 
> http://www.lyx.org/Donate
> 
> and announce it on lyx-users?
> 

i think before we offer something we should know who is responsible for coding
it once we collect the money. but once somebody acks the task we can.

> i could make a first contribution to get it going...

you also could be the one who code it... :)

pavel

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Stephan Witt wrote:
> > but i still dont like we dont care about this consitently. the solution 
> > would be to push
> > this conde into lyxvc.
> 
> I wouldn't vote against such move. :-)

can you move it please?

> Where is the conflict?
> 
> The conflict is when more than one user is changing the *same part* of the 
> document.
...
> The conflict is detected later by cvs on "cvs update".

yes. i meant something like what we currently do with svn:

cvs update > file.log
if contains(file.log,"^C ")
dialog("Error when updating from repository, You have to manually 
resolve the conflicts NOW!")

but its up to you. the current state is better than it was.

> > please can you update our additional manual with the current state of art
> > for CVS and also suggest typical lyx use cases for cvs regime? mean cvs
> > user has probaly no idea about cvs edit and so on.
> > 
> > also add some note into revision control item in newinlyx20 wiki...
> 
> This I can do. I'm on vacation next two weeks. Some time should be 
> available...

thanks.
pavel

Re: r35575 - lyx-devel/trunk/src

[Edwin Leuven]

Pavel Sanda  wrote:
> btw for single files only, this sort of works already when you use RCS...

i wouldn't give me a single file with the history, and i would need to
use svn. right?

> i think before we offer something we should know who is responsible for coding
> it once we collect the money. but once somebody acks the task we can.

ok, let's see whether someone is tempted

>> i could make a first contribution to get it going...
>
> you also could be the one who code it... :)

unfortunately i have more money than time...

ed.

Re: r35575 - lyx-devel/trunk/src

[Pavel Sanda]

Edwin Leuven wrote:
> Pavel Sanda  wrote:
> > btw for single files only, this sort of works already when you use RCS...
> 
> i wouldn't give me a single file with the history, and i would need to
> use svn. right?

no. you need to have gnu rcs installed (http://www.gnu.org/software/rcs/).
then next by your own "file.lyx" there is created "file.lyx,v" with the
full history. this is nicely connected with lyx gui, so you can use
update/commit, in 2.0 even compare with history.

pavel

Re: r35575 - lyx-devel/trunk/src

[Stephan Witt]

Am 08.10.2010 um 16:19 schrieb Pavel Sanda:

> Stephan Witt wrote:
>>> but i still dont like we dont care about this consitently. the solution 
>>> would be to push
>>> this conde into lyxvc.
>> 
>> I wouldn't vote against such move. :-)
> 
> can you move it please?

Yes, I do it when working on the backport of my changes.
I didn't do that first because of you said the API should remain stable.

>> Where is the conflict?
>> 
>> The conflict is when more than one user is changing the *same part* of the 
>> document.
> ...
>> The conflict is detected later by cvs on "cvs update".
> 
> yes. i meant something like what we currently do with svn:
> 
> cvs update > file.log
> if contains(file.log,"^C ")
>   dialog("Error when updating from repository, You have to manually 
> resolve the conflicts NOW!")
> 
> but its up to you. the current state is better than it was.

Yes, thanks. I read the SVN solution already and was thinking of doing 
something similar...
I think it would make a big difference for collaborative work.

> 
>>> please can you update our additional manual with the current state of art
>>> for CVS and also suggest typical lyx use cases for cvs regime? mean cvs
>>> user has probaly no idea about cvs edit and so on.
>>> 
>>> also add some note into revision control item in newinlyx20 wiki...
>> 
>> This I can do. I'm on vacation next two weeks. Some time should be 
>> available...
> 
> thanks.
> pavel