bastibe / SoundCard

A Pure-Python Real-Time Audio Library
https://soundcard.readthedocs.io
BSD 3-Clause "New" or "Revised" License
680 stars 69 forks source link

Implement application name getting and setting #82

Closed flying-sheep closed 4 years ago

flying-sheep commented 4 years ago

Fixes #81

Fun fact: This is the 3rd time in my life that a non-trivial piece of code in unfamiliar territory worked on the first try.

bastibe commented 4 years ago

By the way, perhaps we should use this opportunity to change the default name.

"Python SoundCard" would certainly be better than "audio". Or perhaps, we could even do something like " ".join(sys.argv) by default. But I am not sure whether that's actually a good idea... Do you have a preference?

flying-sheep commented 4 years ago

I created a little function to infer the appname. Please tell me if you think the full path or parameters are too valuable information to throw away. I think keeping things too long is confusing here.

bastibe commented 4 years ago

I agree. As long as it's reasonably obvious to the user that this is the Python script they're running, a short name is better than a longer one.

Could you give an example of how your function would handle, say python mycoolscript.py and ./mycoolscript.py?

As this functionality is currently pulseaudio/Linux-only, I'd prefer to keep it inside pulseaudio.py instead of a separate file for now.

flying-sheep commented 4 years ago

Okay!

$ cd /tmp
$ echo >mycoolscript.py <EOF
#!/usr/bin/env python3
import sys, os
print(os.path.basename(sys.argv[0]))
EOF
$ chmod +x mycoolscript.py
$ python3 mycoolscript.py
mycoolscript.py
$ ./mycoolscript.py
mycoolscript.py
$ python -m mycoolscript
mycoolscript.py

I’ll also fix the package case, which currently does:

$ mkdir mycoolscript
$ mv mycoolscript.py mycoolscript/__main__.py  
$ python -m mycoolscript
__main__.py

/edit: done, the last one now returns “mycoolscript”

bastibe commented 4 years ago

Cool, thank you very much!

This looks very good to me! Only one small change is still needed, that I missed last time (sorry): The documentation for SoundCard is built from the Linux source, so the docstrings of set_name and get_name need to mention that they are only implemented for Linux at the moment.

flying-sheep commented 4 years ago

Done. I also edited the doc config:

https://github.com/bastibe/SoundCard/blob/c39b2bb80f4cf59e338f14a19eef5f360d223781/doc/conf.py#L184-L191

It didn’t work for me, as it infinitely hung here:

https://github.com/bastibe/SoundCard/blob/c39b2bb80f4cf59e338f14a19eef5f360d223781/soundcard/pulseaudio.py#L70

bastibe commented 4 years ago

Great work! Thank you.

Do you feel this is ready to merge?

flying-sheep commented 4 years ago

Yeah, I think so!

bastibe commented 4 years ago

Thank you very much for your contribution, and a pleasant conversation!

(Should I forget to cut a new release for SoundCard in a month or so, feel free to ping me on this thread.)