Re: 2.3.0alpha1-1: file save

[Guillaume MM]

Le 14/05/2017 à 19:48, Stephan Witt a écrit :

Am 13.05.2017 um 08:19 schrieb Guillaume MM :


Le 11/05/2017 à 09:59, Stephan Witt a écrit :


The delay is defined in Buffer::Impl::blockFileMonitor (Buffer.cpp:388)
currently at 10ms. Can you try to increase this value and see if that
helps? It will likely help, but this is not a nice solution.


I tried it with a delay of 100ms. The effect is now LyX presents the external
modification message box every 2nd or 3rd save. So it is not the solution.


Thanks for the test, this helps. BTW is there a delay before the message
appears and if so how long? Do you see the following error message with
the files debug flag:
 LYXERR(Debug::FILES, "Could not add path to QFileSystemWatcher: " <<
filename_);
?


This is the output of the un-patched LyX on save with false-positive message:

support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile12.lyx" 3874605062 lasted 1 ms.
support/TempFile.cpp (35): Temporary file in 
/Users/stephan/Documents/newfile12-XX.lyx
support/TempFile.cpp (38): Temporary file 
`/Users/stephan/Documents/newfile12-P37773.lyx' created.
Buffer.cpp (1435): Saving to /Users/stephan/Documents/newfile12-P37773.lyx
BufferParams.cpp (321): Checking whether document is in a system dir... no
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile12.lyx" 3874605062 lasted 1 ms.
Buffer.cpp (1461): Backing up original file to 
/Users/stephan/Documents/newfile12.lyx~
support/FileName.cpp (267): Moving /Users/stephan/Documents/newfile12.lyx~ to 
/Users/stephan/Documents/newfile12.lyx
support/FileName.cpp (267): Moving /Users/stephan/Documents/newfile12.lyx to 
/Users/stephan/Documents/newfile12-P37773.lyx
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile12.lyx" 2474078819 lasted 1 ms.


Thanks, this is as expected. Can I ask again, is there a delay between
saving and the time when the notification appears, and if so how long?


Can you please try the attached patch?


This is the output of the patched one - no false-positive message:
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile13.lyx" 2101337338 lasted 0 ms.
support/TempFile.cpp (35): Temporary file in 
/Users/stephan/Documents/newfile13-XX.lyx
support/TempFile.cpp (38): Temporary file 
`/Users/stephan/Documents/newfile13-B38676.lyx' created.
Buffer.cpp (1435): Saving to /Users/stephan/Documents/newfile13-B38676.lyx
BufferParams.cpp (321): Checking whether document is in a system dir... no
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile13.lyx" 2101337338 lasted 1 ms.
Buffer.cpp (1461): Backing up original file to 
/Users/stephan/Documents/newfile13.lyx~
support/FileName.cpp (267): Moving /Users/stephan/Documents/newfile13.lyx~ to 
/Users/stephan/Documents/newfile13.lyx
support/FileName.cpp (267): Moving /Users/stephan/Documents/newfile13.lyx to 
/Users/stephan/Documents/newfile13-B38676.lyx
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile13.lyx" 834715601 lasted 2 ms.
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile13.lyx" 834715601 lasted 0 ms.



Thanks for testing. Again the trace is as expected. Good to know that it
works.



And this is for a big document (on a SSD based machine):

