search

rbn42 / panon

An Audio Visualizer Widget in KDE Plasma
GNU General Public License v3.0
191 stars 31 forks source link

Removed subprocess and some small script improvements :) #49

Closed yuannan closed 3 years ago

rbn42 commented 3 years ago

https://github.com/rbn42/panon/blob/864dd15bd4800cb461054286398d537bf1b4c5e4/plasmoid/contents/ui/config/ConfigBackend.qml#L189

So "Any Active Speaker" is no longer a suitable name. Do you have a better suggestion?

yuannan commented 3 years ago

https://github.com/rbn42/panon/blob/864dd15bd4800cb461054286398d537bf1b4c5e4/plasmoid/contents/ui/config/ConfigBackend.qml#L189

So "Any Active Speaker" is no longer a suitable name. Do you have a better suggestion?

"Monitor Active Speaker"? or "Monitor of Current Speaker" or "Monitor of Default Speaker" Fits in the current options better :) I think "Monitor of Default Speaker" is the most accurate option but "Monitor of Current Speaker" is more user friendly. image

rbn42 commented 3 years ago

Monitor Active Speaker = p.server_info['default sink id']

Your code is searching for the default one, I mean, not an active one.

And unlike the original "default device", you assumed the default device could be changed during running.

I would come to a name like "Hotplugging default device". I hope you have a better idea.

yuannan commented 3 years ago

Monitor Active Speaker = p.server_info['default sink id']

Your code is searching for the default one, I mean, not an active one.

And unlike the original "default device", you assumed the default device could be changed during running.

It's fine. Pulse keeps this up to date and soundcard has multi-processing support with locks :). It's not the first device but instead the default CURRENT playback device. It works just fine on my setup when I switch between them.

I'm pretty sure under KDE when you change your device it changes all the sources to the new sink.

AFIAK, this completes the exact same function as the old code but just faster as it uses a native python library that calls C libs instead of using a subprocess.

rbn42 commented 3 years ago

Everything is good, except for the name.

Do you want me to merge now? Or do you want to rename the "Any Active Speaker" option?

yuannan commented 3 years ago

I think "Current Playback Device" would be my vote right now. As this implies that it's needed to be hot plugged which is not true. We don't need to expose the user to the implementation of the code.

yuannan commented 3 years ago

Change it to "Monitor Current Device" and merge it please :)