New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bluetooth headset i3block #20

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@valir

valir commented Nov 29, 2015

This pull request is for a small i3block I put together today, when playing with my new bluetooth headset and PulseAudio. It's usage should be quite straightforward, as the bluetooth setup and device pairing is out-of-scope of this i3block.

Thanks for reviewing it.

Valentin Rusu (a.k.a. valir)

Valentin Rusu and others added some commits Nov 29, 2015

Fixing document markup
During these last weeks I used a lot asciidoc, and markdown seems to be a little different ;-)
@Nycroth

This comment has been minimized.

Show comment
Hide comment
@Nycroth

Nycroth Nov 30, 2015

Contributor

I love the script functionality. 👌 Never even though about a blocklet to handle bluetooth 😄, however, I just have some comments about the patch:

  • My first question is: why two scripts?
  • Typically blocklets should refrain from utilizing the left click. We keep this for manual refresh of blocklets by the user. Would changing the click to the right click (button 3) be alright?
  • Your script depends on FontAwesome (totally welcome) but putting this in the README.md will be a help to those who end up with no output if they don't have the font installed. If you want a template for your README.md, I can recommend the wiki.
  • You directly pulled the headset ID from $1 (and mode setting in your .sh) but I'd suggest using getopts to just formalize these in your script. This way, when someone whats to extend your script with an option for something, the getopts loop already exists. Plus, it conforms to the POSIX Utility Syntax Guidelines (not a POSIX preacher but copying user interface is always a good thing).
  • You used tests like [ "x$HEADSET_ID" == "x" ], using [[ ]] instead uses internal shell tests (so no other program is called), and using the -z option will test for empty strings (man test for more info).
  • Something to skim over would be Common Bash Pitfalls wiki. There are a few of them within your program that you may want to correct.
  • Is there a way to avoid requiring root privileges? Perhaps by depending on consolekit, polkitd, or users/groups?

Again, the blocklet is a fantastic idea! Can't wait to give this patch the old 👍.

Contributor

Nycroth commented Nov 30, 2015

I love the script functionality. 👌 Never even though about a blocklet to handle bluetooth 😄, however, I just have some comments about the patch:

  • My first question is: why two scripts?
  • Typically blocklets should refrain from utilizing the left click. We keep this for manual refresh of blocklets by the user. Would changing the click to the right click (button 3) be alright?
  • Your script depends on FontAwesome (totally welcome) but putting this in the README.md will be a help to those who end up with no output if they don't have the font installed. If you want a template for your README.md, I can recommend the wiki.
  • You directly pulled the headset ID from $1 (and mode setting in your .sh) but I'd suggest using getopts to just formalize these in your script. This way, when someone whats to extend your script with an option for something, the getopts loop already exists. Plus, it conforms to the POSIX Utility Syntax Guidelines (not a POSIX preacher but copying user interface is always a good thing).
  • You used tests like [ "x$HEADSET_ID" == "x" ], using [[ ]] instead uses internal shell tests (so no other program is called), and using the -z option will test for empty strings (man test for more info).
  • Something to skim over would be Common Bash Pitfalls wiki. There are a few of them within your program that you may want to correct.
  • Is there a way to avoid requiring root privileges? Perhaps by depending on consolekit, polkitd, or users/groups?

Again, the blocklet is a fantastic idea! Can't wait to give this patch the old 👍.

@valir

This comment has been minimized.

Show comment
Hide comment
@valir

valir Nov 30, 2015

Thanks for the review and 👍 Answers to the questions first:

  • two scripts because the btheadset.sh is also handy on the command line. I'm living lots of the time on the command-line and I think I'm not alone like that. So I keep the connect/disconnect logic in a CLI-only script and call it from the i3block. I also wrote about this precise point in the README
  • OK, wasn't aware about that small detail about mouse buttons - will do in next commit
  • getopts - is not very useful here IMHO, as there are no options but only operands. Adding it would make add unused code. Let the one who wants to extend the script add getopts.
  • Whaouh - I'm using a bash script linter in my vim and it didn't shown these pitfalls ;-) Fixing them.
  • About sudo - well, that's because bluetoothctl connect/disconnect is not working without it and I fail to see how to "convince" it to. There's no 'bluetooth' group or something like it here. And I realize that I should add a line about this also on the README.