support/FileName.cpp (605): Checksumming 
"/Users/stephan/git/lyx/lib/doc/de/UserGuide.lyx" 1327983642 lasted 35 ms.
support/TempFile.cpp (35): Temporary file in 
/Users/stephan/git/lyx/lib/doc/de/UserGuide-XX.lyx
support/TempFile.cpp (38): Temporary file 
`/Users/stephan/git/lyx/lib/doc/de/UserGuide-X38676.lyx' created.
Buffer.cpp (1435): Saving to 
/Users/stephan/git/lyx/lib/doc/de/UserGuide-X38676.lyx
BufferParams.cpp (315): Checking whether document is in a system dir... yes
support/FileName.cpp (605): Checksumming 
"/Users/stephan/git/lyx/lib/doc/de/UserGuide.lyx" 1327983642 lasted 37 ms.
Buffer.cpp (1461): Backing up original file to 
/Users/stephan/git/lyx/lib/doc/de/UserGuide-lyxformat-541.lyx~
support/FileName.cpp (267): Moving 
/Users/stephan/git/lyx/lib/doc/de/UserGuide-lyxformat-541.lyx~ to 
/Users/stephan/git/lyx/lib/doc/de/UserGuide.lyx
support/FileName.cpp (267): Moving 
/Users/stephan/git/lyx/lib/doc/de/UserGuide.lyx to 
/Users/stephan/git/lyx/lib/doc/de/UserGuide-X38676.lyx
support/FileName.cpp (605): Checksumming 
"/Users/stephan/git/lyx/lib/doc/de/UserGuide.lyx" 582248824 lasted 35 ms.
support/FileName.cpp (605): Checksumming 
"/Users/stephan/git/lyx/lib/doc/de/UserGuide.lyx" 582248824 lasted 37 ms.



Good to know that the time to check the sum is reasonable for a big 
document. For slow file systems one can expect the OS cache to help in 
this precise situation.


The best solution would be to reimplement the saving with QSaveFile
+ Safer save mechanism
- Not sure it fixes the bug
- Requires some work

Alternatively, the patch that you just tested:
+ Fixes the bug and probably #10642 too
? Only warns if the 

Re: 2.3.0alpha1-1: file save

[Stephan Witt]

Am 13.05.2017 um 08:19 schrieb Guillaume MM :
> 
> Le 11/05/2017 à 09:59, Stephan Witt a écrit :
 
 The delay is defined in Buffer::Impl::blockFileMonitor (Buffer.cpp:388)
 currently at 10ms. Can you try to increase this value and see if that
 helps? It will likely help, but this is not a nice solution.
>> 
>> I tried it with a delay of 100ms. The effect is now LyX presents the external
>> modification message box every 2nd or 3rd save. So it is not the solution.
> 
> Thanks for the test, this helps. BTW is there a delay before the message
> appears and if so how long? Do you see the following error message with
> the files debug flag:
>  LYXERR(Debug::FILES, "Could not add path to QFileSystemWatcher: " <<
> filename_);
> ?

This is the output of the un-patched LyX on save with false-positive message:

support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile12.lyx" 3874605062 lasted 1 ms.
support/TempFile.cpp (35): Temporary file in 
/Users/stephan/Documents/newfile12-XX.lyx
support/TempFile.cpp (38): Temporary file 
`/Users/stephan/Documents/newfile12-P37773.lyx' created.
Buffer.cpp (1435): Saving to /Users/stephan/Documents/newfile12-P37773.lyx
BufferParams.cpp (321): Checking whether document is in a system dir... no
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile12.lyx" 3874605062 lasted 1 ms.
Buffer.cpp (1461): Backing up original file to 
/Users/stephan/Documents/newfile12.lyx~
support/FileName.cpp (267): Moving /Users/stephan/Documents/newfile12.lyx~ to 
/Users/stephan/Documents/newfile12.lyx
support/FileName.cpp (267): Moving /Users/stephan/Documents/newfile12.lyx to 
/Users/stephan/Documents/newfile12-P37773.lyx
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile12.lyx" 2474078819 lasted 1 ms.

 Ideally I would like to have everything work with a delay of 0ms, to be
 sure that everyone experiences the same. Can you help me and see if it
 is possible to flush the file operations in FileName::copyTo and
 FileName::moveTo and if it makes a difference ? There is QFile::flush
 but I do not really see how to use it in this context and I cannot test
 the situation.
>> 
>> I cannot see how to do it here.
>> 
>> The save operation in this scenario is done with one create (temp), one
>> rename (backup) and another rename (final name).
>> 
>> Are you sure the file monitor is able to follow these steps?
> 
> What is expected to happen is:
> 
> In Buffer::save:
> 
> 1. Disconnect from QFileSystemWatcher
> 2. Perform various file operations
> 3. Queue the reconnection to QFileSystemWatcher
> 
> Later:
> 
> 4. QFileSystemWatcher notifies of the deletion
> 5. The new file is moved
> 6. Reconnect to QFileSystemWatcher and refresh (watch the new file)
> 
> Given the behaviour that you report above, it seems that the
> reconnection happens too early. The number and the nature of the
> operations is not important.
> 
> I notice that the QSaveFile class which is provided by Qt (but not used
> in LyX) calls a function fileEngine->syncToDisk() not accessible from
> the API, and below there are the comments:
> // atomically replace old file with new file
> // Can't use QFile::rename for that, must use the file engine directly
> 
> I suspect that using QSaveFile instead of the current implementation
> would solve the bug and make file saving safer (even).
> 
> Can you please try the attached patch?

