elementary OS

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

pause-media-player-script
Merge into default-settings-trusty

Revision history for this message

Jason Anderson (ertain) wrote on 2015-02-17:

Okay, I've got some good news and some bad news.

The good news is that I have taken out the calls to "setuid" and
substituted "su". That part seems to work. Also I have solved the
duplicate user problem as well as that Totem problem. Now for the bad news.

When I try out the script (start the music, place it in /etc/pm/sleep.d,
put the computer to sleep [hibernation or suspend], and then bring the
system back up), the system waits a full second before
suspending/hibernating. I have no clue why this happens. I looked for
a bottleneck, but I can't find the source. It could have something to
do with the command "users | tr ' ' '\n'" that I put in to weed out the
duplicate users. It could have something to do with ConsoleKit and
DBus. I don't really know. And worse, it can shut off WiFi (what I
think happens is that, when the computer comes back from sleep it
doesn't tell the wireless to wake up).

Anyway, I'll work on this and keep you posted.

On 02/09/2015 02:06 PM, Sergey "Shnatsel" Davidoff wrote:
> Review: Needs Fixing
>
> "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!
>

Okay, I've got some good news and some bad news.

The good news is that I have taken out the calls to "setuid" and
substituted "su".  That part seems to work.  Also I have solved the
duplicate user problem as well as that Totem problem.  Now for the bad news.

When I try out the script (start the music, place it in /etc/pm/sleep.d,
put the computer to sleep [hibernation or suspend], and then bring the
system back up), the system waits a full second before
suspending/hibernating.  I have no clue why this happens.  I looked for
a bottleneck, but I can't find the source.  It could have something to
do with the command "users | tr ' ' '\n'" that I put in to weed out the
duplicate users.  It could have something to do with ConsoleKit and
DBus.  I don't really know.  And worse, it can shut off WiFi (what I
think happens is that, when the computer comes back from sleep it
doesn't tell the wireless to wake up).

Anyway, I'll work on this and keep you posted.

On 02/09/2015 02:06 PM, Sergey "Shnatsel" Davidoff wrote:
> Review: Needs Fixing
> 
> "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!
>

« Back to merge proposal