valir commented Nov 30, 2015

Thanks for the review and 👍 Answers to the questions first:

  • two scripts because the btheadset.sh is also handy on the command line. I'm living lots of the time on the command-line and I think I'm not alone like that. So I keep the connect/disconnect logic in a CLI-only script and call it from the i3block. I also wrote about this precise point in the README
  • OK, wasn't aware about that small detail about mouse buttons - will do in next commit
  • getopts - is not very useful here IMHO, as there are no options but only operands. Adding it would make add unused code. Let the one who wants to extend the script add getopts.
  • Whaouh - I'm using a bash script linter in my vim and it didn't shown these pitfalls ;-) Fixing them.
  • About sudo - well, that's because bluetoothctl connect/disconnect is not working without it and I fail to see how to "convince" it to. There's no 'bluetooth' group or something like it here. And I realize that I should add a line about this also on the README.
@valir

This comment has been minimized.

Show comment
Hide comment
@valir

valir Nov 30, 2015

Investigating more on the need to use sudo, I now understand that's distribution specific. I'm using Arch Linux and here bluetoothctl is not tied to ConsoleKit. However, Gentoo actually do it. Seems to me that this bluetoothctl utility is somehow too recent and it's usage is not yet well established. So sudo should be safer to use FTM, even if it's not optimal.

valir commented Nov 30, 2015

Investigating more on the need to use sudo, I now understand that's distribution specific. I'm using Arch Linux and here bluetoothctl is not tied to ConsoleKit. However, Gentoo actually do it. Seems to me that this bluetoothctl utility is somehow too recent and it's usage is not yet well established. So sudo should be safer to use FTM, even if it's not optimal.

@Nycroth

This comment has been minimized.

Show comment
Hide comment
@Nycroth

Nycroth Dec 1, 2015

Contributor

The initial changes look real good. I will get back to this, just really busy with school as the semester comes to an end. Thanks again for the patch! :)

Contributor

Nycroth commented Dec 1, 2015

The initial changes look real good. I will get back to this, just really busy with school as the semester comes to an end. Thanks again for the patch! :)

@valir

This comment has been minimized.

Show comment
Hide comment
@valir

valir Dec 1, 2015

The sudo will still be needed for the beginning as Linux the distributions are yet to integrate bluetoothctl with PolicyKit. I'll modify the script as soon as it'll be correctly integrated.

valir commented Dec 1, 2015

The sudo will still be needed for the beginning as Linux the distributions are yet to integrate bluetoothctl with PolicyKit. I'll modify the script as soon as it'll be correctly integrated.

@kb100

This comment has been minimized.

Show comment
Hide comment
@kb100

kb100 Jan 7, 2016

Collaborator

@valir I like this idea, but it needs a bit more tailoring to this repo. Firstly, i3blocks should not be run as root, and we do not want users to have to figure out how to setup themselves to use sudo without a password safely. This is too easily done incorrectly. Due to the graphical nature of blocklets, I suggest calling a graphical front-end to sudo like gksu, and adding this as a dependency. Secondly, the scripts should be consolidated into a single file. I completely agree that the connect/disconnect script can be useful on it's own, but the whole purpose of having an i3blocks blocklet is to avoid having to open up another terminal. You are certainly welcome to maintain a separate script repo for the connect/disconnect script, and even link to it in your README, but the blocklet itself should really just be a single script.

Collaborator

kb100 commented Jan 7, 2016