This is the output of the patched one - no false-positive message:
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile13.lyx" 2101337338 lasted 0 ms.
support/TempFile.cpp (35): Temporary file in 
/Users/stephan/Documents/newfile13-XX.lyx
support/TempFile.cpp (38): Temporary file 
`/Users/stephan/Documents/newfile13-B38676.lyx' created.
Buffer.cpp (1435): Saving to /Users/stephan/Documents/newfile13-B38676.lyx
BufferParams.cpp (321): Checking whether document is in a system dir... no
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile13.lyx" 2101337338 lasted 1 ms.
Buffer.cpp (1461): Backing up original file to 
/Users/stephan/Documents/newfile13.lyx~
support/FileName.cpp (267): Moving /Users/stephan/Documents/newfile13.lyx~ to 
/Users/stephan/Documents/newfile13.lyx
support/FileName.cpp (267): Moving /Users/stephan/Documents/newfile13.lyx to 
/Users/stephan/Documents/newfile13-B38676.lyx
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile13.lyx" 834715601 lasted 2 ms.
support/FileName.cpp (605): Checksumming 
"/Users/stephan/Documents/newfile13.lyx" 834715601 lasted 0 ms.

And this is for a big document (on a SSD based machine):

support/FileName.cpp (605): Checksumming 
"/Users/stephan/git/lyx/lib/doc/de/UserGuide.lyx" 1327983642 lasted 35 ms.
support/TempFile.cpp (35): Temporary file in 
/Users/stephan/git/lyx/lib/doc/de/UserGuide-XX.lyx
support/TempFile.cpp (38): Temporary file 

Re: 2.3.0alpha1-1: file save

[Guillaume MM]

Le 11/05/2017 à 07:46, Stephan Witt a écrit :


On the first save LyX asks for a file name because it’s a new file.
This in fact is a Save-As operation. The save succeeds normal.
After that every change+save is accompanied by the message popup.
NB: I’m accepting the file name proposal - so it’s the same file name
before and after the save-as.


Thanks.




Guillaume, it’s the FileMonitor which is detecting the file save as external 
modification.
How can I provide further info to correct this annoying behavior?
My attempt to diagnose the culprit failed - the code is too weird for me :(

I tried to stop in Buffer::Impl::refreshFileMonitor() at the first line.
The debugger didn’t stop - but it stopped in 
Buffer::Impl::fileExternallyModified()
and the call stack claims it comes from the last line in refreshFileMonitor() 
???


As QFileSystemWatcher is intrinsically asynchronous, gdb does not really
help with debugging here. What you see on the trace is
QFileSystemWatcher calling the function that was passed to connect at
the end of refreshFileMonitor (via a qt signal that is converted into a
boost signal).


That makes sense. BTW, what’s the semantic of a void C++ function when
it returns a value (2nd line after the if())? I’m surprised it’s allowed.


I am not surprised that it is allowed (void is a type and here it
type-checks), but indeed this is not how I write usually. This
looks like something that used to return a bool at some point. I'll
correct it.

Guillaume

Re: 2.3.0alpha1-1: file save

[Guillaume MM]

Le 11/05/2017 à 09:59, Stephan Witt a écrit :


The delay is defined in Buffer::Impl::blockFileMonitor (Buffer.cpp:388)
currently at 10ms. Can you try to increase this value and see if that
helps? It will likely help, but this is not a nice solution.


I tried it with a delay of 100ms. The effect is now LyX presents the external
modification message box every 2nd or 3rd save. So it is not the solution.


Thanks for the test, this helps. BTW is there a delay before the message
appears and if so how long? Do you see the following error message with
the files debug flag:
  LYXERR(Debug::FILES, "Could not add path to QFileSystemWatcher: " <<
filename_);
?





Ideally I would like to have everything work with a delay of 0ms, to be
sure that everyone experiences the same. Can you help me and see if it
is possible to flush the file operations in FileName::copyTo and
FileName::moveTo and if it makes a difference ? There is QFile::flush
but I do not really see how to use it in this context and I cannot test
the situation.


I cannot see how to do it here.

The save operation in this scenario is done with one create (temp), one
rename (backup) and another rename (final name).

Are you sure the file monitor is able to follow these steps?


What is expected to happen is:

In Buffer::save:

1. Disconnect from QFileSystemWatcher
2. Perform various file operations
3. Queue the reconnection to QFileSystemWatcher

Later:

4. QFileSystemWatcher notifies of the deletion
5. The new file is moved
6. Reconnect to QFileSystemWatcher and refresh (watch the new file)

Given the behaviour that you report above, it seems that the
reconnection happens too early. The number and the nature of the
operations is not important.

I notice that the QSaveFile class which is provided by Qt (but not used
in LyX) calls a function fileEngine->syncToDisk() not accessible from
the API, and below there are the comments:
// atomically replace old file with new file
// Can't use QFile::rename for that, must use the file engine directly

I suspect that using QSaveFile instead of the current implementation
would solve the bug and make file saving safer (even).

Can you please try the attached patch? Before deciding which solution is
the best I will still need to know the answer to the questions at the
beginning, if you please can help with that.

Thank you
Guillaume

>From 17f644c2cc1c21020aa71215762941930689f951 Mon Sep 17 00:00:00 2001
From: Guillaume MM 
Date: Sat, 13 May 2017 01:00:30 +0200
Subject: [PATCH] Prevent false positives in external modifications

When the Buffer is notified to be externally modified, check that the
file contents have changed using the checksum.
---
 src/Buffer.cpp | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 239bacd..60619dc 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -386,7 +386,7 @@ public:
 	void fileExternallyModified(bool modified) const;
 
 	/// Block notifications of external modifications
-	FileMonitorBlocker blockFileMonitor() { return file_monitor_->block(10); }
+	FileMonitorBlocker blockFileMonitor() { return file_monitor_->block(); }
 
 private:
 	/// So we can force access via the accessors.
@@ -5340,8 +5340,11 @@ void Buffer::Impl::refreshFileMonitor()
 
 void Buffer::Impl::fileExternallyModified(bool modified) const
 {
-	if (modified)
+	if (modified) {
+		if (filename.exists() && checksum_ == filename.checksum())
+			return;
 		lyx_clean = bak_clean = false;
+	}
 	externally_modified_ = modified;
 	if (wa_)
 		wa_->updateTitles();
-- 
2.7.4

Re: 2.3.0alpha1-1: file save

[Stephan Witt]

Am 11.05.2017 um 07:46 schrieb Stephan Witt :
> 
> Am 11.05.2017 um 00:40 schrieb Guillaume MM :
>> 
>> Le 10/05/2017 à 22:57, Stephan Witt a écrit :
>>> Am 09.05.2017 um 21:19 schrieb john kennan :
 
 Start a new file
 type something
 save
 type some more
 save
 
 I get "the file ... changed on disk." with two buttons: "Reload" and 
 "Ignore"
 
 hitting Ignore leaves the file unsaved
 hitting Reload works -- I get a warning:
 
 Any changes will be lost. Are you sure you want to load the version on 
 disk of the document .../Papers/LyX/test1.lyx?
 
 with buttons for Revert and Cancel
 Hitting Revert saves the file, as desired, and the changes are not lost.
>> 
>> Dear John, thanks for the report.
> 
> +1 - thank you for testing!
> 
>> This looks like LyX incorrectly
>> recording the modification it has made itself as an external
>> modification. This means that in fact the file is already correctly
>> saved when the message shows up, and the two buttons do nothing except
>> annoy. I am looking into it.
>> 
>> Does it happen only on the second save? Does it happen on the third save
>> if you carry on? Does it not happen on the first save? If not, is it
>> because the first save is a "Save As"?
>> 
>>> 
>>> Confirmed.
>> 
>> Dear Stephan, thanks for the test. This looks specific to OSX. I would
>> ask the same questions as above.
> 
> On the first save LyX asks for a file name because it’s a new file.
> This in fact is a Save-As operation. The save succeeds normal.
> After that every change+save is accompanied by the message popup.
> NB: I’m accepting the file name proposal - so it’s the same file name
> before and after the save-as.
> 
>>> Guillaume, it’s the FileMonitor which is detecting the file save as 
>>> external modification.
>>> How can I provide further info to correct this annoying behavior?
>>> My attempt to diagnose the culprit failed - the code is too weird for me :(
>>> 
>>> I tried to stop in Buffer::Impl::refreshFileMonitor() at the first line.
>>> The debugger didn’t stop - but it stopped in 
>>> Buffer::Impl::fileExternallyModified()
>>> and the call stack claims it comes from the last line in 
>>> refreshFileMonitor() ???
>> 
>> As QFileSystemWatcher is intrinsically asynchronous, gdb does not really
>> help with debugging here. What you see on the trace is
>> QFileSystemWatcher calling the function that was passed to connect at
>> the end of refreshFileMonitor (via a qt signal that is converted into a
>> boost signal).
> 
> That makes sense. BTW, what’s the semantic of a void C++ function when
> it returns a value (2nd line after the if())? I’m surprised it’s allowed.
> 
> Now I have to go… I’ll answer the other questions later.
> 
> Stephan
> 
>> What should happen is inside Buffer::save there is a FileMonitorBlocker
>> that should block the signal. Now, since QFileSystemWatcher is
>> asynchronous, the signal is not received before the next iteration of
>> the even loop. At this point the FileMonitorBlocker has been destroyed a
>> long time ago. This is why FileMonitorBlocker unblocks the signal in an
>> asynchronous way too, using a QTimer. On Linux, a delay of 0 (meaning
>> wait until the next event loop) is enough to actually block the signal.
>> 
>> The delay is defined in Buffer::Impl::blockFileMonitor (Buffer.cpp:388)
>> currently at 10ms. Can you try to increase this value and see if that
>> helps? It will likely help, but this is not a nice solution.

I tried it with a delay of 100ms. The effect is now LyX presents the external
modification message box every 2nd or 3rd save. So it is not the solution.

>> Ideally I would like to have everything work with a delay of 0ms, to be
>> sure that everyone experiences the same. Can you help me and see if it
>> is possible to flush the file operations in FileName::copyTo and
>> FileName::moveTo and if it makes a difference ? There is QFile::flush
>> but I do not really see how to use it in this context and I cannot test
>> the situation.

I cannot see how to do it here.

The save operation in this scenario is done with one create (temp), one
rename (backup) and another rename (final name).

Are you sure the file monitor is able to follow these steps?

>> 
>>> 
>>> Frankly said: I’m lost now.
>> 
>> For this bug and the other similar bug on the bugtracker [down currently
>> again] I thought about detecting false positives by comparing the file
>> hashes before setting the flag.

Perhaps the monitor should be informed/synchronized by/with the save process?

Stephan
>> 
>> Before this drastic measure I would like to see if I can understand why
>> the signal comes too late for the blocker, and if it's just missing
>> something simple.
>> 
>> 
>> Guillaume

Re: 2.3.0alpha1-1: file save

[Stephan Witt]

Am 11.05.2017 um 00:40 schrieb Guillaume MM :
> 
> Le 10/05/2017 à 22:57, Stephan Witt a écrit :
>> Am 09.05.2017 um 21:19 schrieb john kennan :
>>> 
>>> Start a new file
>>> type something
>>> save
>>> type some more
>>> save
>>> 
>>> I get "the file ... changed on disk." with two buttons: "Reload" and 
>>> "Ignore"
>>> 
>>> hitting Ignore leaves the file unsaved
>>> hitting Reload works -- I get a warning:
>>> 
>>> Any changes will be lost. Are you sure you want to load the version on disk 
>>> of the document .../Papers/LyX/test1.lyx?
>>> 
>>> with buttons for Revert and Cancel
>>> Hitting Revert saves the file, as desired, and the changes are not lost.
> 
> Dear John, thanks for the report.

+1 - thank you for testing!

> This looks like LyX incorrectly
> recording the modification it has made itself as an external
> modification. This means that in fact the file is already correctly
> saved when the message shows up, and the two buttons do nothing except
> annoy. I am looking into it.
> 
> Does it happen only on the second save? Does it happen on the third save
> if you carry on? Does it not happen on the first save? If not, is it
> because the first save is a "Save As"?
> 
>> 
>> Confirmed.
> 
> Dear Stephan, thanks for the test. This looks specific to OSX. I would
> ask the same questions as above.

On the first save LyX asks for a file name because it’s a new file.
This in fact is a Save-As operation. The save succeeds normal.
After that every change+save is accompanied by the message popup.
NB: I’m accepting the file name proposal - so it’s the same file name
before and after the save-as.
 
>> Guillaume, it’s the FileMonitor which is detecting the file save as external 
>> modification.
>> How can I provide further info to correct this annoying behavior?
>> My attempt to diagnose the culprit failed - the code is too weird for me :(
>> 
>> I tried to stop in Buffer::Impl::refreshFileMonitor() at the first line.
>> The debugger didn’t stop - but it stopped in 
>> Buffer::Impl::fileExternallyModified()
>> and the call stack claims it comes from the last line in 
>> refreshFileMonitor() ???
> 
> As QFileSystemWatcher is intrinsically asynchronous, gdb does not really
> help with debugging here. What you see on the trace is
> QFileSystemWatcher calling the function that was passed to connect at
> the end of refreshFileMonitor (via a qt signal that is converted into a
> boost signal).

That makes sense. BTW, what’s the semantic of a void C++ function when
it returns a value (2nd line after the if())? I’m surprised it’s allowed.

Now I have to go… I’ll answer the other questions later.

Stephan

> What should happen is inside Buffer::save there is a FileMonitorBlocker
> that should block the signal. Now, since QFileSystemWatcher is
> asynchronous, the signal is not received before the next iteration of
> the even loop. At this point the FileMonitorBlocker has been destroyed a
> long time ago. This is why FileMonitorBlocker unblocks the signal in an
> asynchronous way too, using a QTimer. On Linux, a delay of 0 (meaning
> wait until the next event loop) is enough to actually block the signal.
> 
> The delay is defined in Buffer::Impl::blockFileMonitor (Buffer.cpp:388)
> currently at 10ms. Can you try to increase this value and see if that
> helps? It will likely help, but this is not a nice solution.
> 
> Ideally I would like to have everything work with a delay of 0ms, to be
> sure that everyone experiences the same. Can you help me and see if it
> is possible to flush the file operations in FileName::copyTo and
> FileName::moveTo and if it makes a difference ? There is QFile::flush
> but I do not really see how to use it in this context and I cannot test
> the situation.
> 
>> 
>> Frankly said: I’m lost now.
> 
> For this bug and the other similar bug on the bugtracker [down currently
> again] I thought about detecting false positives by comparing the file
> hashes before setting the flag.
> 
> Before this drastic measure I would like to see if I can understand why
> the signal comes too late for the blocker, and if it's just missing
> something simple.
> 
> 
> Guillaume

Re: 2.3.0alpha1-1: file save

[Richard Heck]

On 05/10/2017 04:57 PM, Stephan Witt wrote:
> Am 09.05.2017 um 21:19 schrieb john kennan :
>> Start a new file
>> type something
>> save
>> type some more
>> save
>>
>> I get "the file ... changed on disk." with two buttons: "Reload" and "Ignore"
>>
>> hitting Ignore leaves the file unsaved
>> hitting Reload works -- I get a warning:
>>
>> Any changes will be lost. Are you sure you want to load the version on disk 
>> of the document .../Papers/LyX/test1.lyx?
>>
>> with buttons for Revert and Cancel
>> Hitting Revert saves the file, as desired, and the changes are not lost.
> Confirmed.

I reported a similar issue in connection with bug #10295, though it was
intermittent there.

Richard

Re: 2.3.0alpha1-1: file save

[john kennan]

On Wed, May 10, 2017 at 6:33 PM, john kennan  wrote:

>
>
> On Wed, May 10, 2017 at 5:40 PM, Guillaume MM  wrote:
>
>> Le 10/05/2017 à 22:57, Stephan Witt a écrit :
>>
>>> Am 09.05.2017 um 21:19 schrieb john kennan :
>>>

 Start a new file
 type something
 save
 type some more
 save

 I get "the file ... changed on disk." with two buttons: "Reload" and
 "Ignore"

 hitting Ignore leaves the file unsaved
 hitting Reload works -- I get a warning:

 Any changes will be lost. Are you sure you want to load the version on
 disk of the document .../Papers/LyX/test1.lyx?

 with buttons for Revert and Cancel
 Hitting Revert saves the file, as desired, and the changes are not lost.

>>>
>> Dear John, thanks for the report. This looks like LyX incorrectly
>> recording the modification it has made itself as an external
>> modification. This means that in fact the file is already correctly
>> saved when the message shows up, and the two buttons do nothing except
>> annoy. I am looking into it.
>>
>> Does it happen only on the second save? Does it happen on the third save
>> if you carry on? Does it not happen on the first save? If not, is it
>> because the first save is a "Save As"?
>>
>>
> It does not happen using "Save As" with a new filename.
> It does continue to happen on the third save.
> The save apparently works (as you suggested), regardless of which button
> is used, but when quitting the file, there is a (false) warning that there
> are unsaved changes.
> If the Reload button is used, followed by Revert, quitting the file gives
> no warning (and indeed there are no unsaved changes).
>
> After making unsaved changes, the title bar should show filename
> (changed). This doesn't work.
> But if two files are open in separate tabs, there is an asterisk after the
> filename with unsaved changes (the normal behavior).
> And in this case, when the Reload and Ignore buttons show up, a tiny
> danger triangle appears after the asterisk.
>
> John
>
>
>
>
>>> Confirmed.
>>>
>>
>> Dear Stephan, thanks for the test. This looks specific to OSX. I would
>> ask the same questions as above.
>>
>>
>>> Guillaume, it’s the FileMonitor which is detecting the file save as
>>> external modification.
>>> How can I provide further info to correct this annoying behavior?
>>> My attempt to diagnose the culprit failed - the code is too weird for me
>>> :(
>>>
>>> I tried to stop in Buffer::Impl::refreshFileMonitor() at the first line.
>>> The debugger didn’t stop - but it stopped in
>>> Buffer::Impl::fileExternallyModified()
>>> and the call stack claims it comes from the last line in
>>> refreshFileMonitor() ???
>>>
>>
>> As QFileSystemWatcher is intrinsically asynchronous, gdb does not really
>> help with debugging here. What you see on the trace is
>> QFileSystemWatcher calling the function that was passed to connect at
>> the end of refreshFileMonitor (via a qt signal that is converted into a
>> boost signal).
>>
>> What should happen is inside Buffer::save there is a FileMonitorBlocker
>> that should block the signal. Now, since QFileSystemWatcher is
>> asynchronous, the signal is not received before the next iteration of
>> the even loop. At this point the FileMonitorBlocker has been destroyed a
>> long time ago. This is why FileMonitorBlocker unblocks the signal in an
>> asynchronous way too, using a QTimer. On Linux, a delay of 0 (meaning
>> wait until the next event loop) is enough to actually block the signal.
>>
>> The delay is defined in Buffer::Impl::blockFileMonitor (Buffer.cpp:388)
>> currently at 10ms. Can you try to increase this value and see if that
>> helps? It will likely help, but this is not a nice solution.
>>
>> Ideally I would like to have everything work with a delay of 0ms, to be
>> sure that everyone experiences the same. Can you help me and see if it
>> is possible to flush the file operations in FileName::copyTo and
>> FileName::moveTo and if it makes a difference ? There is QFile::flush
>> but I do not really see how to use it in this context and I cannot test
>> the situation.
>>
>>
>>> Frankly said: I’m lost now.
>>>
>>
>> For this bug and the other similar bug on the bugtracker [down currently
>> again] I thought about detecting false positives by comparing the file
>> hashes before setting the flag.
>>
>> Before this drastic measure I would like to see if I can understand why
>> the signal comes too late for the blocker, and if it's just missing
>> something simple.
>>
>>
>> Guillaume
>>
>>
>>
>

Re: 2.3.0alpha1-1: file save

[Guillaume MM]

Le 10/05/2017 à 22:57, Stephan Witt a écrit :

Am 09.05.2017 um 21:19 schrieb john kennan :


Start a new file
type something
save
type some more
save

I get "the file ... changed on disk." with two buttons: "Reload" and "Ignore"

hitting Ignore leaves the file unsaved
hitting Reload works -- I get a warning:

Any changes will be lost. Are you sure you want to load the version on disk of 
the document .../Papers/LyX/test1.lyx?

with buttons for Revert and Cancel
Hitting Revert saves the file, as desired, and the changes are not lost.


Dear John, thanks for the report. This looks like LyX incorrectly
recording the modification it has made itself as an external
modification. This means that in fact the file is already correctly
saved when the message shows up, and the two buttons do nothing except
annoy. I am looking into it.

Does it happen only on the second save? Does it happen on the third save
if you carry on? Does it not happen on the first save? If not, is it
because the first save is a "Save As"?



Confirmed.


Dear Stephan, thanks for the test. This looks specific to OSX. I would
ask the same questions as above.



Guillaume, it’s the FileMonitor which is detecting the file save as external 
modification.
How can I provide further info to correct this annoying behavior?
My attempt to diagnose the culprit failed - the code is too weird for me :(

I tried to stop in Buffer::Impl::refreshFileMonitor() at the first line.
The debugger didn’t stop - but it stopped in 
Buffer::Impl::fileExternallyModified()
and the call stack claims it comes from the last line in refreshFileMonitor() 
???


As QFileSystemWatcher is intrinsically asynchronous, gdb does not really
help with debugging here. What you see on the trace is
QFileSystemWatcher calling the function that was passed to connect at
the end of refreshFileMonitor (via a qt signal that is converted into a
boost signal).

What should happen is inside Buffer::save there is a FileMonitorBlocker
that should block the signal. Now, since QFileSystemWatcher is
asynchronous, the signal is not received before the next iteration of
the even loop. At this point the FileMonitorBlocker has been destroyed a
long time ago. This is why FileMonitorBlocker unblocks the signal in an
asynchronous way too, using a QTimer. On Linux, a delay of 0 (meaning
wait until the next event loop) is enough to actually block the signal.

The delay is defined in Buffer::Impl::blockFileMonitor (Buffer.cpp:388)
currently at 10ms. Can you try to increase this value and see if that
helps? It will likely help, but this is not a nice solution.

Ideally I would like to have everything work with a delay of 0ms, to be
sure that everyone experiences the same. Can you help me and see if it
is possible to flush the file operations in FileName::copyTo and
FileName::moveTo and if it makes a difference ? There is QFile::flush
but I do not really see how to use it in this context and I cannot test
the situation.



Frankly said: I’m lost now.


For this bug and the other similar bug on the bugtracker [down currently
again] I thought about detecting false positives by comparing the file
hashes before setting the flag.

Before this drastic measure I would like to see if I can understand why
the signal comes too late for the blocker, and if it's just missing
something simple.


Guillaume

Re: 2.3.0alpha1-1: file save

[Stephan Witt]

Am 09.05.2017 um 21:19 schrieb john kennan :
> 
> Start a new file
> type something
> save
> type some more
> save
> 
> I get "the file ... changed on disk." with two buttons: "Reload" and "Ignore"
> 
> hitting Ignore leaves the file unsaved
> hitting Reload works -- I get a warning:
> 
> Any changes will be lost. Are you sure you want to load the version on disk 
> of the document .../Papers/LyX/test1.lyx?
> 
> with buttons for Revert and Cancel
> Hitting Revert saves the file, as desired, and the changes are not lost.

Confirmed.

Guillaume, it’s the FileMonitor which is detecting the file save as external 
modification.
How can I provide further info to correct this annoying behavior?
My attempt to diagnose the culprit failed - the code is too weird for me :(

I tried to stop in Buffer::Impl::refreshFileMonitor() at the first line.
The debugger didn’t stop - but it stopped in 
Buffer::Impl::fileExternallyModified()
and the call stack claims it comes from the last line in refreshFileMonitor() 
???

Frankly said: I’m lost now.

Stephan

(lldb) bt
* thread #1: tid = 0x5bbf69, 0x0001001260a6 
LyX`lyx::Buffer::Impl::fileExternallyModified(this=0x000107129400, 
modified=true) const + 38 at Buffer.cpp:5342, queue = 'com.apple.main-thread', 
stop reason = breakpoint 2.1
frame #0: 0x0001001260a6 
LyX`lyx::Buffer::Impl::fileExternallyModified(this=0x000107129400, 
modified=true) const + 38 at Buffer.cpp:5342
  * frame #1: 0x00010017f83d 
LyX`lyx::Buffer::Impl::refreshFileMonitor(this=0x000103e5f720)::$_0::operator()()
 const + 29 at Buffer.cpp:5335
