Hi,
On Fri, Dec 24, 2010 at 12:31:37PM -0500, [email protected] wrote:
>
> I'm getting near completion on the fvwm-menu-desktop rewrite.
> I thought I'd solicit some comments.
Thanks, Dan. It looks good. Comments in-lined below. I tried it, seems to
work OK so far. :)
> I'm thinking of sticking an FvwmForm into the process to
> prompt for the options.
Sounds like an idea. Providing some sane defaults exist, such that one can
do:
PipeRead `fvwm-menu-desktop`
Popup xdg_menu
Without any questions should be fine.
Some very quick comments of the code based on my cursive glance.
> #!/usr/bin/perl -w
I prefer seeing this:
use warnings;
So as not to hide the fact it's there on the she-bang line.
> use strict;
> no warnings qw(uninitialized prototype);
Please don't do this. Just have warnings moan about everything.
*Especially* for undef values. We can fix them up later, and *should* so as
not to overlook side-effects from things. Never hide them in this way.
I don't like prototypes -- PCP doesn't advocate them either.
Also, we'll want to enforce a version >=5.8 I feel:
use version '5.0008';
Or somesuch. I dread to think what some UN*Xes still ship with.
> use XML::Parser;
Add this to the top of the file, along with the other "use" statements.
> my $root_menu;
> my $help;
In sub icon_init, we see loops like this:
> foreach $i (keys(%DI)) {
> foreach $i (qw(fvwm_app fvwm_folder fvwm_title fvwm_toptitle)) {
But these are lexically scoped as global to the function, and not for the
duration of the loop. This has nasty side-effects (or can have) if the loop
never runs, leaving the older value in place. It's perhaps better to scope
them like this:
foreach my $foo (...) { ... }
> while ((my $key, my $value) = each %dmicon) {
Style nit:
while( my($key, $value) = each %dmicon ) { .. }
I see also some subroutine prototypes. Ugh. :) No prototypes please. :)
> print STDERR "Read desktop entry, opening file $file.\n";
I've seen other instances of this. Would be better to use "warn" here.
Maybe consider "use carp" and croak()ing?
Things like:
> open( FILE, "<$file" ) or return;
I prefer to see:
open( my $fh, '<', $file ) or warn "Couldn't open $file: $!" and return;
It would be nice to *not* use bareword filehandles, BTW, for open calls, but
use lexically-scoped ones, as shown above. But there's many insances where
this still applies.
> sub interpret_Exclude {
> my ( $tree, $entries, $pool ) = @_;
>
> my $i = 0;
Hmm?
> my @list = interpret_entry_node( $tree, 'Or', $pool );
>
> foreach my $entry (@list) {
>
> my $i = 0;
Woops. That'll give a warning. You probably didn't want to to declare this
outside this loop. ;)
Scanning the rest reveals much the same sort of repition as the above in all
other places it occurs. There's a few oddities in the way the XML stuff
works, such as this:
> else {
> $i++;
> $i++;
> }
But that's not your fault -- it was like that in the original code. It just
looks nasty to me. :)
I appreciate most of the comments above might not relate to the work you've
done, so don't think I'm picking holes unnecessarily. On the contrary, I
really appreciate this, it's looking good. But it's these smaller nits that
might get over-looked otherwise.
Ideally, I'd love it if this could go in CVS so it can be worked on
collaboratively. I don't plan on releasing the CVS version anytime soon.
Dan, thanks a lot for this, it's a fantastic start. :)
-- Thomas Adam
--
"It was the cruelest game I've ever played and it's played inside my head."
-- "Hush The Warmth", Gorky's Zygotic Mynci.
