Hello: I'm not familiar with X::Osd so I'm just assuming that is all happy. :)
/pedantic on
On Wed, Oct 31, 2012 at 01:27:05PM +0200, Thanos Zygouris wrote:
> # delete non-named pipe file (risky)
> unlink $fifo_file or die "cannot remove $fifo_file: $!";
You might as well just die instead of deleting an existing file.
I would consider it user error for an existing file to not be a
fifo, which means that your program should just report it and
refuse to function. It is then possible for the user to decide
what to do.
unless(-p $fifo_file) {
if(-e $fifo_file) {
die "Fatal: '$fifo_file' is not a fifo.";
}
# ...
}
> if ($fifo_line eq 'up') {
> $vol = '3%+';
> } elsif ($fifo_line eq 'down') {
> $vol = '3%-';
> } elsif ($fifo_line eq 'toggle') {
> $vol = 'toggle';
> } else {
> die "invalid input: $fifo_line";
> }
It seems strange to die here. You might want to log the error to
a file instead. You probably don't want your background process
to die because of an invalid request from an external program. :)
It can just do nothing instead.
else {
# Optionally log somewhere...
next;
}
> $vol = $1 if ($amixer =~ m/(\d{1,3})(?:%)/);
As Shlomi Fish pointed out, using an outer scope for $vol and
conditionally setting it here is unnecessary. Also, it's
unnecessary to use m// with //. That's a personal preference, but
some might find that it clutters the code. It also seems that a
cluster group '(?:pattern)' is unnecessary around the % symbol.
Also I note that you're fetching the volume here, but not using
it until after the next if...else. I'd probably rearrange the
code to keep it closer to where it's needed. Also, I believe that
\d can match much more than [0-9]. You might prefer to use [0-9]
instead, assuming that is what you meant.
> # change output color if volume is muted
> if ($amixer =~ m/\[off\]/) {
> $osd->set_colour("#DD0000");
> } else {
> $osd->set_colour("#1E90FF");
> }
I see the two calls to X::Osd::set_colour as redundant. I'd
probably normalize them into a single call with a variable
colour.
> # print volume bar
> $osd->string(0, 'Master Volume:'.$vol.'%');
> $osd->percentage(1, $vol);
It seems to me that there's no point to updating the string or
percentage if the volume hasn't been updated. I might write that
like this:
my $colour = $amixer =~ /\[off\]/ ? '#DD0000' : '#1E90FF';
$osd->set_colour($colour);
my ($vol) = $amixer =~ /([0-9]{1,3})%/;
if(defined $vol)
{
$osd->string(0, "Master Volume: ${vol}%");
$osd->percentage(1, $vol);
}
That's my two cents. :)
Regards,
--
Brandon McCaig <[email protected]> <[email protected]>
Castopulence Software <https://www.castopulence.org/>
Blog <http://www.bamccaig.com/>
perl -E '$_=q{V zrna gur orfg jvgu jung V fnl. }.
q{Vg qbrfa'\''g nyjnlf fbhaq gung jnl.};
tr/A-Ma-mN-Zn-z/N-Zn-zA-Ma-m/;say'
signature.asc
Description: Digital signature