@valir I like this idea, but it needs a bit more tailoring to this repo. Firstly, i3blocks should not be run as root, and we do not want users to have to figure out how to setup themselves to use sudo without a password safely. This is too easily done incorrectly. Due to the graphical nature of blocklets, I suggest calling a graphical front-end to sudo like gksu, and adding this as a dependency. Secondly, the scripts should be consolidated into a single file. I completely agree that the connect/disconnect script can be useful on it's own, but the whole purpose of having an i3blocks blocklet is to avoid having to open up another terminal. You are certainly welcome to maintain a separate script repo for the connect/disconnect script, and even link to it in your README, but the blocklet itself should really just be a single script.

@valir

This comment has been minimized.

Show comment
Hide comment
@valir

valir Jan 10, 2016

@kb100 Well, the sudo is actually needed and no prompt would come if you're the member of the wheel group or the equivalent. I searched the docs and there's no mean to have bluetoothctl successfully handle connects/disconnects without root access AFAICT.

Still about sudo, well, kgsu of kdesu - this should be configurable. But once again, it's not needed if user is member of the wheel group.

valir commented Jan 10, 2016

@kb100 Well, the sudo is actually needed and no prompt would come if you're the member of the wheel group or the equivalent. I searched the docs and there's no mean to have bluetoothctl successfully handle connects/disconnects without root access AFAICT.

Still about sudo, well, kgsu of kdesu - this should be configurable. But once again, it's not needed if user is member of the wheel group.

@kb100

This comment has been minimized.

Show comment
Hide comment
@kb100

kb100 Jan 11, 2016

Collaborator

@valir we should not assume the user has passwordless sudo setup. Currently the script hangs indefinitely if the user does not have passwordless sudo setup. With kdesudo or gksudo the user will be prompted for a password only if sudo would prompt them for a password. You can have the user pass a flag about whether they prefer kdesudo, gksudo, or even an option if the user is confident they have passwordless sudo setup to just use plain sudo.

Also please indent the body of functions, e.g.

function example()
{
    exit
}

Tabs/number of spaces is up to you, just be consistent.

Also please use [[ ]] instead of [ ] when possible, and once everything is settled change the README to reflect new changes.

Collaborator

kb100 commented Jan 11, 2016

@valir we should not assume the user has passwordless sudo setup. Currently the script hangs indefinitely if the user does not have passwordless sudo setup. With kdesudo or gksudo the user will be prompted for a password only if sudo would prompt them for a password. You can have the user pass a flag about whether they prefer kdesudo, gksudo, or even an option if the user is confident they have passwordless sudo setup to just use plain sudo.

Also please indent the body of functions, e.g.

function example()
{
    exit
}

Tabs/number of spaces is up to you, just be consistent.

Also please use [[ ]] instead of [ ] when possible, and once everything is settled change the README to reflect new changes.

@valir

This comment has been minimized.

Show comment
Hide comment
@valir

valir Jan 11, 2016

I updated the indentation, however, I use here-documents so that'll not be perfect. That's why I initially opted for not indenting, even if I also prefer indented code.

valir commented Jan 11, 2016

I updated the indentation, however, I use here-documents so that'll not be perfect. That's why I initially opted for not indenting, even if I also prefer indented code.

@kb100

This comment has been minimized.

Show comment
Hide comment
@kb100

kb100 Jan 12, 2016

Collaborator

@valir use <<- instead of << in here documents to ignore leading tabs, see https://en.wikipedia.org/wiki/Here_document#Unix_shells

Additionally, I would like to find a resolution to the script hanging. Currently if the user doesn't have either graphical sudo installed and the user doesn't have passwordless sudo setup, the script will just hang. Is there some way to tell if doing a sudo would ask for a password, and then instead print a user friendly error instead of hanging at a headless stdin? Perhaps something like closing stdin and catching the error if sudo tries to read from it (only in the case where no graphical sudo installed).

Also, please avoid using which to check if a program exists, see an excellent explanation at https://stackoverflow.com/questions/592620/check-if-a-program-exists-from-a-bash-script

Collaborator

kb100 commented Jan 12, 2016

@valir use <<- instead of << in here documents to ignore leading tabs, see https://en.wikipedia.org/wiki/Here_document#Unix_shells

