Hi Thanos,
some comments about your code.
On Wed, 31 Oct 2012 13:27:05 +0200
Thanos Zygouris <athanasios.zygou...@gmail.com> wrote:
> I made a small program to display a X::Osd bar displaying my volume
> percentage (on GNU/Linux box). It works, but I'd like to have any suggestions
> or corrections about it (i'm not confident about my skills i suppose).
>
> So, here is how it works:
> 1) Have a named pipe defined at $OSD_VOLUME environmental variable.
> 2) Run the program in the background.
> 3) When echoing 'up', 'down' or 'toggle' in the named pipe, it raises,
> lowers or toggles mute state using amixer program.
>
> I have the following questions:
> 1) Is the code OK? (I mean is there anything I should avoid or add?)
> 2) Is there a better solution to make a perl program and a shell script
> and/or window manager communicate? (I really didn't love that named
> pipe solution, but I didn't know of anything else)
>
> Enough words, here is the code:
>
> #!/usr/bin/perl
>
> use strict;
> use warnings;
strict and warnings are a good idea. Well done.
> # extra modules
> use X::Osd;
The comment is not really needed. It doesn't hurt much though.
>
> # create osd bar (two output lines)
> my $osd = X::Osd->new(2);
> # osd bar properties
> $osd->set_font("-*-terminus-bold-*-*-*-18-*-*-*-*-*-*-*");
> $osd->set_shadow_offset(1);
> $osd->set_pos(XOSD_bottom);
> $osd->set_align(XOSD_center);
> $osd->set_horizontal_offset(0);
> $osd->set_vertical_offset(30);
> $osd->set_timeout(5);
>
> # locate (and if missing create) named pipe
> my $fifo_file = (defined $ENV{OSD_VOLUME}) ? $ENV{OSD_VOLUME} :
> glob("~/.osd-volume.fifo");
Better do exists instead of defined here, and if you're on perl-5.10.x you
might wish to use the "//" (defined-or) operator. The glob can be replaced by
$ENV{HOME}
> unless (-p $fifo_file) {
> # delete non-named pipe file (risky)
> unlink $fifo_file or die "cannot remove $fifo_file: $!";
> # create the named pipe
> require POSIX;
> POSIX::mkfifo($fifo_file, 0600) or die "cannot mkfifo $fifo_file: $!";
> }
>
> # open named pipe
> open(FIFO, "+<", $fifo_file) or die "cannot open $fifo_file: $!";
You should use lexical file handles instead of typeglobs.
> # constantly read from it
> my $vol;
> while (chomp(my $fifo_line = <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";
> }
I would do that using a dispatch table.
> # set new volume value and read the output
> my $amixer = `amixer sset Master,0 $vol` or die "error: $!";
You should put $vol in a more inner scope.
With `...` you risk shell-variable injection - maybe look at IPC::Run.
> # get new volume value
> $vol = $1 if ($amixer =~ m/(\d{1,3})(?:%)/);
> # change output color if volume is muted
> if ($amixer =~ m/\[off\]/) {
This can be done using perldoc -f index.
> $osd->set_colour("#DD0000");
> } else {
> $osd->set_colour("#1E90FF");
> }
> # print volume bar
> $osd->string(0, 'Master Volume:'.$vol.'%');
> $osd->percentage(1, $vol);
> }
> # close pipe and exit (with error)
> # (impossible to get here)
> close(FIFO);
> exit(0);
>
>
Regards,
Shlomi Fish
--
-----------------------------------------------------------------
Shlomi Fish http://www.shlomifish.org/
Escape from GNU Autohell - http://www.shlomifish.org/open-source/anti/autohell/
XSLT is the number one cause of programmers’ suicides since Visual Basic 1.0.
Please reply to list if it's a mailing list post - http://shlom.in/reply .
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
