Add flags for setting icons, sink names #34

OJFord · 2020-10-13T18:10:23Z

For example:

polybar-pulseaudio-control --sink-icon='⇉ ' \
  --sink-name-from='device.description' --vol-icon-low='🔈 ' \
  --vol-icon-mid='🔉 ' --vol-icon-high='🔊 ' listen

Towards #26. I haven't added options for everything since I thought it'd be better to get some early feedback, and, frankly, these are the ones that I care about 😄.

Sink nicknames from a pulseaudio property is a bit of a departure from the way it works setting within the file, but it seemed like it's be easier to use, and on my system at least has pretty usable results (to the point I almost want to suggest device.description as a default instead of numbers) - and perhaps there's a pulseaudio-wide way of overwriting descriptions (or other properties) that might be better than maintaining a separate list of polybar nicknames? Haven't looked into that yet.

In fact, it's surely possible, since I can change device.description & bluez.alias together for bluetooth devices by renaming them in blueman.

For example: polybar-pulseaudio-control --sink-icon='⇉ ' \ --sink-name-from='device.description' --vol-icon-low='🔈 ' \ --vol-icon-mid='🔉 ' --vol-icon-high='🔊 ' listen Towards marioortizmanero#26.

Note that this is a change in behaviour - previously the `--output` was incorrect and the usage was displayed as though an error but it was ignored. It seems clear that the intention was the `output` subcommand, so I've left that in rather than remove it, or change the `--`-parsing behaviour to exit with status 0 on a bad argument in order to continue to allow it. I've also changed it to a single line output instead of the full `usage` - since this makes it clearer what's wrong in polybar, where it may occur if typo'd in config, for example.

pulseaudio-control.bash

marioortizmanero · 2020-10-14T15:36:06Z

I like your idea about --sink-name-from, but what if, instead of applying it with setNickname at the beginning of the script, we read the property as a fallback before it's displayed in getNickname? That way, if a new sink is added we'll get its description as well.

OJFord · 2020-10-14T19:32:17Z

Yes that's actually a problem I encountered running it today - no description when I connected my Bluetooth earphones.

I was thinking the solution would be setting interval to something reasonable (and putting up with that as a maximum period of no nice name) - but I prefer yours. Coming.

Previously a newly connected device wouldn't have its prop read.

pulseaudio-control.bash

@marioortizmanero

Thanks @marioortizmanero for the suggestion.

marioortizmanero · 2020-10-16T11:25:28Z

Great! I think the base implementation is quite good already. If you have time, you can add the rest of the flags now (and update the README if possible) 👍

OJFord · 2020-10-16T22:18:06Z

Done, and I realise now forgot the readme (sec) - though I confess I haven't tested them all (some I'm not set up to) but they were pretty straightforward with the parsing already there.

OJFord · 2020-10-16T22:28:02Z

README.md

