Re: my first useful program...any corrections/suggestions?

[Brandon McCaig]

On Fri, Nov 02, 2012 at 03:00:08AM +0200, Thanos Zygouris wrote:
> Also, i couldn't find any reference that \d in regex is
> different from [0-9] (or [:digit:]), but i'm interested for
> more information about it.

I wouldn't have found it either if I didn't know to look for it.
I guess I have this mailing list to thank for that. :) See
'Digits' in `perldoc perlrecharclass'. :) I guess you can
alternatively use \d with /a, but [0-9] isn't many more
characters and should always work whereas /a might affect other
code (and it also requires the reader to know what it does).

> So, here is the new code (i'm pretty happy with how it became):

Looks good to me. The only part that I question is this:

> my $amixer_cmd = "$amixer_path sset Master,0 $amixer_vol";
> my $amixer;
> run(
> command => $amixer_cmd,
> verbose => 0,
> buffer  => \$amixer,
> );

It seems that IPC::Cmd::run can accept an array reference for the
command so perhaps that would be better.

my $amixer_cmd = [$amixer_path, 'sset', 'Master,0', $amixer_vol];

In this case $amixer_vol is currently trusted though so maybe I'm
just being paranoid. :) Just be careful not to let the user
directly input that and make sure that you are careful about
shell meta characters.

Regards,


-- 
Brandon McCaig  
Castopulence Software 
Blog 
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

Re: my first useful program...any corrections/suggestions?

[Jim Gibson]

On Nov 1, 2012, at 6:00 PM, Thanos Zygouris wrote:

> @Brandon: I'll stick with the m//, mostly because it reminds me that i can use
> any delimiter instead of the slashes. I hope to get more experience tho, and
> get rid of it. Also, i couldn't find any reference that \d in regex is
> different from [0-9] (or [:digit:]), but i'm interested for more information
> about it.

You don't have to worry about the difference between \d and [0-9] unless you 
are dealing with non-Ascii characters. For Ascii, they are the same. For 
non-Ascii (e.g., UTF), \d will match additional numeric characters. Whether or 
not that is desirable depends upon your needs and expectations. As I never deal 
with non-Ascii characters, I can't make any recommendations on using or not 
using \d in those situations.


--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/

Re: my first useful program...any corrections/suggestions?

[Thanos Zygouris]

Thank you both for your suggestions.

@Shlomi: It seems that i'm completely unable to implement dispatch tables here
(but i now know what they are, hope to use them in the near future), so i used
the given-when approach...it seemed better than endless if-elsif's.

@Brandon: I'll stick with the m//, mostly because it reminds me that i can use
any delimiter instead of the slashes. I hope to get more experience tho, and
get rid of it. Also, i couldn't find any reference that \d in regex is
different from [0-9] (or [:digit:]), but i'm interested for more information
about it.

@both: Thanks again for looking into my code. You helped me learn a lot more
(and in less time) than try to figure them out myself. I tried to use most
of your suggestions...hope I've done it well enough.

So, here is the new code (i'm pretty happy with how it became):

#!/usr/bin/perl

use strict;
use warnings FATAL => qw( uninitialized );
use v5.16;

use X::Osd;
use IPC::Cmd qw( can_run run );

# check if amixer exists and get its path
my $amixer_path = can_run('amixer') 
or die 'amixer is not available';

# 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 = $ENV{OSD_VOLUME} // "$ENV{HOME}/.osd-volume.fifo";
unless (-p $fifo_file) {
# if anyother filetype is there, just die
if (-e $fifo_file) {
die "$fifo_file: not a named pipe";
}
else {
# create the named pipe
require POSIX;
POSIX::mkfifo( $fifo_file, 0600 )
or die "cannot mkfifo $fifo_file: $!";
}
}

# open named pipe
open( my $fifo_fh, "+<", $fifo_file ) 
or die "cannot open $fifo_file: $!";

my $amixer_vol;

# constantly read from it
FIFO_INPUT:
while (chomp( my $fifo_input = <$fifo_fh> )) {
given ($fifo_input) {
when ('up') { $amixer_vol = '3%+'; }
when ('down')   { $amixer_vol = '3%-'; }
when ('toggle') { $amixer_vol = 'toggle'; }
when ('exit')   { last FIFO_INPUT; }
default {
warn "$fifo_input: invalid input";
next FIFO_INPUT;
}
}

# set new volume value and read the output
my $amixer_cmd = "$amixer_path sset Master,0 $amixer_vol";
my $amixer;
run(
command => $amixer_cmd,
verbose => 0,
buffer  => \$amixer,
);

# red output color if sound mutes, blue otherwise
my $colour = ( index($amixer, '[off]') != -1 ) ? '#DD' : '#1E90FF';
$osd->set_colour($colour);

# get new volume value and print osd bar
if (my ($volume) = $amixer =~ m/(\d{1,3})%/ ) {
$osd->string(0, "Master Volume:${volume}%");
$osd->percentage(1, $volume);
}
}

# close pipe before exit
close($fifo_fh);
exit(0);

-- 
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/

Re: my first useful program...any corrections/suggestions?

[Brandon McCaig]

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("#DD");
> } 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\]/ ? '#DD' : '#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  
Castopulence Software 
Blog 
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

Re: my first useful program...any corrections/suggestions?

[Shlomi Fish]

Hi Thanos,

some comments about your code.

On Wed, 31 Oct 2012 13:27:05 +0200
Thanos Zygouris  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 = )) {
> 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("#DD");
> } 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/

my first useful program...any corrections/suggestions?

[Thanos Zygouris]

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;
# extra modules
use X::Osd;

# 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");
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: $!";
# constantly read from it
my $vol;
while (chomp(my $fifo_line = )) {
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";
}
# set new volume value and read the output
my $amixer = `amixer sset Master,0 $vol` or die "error: $!";
# get new volume value
$vol = $1 if ($amixer =~ m/(\d{1,3})(?:%)/);
# change output color if volume is muted
if ($amixer =~ m/\[off\]/) {
$osd->set_colour("#DD");
} 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);


-- 
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/