FIX: Not looping leading to TypeError

[sappelhoff]

A flag like --loop in this case is typically used as a "true-if-supplied" flag. However, it was instead (confusingly counter-intuitively) used as a "false-if-supplied" flag.

This is now fixed. python -m pyxdf.examples.playback_lsl.py --loop will loop, whereas not supplying --loop will not loop.

[sappelhoff]

Actually, there is more wrong an issue that comes up. Not looping is not possible currently:

  File "C:\Users\stefan\AppData\Local\miniforge3\envs\naf\Lib\site-packages\pyxdf\examples\playback_lsl.py", line 229, in <module>
    main(
  File "C:\Users\stefan\AppData\Local\miniforge3\envs\naf\Lib\site-packages\pyxdf\examples\playback_lsl.py", line 192, in main
    timer.update()
  File "C:\Users\stefan\AppData\Local\miniforge3\envs\naf\Lib\site-packages\pyxdf\examples\playback_lsl.py", line 103, in update
    overrun = self._n_loop * self._boundary
              ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
TypeError: unsupported operand type(s) for *: 'int' and 'NoneType'

[sappelhoff]

@cbrnr I'd be happy to receive a review on this!

[cboulay]

playback_lsl is mine, but I'm happy to see others use it and improve it.

The previous default behaviour was to loop, and now the default behaviour is to not loop. I prefer if the default is to loop, but then how do we fix the argument name in argparse? Do we only offer "--no-loop"?

I'm not great with argparse. I tend to use typer more but I don't think we need to add that as a dependency unless we really plan to expand the CLI offerings. Anyway, in typer, bool args get exposed both in the affirmative and negative. e.g., this would give use --loop and --no-loop; both would be exposed regardless of what we set the default to.

[cboulay]

I can also be convinced that not-looping is the better default behaviour.

[sappelhoff]

Hey @cboulay -- thanks for programming and sharing this script, it has been very helpful for me! (this comment is what I should have started this PR with, sorry if this all came across as brash)

Do we only offer "--no-loop"?

yes, that would be a sensible alternative solution.

I can also be convinced that not-looping is the better default behaviour.

I think from a user experience perspective it's more natural to expect a playback to NOT loop by default (think of music that you play).

Please let me know if you want me to change the default back to looping, and adjust the flag to be --no-loop.

Other than that, it would be ready for merge from my side :)

[cbrnr]

Just my two cents (and you said you can be convinced), I'd also think that not looping is a better default. But I leave this up to you, so feel free to merge whenever you're happy @cboulay!

[cbrnr]

@sappelhoff can you rebase please? Afterwards I think we can merge.

[sappelhoff]

@cbrnr sure -- done!

[cbrnr]

Thanks @sappelhoff!