The structure of the parameters passed to the procedures, and the seeming large number of these, leads either to a large number of "temporary" variables to store them, which simply leads to confusion when it comes to maintenance, or to code which seems definitively unclear.

From having written a number of fairly large Tk programs, I've developed, and try to stick to, a style of careful spacing when it comes to defining and using widgets. While I'm not certain this is the best method of writing a script, it does serve to make the previously horrible-to-look-at calls much cleaner, with obvious benefits when debugging and maintaining the code.

I do make use of global variables with Tk, simply because it makes the use of subs to carry out functions much easier. Avoiding this, as you say, results in fairly unclear code.

I personally think that neither style is definitively "better". If the script can be easily understood, maintained and extended, then whichever style has been used is obviously the better one. This, I think, depends muchly on the length and complexity of the script, and is something best determined while thinking about the overall design of the program.

I guess my opinion is to try and keep stuff as simple as possible, when it comes to layout, variable referencing and parameter passing, hopefully giving something that you can work from in the future.

Just some quick thoughts ..
--Foxcub

my $cmds = $tl->Frame;
$cmds->pack;
    
if (defined $idx) {
# Existing activity - OK Apply Delete Cancel
    $cmds->Button(-text => 'OK', -command => sub {&edit_entry($idx,$le
+gentry);$tl->destroy;})
         ->pack(-side => 'left');
    $cmds->Button(-text => 'Apply', -command => [\&edit_entry, $idx, $
+legentry])
         ->pack(-side => 'left');
    $cmds->Button(-text => 'Delete', -command => sub {&delete_entry($i
+dx);$tl->destroy;})
            ->pack(-side => 'left');
} else {
# New activity - OK Apply Cancel
    $cmds->Button(-text => 'OK', -command => sub {&add_entry($legentry
+);$tl->destroy;})
         ->pack(-side => 'left');
    $cmds->Button(-text => 'Apply', -command => [\&add_entry, $legentr
+y])
         ->pack(-side => 'left');
}
$cmds->Button(-text => 'Cancel', -command => [$tl => 'destroy'])->pack
+(-side => 'left');
[download]

my @button = defined $idx ? (
    OK     => sub { &edit_entry($idx,$legentry); $tl->destroy; },
    Apply  => [\&edit_entry, $idx, $legentry],
    Delete => sub { &delete_entry($idx); $tl->destroy; },
    Cancel => [$tl => 'destroy'],
)
: (
    OK     => sub { &add_entry($legentry); $tl->destroy; },
    Apply  => [\&add_entry, $legentry],
    Cancel => [$tl => 'destroy'],
);

for($tl->Frame) {
    $_->pack;
    while(my ($text, $command) = splice @button, 0, 2) {
        $_->Button(-text => $text, -command => $command)
          ->pack(-side => 'left');
    }
}
[download]

Note how the layout now directly conveys the information you previously had to sum up in comments.

If you don't like the scoping to main:: (I don't either), and you have a relatively involved data structure, that usually means you should abstract out your backend logic into a class. Use Class::Accessor to save the hassle of writing a lot of accessors and mutators and life is good. See also Damian Conway's ten rules for when to use OO.

F.ex given a class Activity, and with a slightly different INI format, you could have wittled down this:

print SAVE <<END;
for (0..$#activity) {
    my ($legend,$time,$start,$end,$elapsed) = 
        @{%{$activity[$_]}}{qw(legend timstr start end elapsed)};

# Add up times of current activity
    my $tsecs = 0;
    for my $i (0..$#$end) {
        $tsecs += tv_interval($start->[$i],$end->[$i]);
    }
    $tsecs += $elapsed if $elapsed;
    $tsecs += tv_interval($start->[-1],[gettimeofday]) if defined($cur
+rent) && ($_ == $current);
    print SAVE <<END2;
[Button $legend]
time=$time
elapsed=$tsecs
END2
}
[download]

for (@activity) {
    print SAVE "[Button]\n";
    my $item;
    print SAVE "$item=" . $_->$item() for qw(legend time elapsed);
}
[download]

with calculation for time elapsed being hidden away at the particular spot, since it's not where it logically belongs anyway.

In general, try to abstract a bit more - capture regularities in code, irregularities in data.

Makeshifts last the longest.

Thank you Aristotle, this is exactly the kind of feedback I was looking for.

Your idea for handling the buttons in the activity_configure sub is excellent - very clear and concise. Though I would probably have used a hash instead of @button, and used each instead of splice.

Regarding OO, this has prompted me to review and assess the plethora of Class::* modules on CPAN. Having activity as a class will present a neater way of dealing with the callbacks, as methods, but it still looks that "Next" will be difficult to implement. Can I get the toplevel widget to return me the next frame I wonder?

Regarding the .ini save, I prefer my original, as the heredoc stands out and makes it obvious what is happening. Admittedly, I could refactor the time calculation into a sub - which may become a method in my new class.

But the way the save loop is set up, it reduces some duplication, and you can still add a # SAVE ACTIVITIES to make it stand out. :-) Actually, I didn't take it far enough - it should have been:

for (@activity) {
    print SAVE "[Button]\n";
    my $item;
    print SAVE "$prop=" . $_->$prop() for $_->properties;
}
[download]

where properties() simply looks like sub properties { qw(legend time elapsed) } and then the INI saving loop doesn't require any maintance at all any more.

Makeshifts last the longest.

I prefer analog displays for clocks, and I wrote a Tk lab timer to prevent burning my food while reading Perlmonks!

It would be cool to have multiple independent clocks in one window, with options for which direction the clocks run, and control over how fast the hands spin.

I also like command-line options to be available, so I can type instead of using the mouse.

It should work perfectly the first time! - toma