Code review comment for lp:~ertain/elementaryos/pause-media-player-script

Revision history for this message
Sergey "Shnatsel" Davidoff (shnatsel) wrote :

"setuid" command is not shipped by default. Please use "su" instead.

And please run it through http://www.shellcheck.net/ and fix it up - that thing has some very valid suggestions. Most notably "if [ $(pgrep totem) ]; then" should be "if pgrep totem > /dev/null; then"

And are you sure you still need to hardcode DISPLAY=":0" in there? Keep in mind this specific display number may not always be valid, and there may be several active displays.
If it's still needed, you might need to iterate over the existing displays for commands that depend on it, e.g. http://askubuntu.com/questions/230835/list-existing-x-display-names provides a method.
Off the top of my head, Totem invocation in here might still require it, but Totem has a D-bus interface. You'd have to check if bug #1182301 still applies, though.

You might want to run the output of "users" through "sort -u", because a run of "users" prints my username 3 times in a row. Or not - it shouldn't really matter.

Comments still mention qdbus despite it not being used in the script.

Sorry for the late review and thanks for getting this thing to work!

review: Needs Fixing

« Back to merge proposal