Additionally, I would like to find a resolution to the script hanging. Currently if the user doesn't have either graphical sudo installed and the user doesn't have passwordless sudo setup, the script will just hang. Is there some way to tell if doing a sudo would ask for a password, and then instead print a user friendly error instead of hanging at a headless stdin? Perhaps something like closing stdin and catching the error if sudo tries to read from it (only in the case where no graphical sudo installed).

Also, please avoid using which to check if a program exists, see an excellent explanation at https://stackoverflow.com/questions/592620/check-if-a-program-exists-from-a-bash-script

@valir

This comment has been minimized.

Show comment
Hide comment
@valir

valir Jan 12, 2016

@kb100
which : Well, even if that solution seems leaner for the OS, it executes first the command one would search, spawning the process, and fails afterwards. In our case, I need to know which one is present, get it's path from stdout (I simply don't care about the exit code) then reuse that path in several places. Using what the article says would require rewriting all the script.

sudo : I currently don't know how to check that sudo will trigger a password prompt. I'll eventually investigate this later.

valir commented Jan 12, 2016

@kb100
which : Well, even if that solution seems leaner for the OS, it executes first the command one would search, spawning the process, and fails afterwards. In our case, I need to know which one is present, get it's path from stdout (I simply don't care about the exit code) then reuse that path in several places. Using what the article says would require rewriting all the script.

sudo : I currently don't know how to check that sudo will trigger a password prompt. I'll eventually investigate this later.

@kb100

This comment has been minimized.

Show comment
Hide comment
@kb100

kb100 Jan 12, 2016

Collaborator

@valir There should be no reason to rewrite the whole script beyond the top portion and then probably renaming the variable. Instead of

SUDO_PATH=$(which kdesu 2>/dev/null || echo "")
if [[ -z "$SUDO_PATH" ]]
then
  SUDO_PATH=$(which gksu 2>/dev/null || echo "")
  if [[ -z "$SUDO_PATH" ]]
  then
    # fallback to sudo
    SUDO_PATH=sudo
  fi
fi

you could use

if command -v kdesu &>/dev/null; then
  SUDO=kdesudo
elif command -v gksudo &>/dev/null; then
  SUDO=gksudo
else
  SUDO=sudo
  #TODO check whether sudo alone will work
fi

then refer to $SUDO in your script. If for some reason you really need the path to the sudo command (I don't see where in your script you would need this, so the previous suggestion should work), you can do

SUDO_PATH=$(command -v kdesudo)
if [[ -z "$SUDO_PATH" ]]
then
  SUDO_PATH=$(command -v gksudo)
  if [[ -z "$SUDO_PATH" ]]
  then
    # fallback to sudo
    SUDO_PATH=sudo
  fi
fi
Collaborator

kb100 commented Jan 12, 2016

@valir There should be no reason to rewrite the whole script beyond the top portion and then probably renaming the variable. Instead of

SUDO_PATH=$(which kdesu 2>/dev/null || echo "")
if [[ -z "$SUDO_PATH" ]]
then
  SUDO_PATH=$(which gksu 2>/dev/null || echo "")
  if [[ -z "$SUDO_PATH" ]]
  then
    # fallback to sudo
    SUDO_PATH=sudo
  fi
fi

you could use

if command -v kdesu &>/dev/null; then
  SUDO=kdesudo
elif command -v gksudo &>/dev/null; then
  SUDO=gksudo
else
  SUDO=sudo
  #TODO check whether sudo alone will work
fi

then refer to $SUDO in your script. If for some reason you really need the path to the sudo command (I don't see where in your script you would need this, so the previous suggestion should work), you can do

SUDO_PATH=$(command -v kdesudo)
if [[ -z "$SUDO_PATH" ]]
then
  SUDO_PATH=$(command -v gksudo)
  if [[ -z "$SUDO_PATH" ]]
  then
    # fallback to sudo
    SUDO_PATH=sudo
  fi
fi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment