search

labstreaminglayer / App-LabRecorder

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

[rcs] Hid some more warnings on start/stop #76

Closed brifsttar closed 2 years ago

brifsttar commented 2 years ago

@tstenner @cboulay

(Hi, I'm tagging you because my last (smallest ever) PR went unnoticed, so I'm not sure if anyone's checking them.)

We use automated remote control a lot at our lab, and sometimes the experiment process sends multiple start or stops in a bunch, which each time adds a popup on LabRecorder (The recording is already running and There is not ongoing recording). After a while, we can have dozens of those, which make subsequent manual use difficult.

I realize that the nice way to deal with it is to not send multiple start/stop, which I also fixed on my end; but given that LabRecorder already has a hideWarnings flag for remote control, I figured it might also be used for those two warnings.

While at it, I also added a reset of the flag after each command, otherwise it was never reverted to false. Also, while in remote control, it might be useful to log the warning instead of a popup, but I didn't implement that.

cboulay commented 2 years ago

Hi @brifsttar , I'm sorry I couldn't continue the conversation earlier.

The addition of the rcsStopRecording is quite welcome. Thank you. However, re-enabling warnings automatically, especially when it's not confined to rcs-commands, is setting us up for a future bug.

For example, let's say we add a feature to support setting the hideWarnings flag via cfg file, but there is no GUI-enabled way to turn it on or off, then we'll have a user who wants the warnings to be permanently disabled but they keep getting their warnings re-enabled automatically.

To get this PR merged quickly, I would prefer if the re-enabling was removed.

Longer term, I think we probably want new rcs* commands to set the value of the hideWarnings flag. After these commands are added, to keep behaviour consistent with previous versions, the rcsStartRecording method should probably still set hideWarnings = true;, and I guess the same should be true for rcsStopRecording. But re-enabling should be explicit.

To put a bow on it, having a cfg-entry and a menubar item in the GUI to enable/disable warnings would also be a welcome addition.

brifsttar commented 2 years ago

Hey @cboulay,

I understand your concerns better now, so I removed the re-enabling.

Thank you for your great feedback!

cboulay commented 2 years ago

Looks good, thanks!