labstreaminglayer / App-LabRecorder

An application for streaming one or more LSL streams to disk in XDF file format.
MIT License
123 stars 45 forks source link

Bug in which LabRecorder does not record data if the file name ends with a space #93

Closed jiversen closed 1 year ago

jiversen commented 1 year ago

Hello,

I discovered, painfully, that if the file name string entered in the File Name/Template field of LabRecorder has one or more spaces at the end some weird things happen that can lead to data loss (either data not being recorded or an existing file being overwritten without first making a backup of it as it usually does).

Windows 10 LabRecorder 1.16.3 Unity Marker and position outlet streams only (LSL4Unity) BIDS unchecked

Here's what I've seen when having a space after the file name--it happened because I pasted my favorite file name template and (now realize) it inadvertently included a space (and maybe a return) in the copied text.

• LabRecorder appears to silently fail to record any LSL data (the file size counter remains at 0 kB even though the clock counts up) and though the console says 'started data collection', 'wrote footer', and 'closing the file' like normal, in some cases it does not save any file.

• Sometimes it does save a file, despite the file size counter staying at 0 kB, and the file does appear to have some data in it. I haven't figured out why sometimes it does vs. does not save a file.

• However, if a file of that name (without the trailing spaces) exists already, LabRecorder does not back it up as usual before recording over it and thus you lose the original file.

Anyway, it's easy enough to work around, and seems like it should be a trivial fix to strip trailing spaces, but given the seriousness of the consequences it's probably worth checking the logic to see how it could fail the test for existing files.

Thanks,

John

cboulay commented 1 year ago

Hi John, thanks for the report. I'll just add a couple notes here in case anyone wants to tackle this.

The line to convert the linedit text to the filename: https://github.com/labstreaminglayer/App-LabRecorder/blob/6b0d4ec1da259638ad3644c5c91e6fdc2a255a76/src/mainwindow.cpp#L398.

This calls 2 functions. First is a Qt function, cleanPath. Second is in that same file is the function to replace placeholders with true values: https://github.com/labstreaminglayer/App-LabRecorder/blob/6b0d4ec1da259638ad3644c5c91e6fdc2a255a76/src/mainwindow.cpp#L537-L561.

We could simply modify the last line to be return fullfile.trimmed();

cboulay commented 1 year ago

The line where it calculates the size is very straightforward. https://github.com/labstreaminglayer/App-LabRecorder/blob/6b0d4ec1da259638ad3644c5c91e6fdc2a255a76/src/mainwindow.cpp#L103-L107

It's just QFileInfo(...).size() / 1000. The docs say "If the file does not exist or cannot be fetched, 0 is returned."

So that will explain your 0 kB problem.

That doesn't explain the actual file being empty though. That one surprises me. Might it have been another problem, where you were trying to record from zombie streams and not the new streams that replaced the zombie streams? I've had this happen to me, typically when debugging.

jiversen commented 1 year ago

Hi @cboulay, Thank you for the responses. The trimmed() sounds like an elegant solution. Your explanation makes the 0kB problem clear, as well as the fact that it does not create a backup---it's looking for a file with name ending in ' ', which does not exist because something in the file creation itself silently trimmed the space-appended filename before creating a file with the correct, trimmed name (and overwriting the old one).

I would be happy to make the change. I haven't built LabRecorder from source yet so I may or may not be able to test it.

jiversen commented 1 year ago

I don't seem to have permissions to push a branch to start a pull request, but the patch is attached here... a15b5e.patch

Thank you!

PS. As far as no file being written, perhaps I simply didn't notice that an existing file was being overwritten?