frame #2: 0x00010017f45d 
LyX`boost::detail::function::void_function_obj_invoker0::invoke(function_obj_ptr=0x000103e5f720) + 29 at 
function_template.hpp:159
frame #3: 0x00010108dce8 
LyX`boost::function0::operator(this=0x000103e5f718)() const + 120 at 
function_template.hpp:770
frame #4: 0x00010108dc64 LyX`boost::signals2::detail::void_type 
boost::signals2::detail::call_with_tuple_args::m_invoke >(boost::function&, 
boost::signals2::detail::unsigned_meta_array<>, std::__1::tuple<> const&, 
boost::enable_if, 
void>::type*) const + 36 at variadic_slot_invoker.hpp:117
frame #5: 0x00010108dc19 LyX`boost::signals2::detail::void_type 
boost::signals2::detail::call_with_tuple_args::operator(this=0x7fff5fbfbb70,
 func=0x000103e5f718, args=0x7fff5fbfc040, (null)=size_t<0> @ 
0x7fff5fbfbb30)(boost::function&, 
std::__1::tuple<> const&, mpl_::size_t<0ul>) const + 41 at 
variadic_slot_invoker.hpp:90
frame #6: 0x00010108dbe1 LyX`boost::signals2::detail::void_type 
boost::signals2::detail::variadic_slot_invoker::operator(this=0x7fff5fbfc040,
 
connectionBody=0x000103e5bbb0), boost::signals2::slot >, boost::signals2::mutex> > 
>(boost::shared_ptr, boost::signals2::slot >, boost::signals2::mutex> > const&) const + 65 at 
variadic_slot_invoker.hpp:133
frame #7: 0x00010108da9a 
LyX`boost::signals2::detail::slot_call_iterator_t, boost::signals2::slot >, boost::signals2::mutex> >, void*>, 
boost::signals2::detail::connection_body, boost::signals2::slot >, boost::signals2::mutex> >::dereference(this=0x7fff5fbfbcf0) const + 
154 at slot_call_iterator.hpp:110
frame #8: 0x00010108d9d5 
LyX`boost::signals2::detail::slot_call_iterator_t, boost::signals2::slot >, boost::signals2::mutex> >, void*>, 
boost::signals2::detail::connection_body, boost::signals2::slot >, boost::signals2::mutex> >::reference 
boost::iterators::iterator_core_access::dereference, boost::signals2::slot >, boost::signals2::mutex> >, void*>,