+```ini
+[module/pulseaudio-control]
+type = custom/script
+exec = "polybar-pulseaudio-control --vol-max=150 --vol-icons='🔈 ,🔉 ,🔊 ' --sink-name-from-prop=device.description --osd"


This doesn't need to be quoted, but GitHub renders it weirdly if it isn't: --vol-max, --vol-icons, and --sink-name-from-prop get highlighted, so it's almost an accidental feature, but then --osd doesn't.

marioortizmanero

Thanks! I've left some suggestions we can discuss before merging this. Sorry if I'm asking for too many changes, I want to make sure it's right :P. Also, by updating the README I meant the "Configuration", "Usage" and "Useful icons" as well. If that's too much work I can take care of it, but it should be easy.

pulseaudio-control.bash

marioortizmanero

I think this is actually ready to be merged! Thank you very much for your work :)

I will release a new version with this for those following the repo. Give me a moment to try this myself in my setup first. I'll then make a few changes updating the example module in the README, which is basically what I use.

marioortizmanero · 2020-10-25T13:44:52Z

Also, are you still planning on releasing this on the AUR? If so, let me know when you do. We can add that to the README, and I could help you as an Arch user with a couple packages in there as well.

marioortizmanero · 2020-10-25T15:36:49Z

Should usage be called when an action/option is unrecognized?
Should we have a line length limit for the options help message?

I've made some changes I wanted to do. Let me know what you think.

pulseaudio-control.bash

OJFord · 2020-10-25T19:49:58Z

Also, are you still planning on releasing this on the AUR?

I would like it to be available there yes, happy to package it myself or I'll of course hold off if you'd like to maintain that yourself - you'll always know when there's a new release after all 🙂.

Should usage be called when an action/option is unrecognized?

The reason I didn't was so that there's a single line of output when it's misused - reading nicely in polybar.

When I booted my PC just now for example, since I hadn't changed my main config since making these changes, polybar read 'Unrecognised option: --sink-icon'. If we printed the full usage polybar would just show the first line 'Usage: ...' which is less helpful. I suppose printing usage after the current output would work though (not tested).

Should we have a line length limit for the options help message?

Probably.. actually even the first one (autosync) is a little long compared to where e.g. git & grep wrap. Not sure if there's any pseudo-standard for help text specifically, but I doubt anyone would complain about 80ch.

marioortizmanero · 2020-10-25T20:09:27Z

I would like it to be available there yes, happy to package it myself or I'll of course hold off if you'd like to maintain that yourself - you'll always know when there's a new release after all slightly_smiling_face.

We could just co-maintain the package, if you want (I think that's possible in the AUR).

The reason I didn't was so that there's a single line of output when it's misused - reading nicely in polybar.

When I booted my PC just now for example, since I hadn't changed my main config since making these changes, polybar read 'Unrecognised option: --sink-icon'. If we printed the full usage polybar would just show the first line 'Usage: ...' which is less helpful. I suppose printing usage after the current output would work though (not tested).

Fair enough.

Probably.. actually even the first one (autosync) is a little long compared to where e.g. git & grep wrap. Not sure if there's any pseudo-standard for help text specifically, but I doubt anyone would complain about 80ch.

80 characters is my goto as well. It looks a bit stretched because of how long the arguments are, not sure what you think about that:

It uses a single echo from now on because it's much easier to edit and because of the character line at 80 chars. I've also updated the message according to your review, but I'm not sure if that's exactly what you wanted. Let me know otherwise.

I think we should also change the default value so that it's between [ instead of (. The latter might be thought of as part of the explanation, while the former stands out as something different. This is just nitpicking, though.

OJFord · 2020-10-25T20:55:54Z

We could just co-maintain the package, if you want (I think that's possible in the AUR).

Ah yes, so it is. 👍

👍 for single echo, I was tempted to do that too.

Screenshot looks ok to me, maybe gets a little hard to read at the bottom over several lines, but then spaces around that and not the others might look weirder. (Also nitpicking though!) Think I agree about [] defaults.

marioortizmanero · 2020-10-25T21:47:13Z

I've ended up using an indentation of 2 spaces instead of 4 for the help message, which gives just a bit of space more (not sure what else can be done). I'll merge this and we can discuss the PKGBUILD at #35.

Thanks a lot for your help with this PR!

OJFord · 2020-10-25T22:03:43Z

Awesome, thanks!

OJFord added 4 commits October 13, 2020 18:59

Appease shellcheck

41fc5e8

Add flags for setting icons, sink names

af5d820

For example: polybar-pulseaudio-control --sink-icon='⇉ ' \ --sink-name-from='device.description' --vol-icon-low='🔈 ' \ --vol-icon-mid='🔉 ' --vol-icon-high='🔊 ' listen Towards marioortizmanero#26.

Make usage clear that options precede subcmds

51d3f8f

marioortizmanero requested changes Oct 13, 2020

View reviewed changes

OJFord added 3 commits October 14, 2020 02:53

Redirect at EOL

7a0c797

Support arbitrary number of vol icons

ea34027

Refactor --arg parsing for optional =

cbd8d09

OJFord mentioned this pull request Oct 14, 2020

Config options for exec args - e.g. exec-arg-0 = --value foobar, exec-arg-1 = -vv polybar/polybar#2207

Closed

OJFord added 3 commits October 14, 2020 13:09

Add --sink-nickname setting

b3594f3

Add --sink-blacklist setting

e370e61

Support (future) value-less flags

e46a593

marioortizmanero reviewed Oct 14, 2020

View reviewed changes

pulseaudio-control.bash Outdated Show resolved Hide resolved

OJFord added 2 commits October 14, 2020 20:39

Detail arg values

edf3809

Defer sink name prop lookup to naming-time

f6854d1

Previously a newly connected device wouldn't have its prop read.

OJFord force-pushed the config-args branch from 3629a17 to f6854d1 Compare October 14, 2020 21:13

marioortizmanero reviewed Oct 16, 2020

View reviewed changes

pulseaudio-control.bash Outdated Show resolved Hide resolved

marioortizmanero reviewed Oct 16, 2020

View reviewed changes

pulseaudio-control.bash Outdated Show resolved Hide resolved

OJFord added 2 commits October 16, 2020 11:41

Refactor reading sink properties

85d730c

Thanks @marioortizmanero for the suggestion.

Adopt global var style rather than pseudo-retval

18f527d

Add flags for remaining settings

5a6224c

Mention --args in readme & provide example

f27d41d

OJFord force-pushed the config-args branch from d7df62f to f27d41d Compare October 16, 2020 22:26

OJFord commented Oct 16, 2020

View reviewed changes

OJFord requested a review from marioortizmanero October 16, 2020 22:28

marioortizmanero reviewed Oct 16, 2020

View reviewed changes

OJFord force-pushed the config-args branch from f35a1ea to 505b8ca Compare October 17, 2020 21:07

OJFord added 3 commits October 24, 2020 15:58

Rework CLI args to consistent names

5d21001

Remove default icons, more clearly format help

54c2bc1

Explain sink 'properties' and 'names'

93360e3

OJFord requested a review from marioortizmanero October 24, 2020 15:17

marioortizmanero reviewed Oct 25, 2020

View reviewed changes

Fully consistent variable names

8a4b1e9

marioortizmanero force-pushed the config-args branch from 76067b8 to 8a4b1e9 Compare October 25, 2020 15:33

Fix tests

e59a031

marioortizmanero force-pushed the config-args branch 3 times, most recently from e4495f5 to f50ab9b Compare October 25, 2020 16:20

Updated README.md

72518c7

marioortizmanero force-pushed the config-args branch from f50ab9b to 72518c7 Compare October 25, 2020 16:21

OJFord commented Oct 25, 2020

View reviewed changes

pulseaudio-control.bash Outdated Show resolved Hide resolved

OJFord commented Oct 25, 2020

View reviewed changes

pulseaudio-control.bash Outdated Show resolved Hide resolved

Update help message

7c5eeba

marioortizmanero added 2 commits October 25, 2020 22:48

final touches to the help message

4dcb5cd

VOLUME_INC -> VOLUME_STEP for consistency

710a9d1

marioortizmanero force-pushed the config-args branch from ed28f07 to 710a9d1 Compare October 25, 2020 21:49

marioortizmanero merged commit d40cbf6 into marioortizmanero:master Oct 25, 2020

marioortizmanero mentioned this pull request Oct 25, 2020

Consider using arguments to configure the module, rather than constants inside the script #26

Closed

OJFord deleted the config-args branch October 25, 2020 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flags for setting icons, sink names #34

Add flags for setting icons, sink names #34

OJFord commented Oct 13, 2020 •

edited

Loading

marioortizmanero commented Oct 14, 2020 •

edited

Loading

OJFord commented Oct 14, 2020

marioortizmanero commented Oct 16, 2020

OJFord commented Oct 16, 2020

OJFord Oct 16, 2020

marioortizmanero left a comment

marioortizmanero left a comment •

edited

Loading

marioortizmanero commented Oct 25, 2020

marioortizmanero commented Oct 25, 2020 •

edited

Loading

OJFord commented Oct 25, 2020

marioortizmanero commented Oct 25, 2020

OJFord commented Oct 25, 2020

marioortizmanero commented Oct 25, 2020 •

edited

Loading

OJFord commented Oct 25, 2020

Add flags for setting icons, sink names #34

Add flags for setting icons, sink names #34

Conversation

OJFord commented Oct 13, 2020 • edited Loading

marioortizmanero commented Oct 14, 2020 • edited Loading

OJFord commented Oct 14, 2020

marioortizmanero commented Oct 16, 2020

OJFord commented Oct 16, 2020

OJFord Oct 16, 2020

Choose a reason for hiding this comment

marioortizmanero left a comment

Choose a reason for hiding this comment

marioortizmanero left a comment • edited Loading

Choose a reason for hiding this comment

marioortizmanero commented Oct 25, 2020

marioortizmanero commented Oct 25, 2020 • edited Loading

OJFord commented Oct 25, 2020

marioortizmanero commented Oct 25, 2020

OJFord commented Oct 25, 2020

marioortizmanero commented Oct 25, 2020 • edited Loading

OJFord commented Oct 25, 2020

OJFord commented Oct 13, 2020 •

edited

Loading

marioortizmanero commented Oct 14, 2020 •

edited

Loading

marioortizmanero left a comment •

edited

Loading

marioortizmanero commented Oct 25, 2020 •

edited

Loading

marioortizmanero commented Oct 25, 2020 •

edited

Loading