On Thu, Oct 20, 2011 at 01:07:07PM +0200, Thomas Funk wrote:
> >On Thu, Oct 20, 2011 at 12:12:43PM +0200, Thomas Adam wrote:
> >Thanks, but I'd like to see a unified diff, not an entire file.
>
> Uuups, sorry :S
>
> Attached.
Some very quick observations...
+my $xdg_data_home = $ENV{XDG_DATA_HOME} || "$ENV{HOME}/.local/share";
+my $xdg_menu_prefix = $ENV{XDG_MENU_PREFIX} || '';
+my %xdg_menus;
my @PATH_DIRS = split(':',$ENV{PATH}); # for checking if applications exist
+
Why?
my $version = '2.6.4';
my $menu_prefix='FvwmMenu';
my $DefaultAppDirs;
@@ -116,19 +144,22 @@
my $charset = 'iso-8859-1';
my $root_cmd;
my $kde_cmd;
+my $menu_content = 'applications';
+my $global = 0;
What's global here?
my $die_on_error = 0;
my $verbose = 0;
+my $desktop;
my @language_keys;
#my @accessed_files;
my $TERM_CMD = "xterm -e";
-
Why?
my %Desktop_entries;
my %Directory_entries;
my $root_menu;
+my $user_menu;
my $help;
# Default for the mini-icons is mini/ (relatively to the ImagePath)
@@ -202,14 +233,20 @@
"check-app!" => \&obsolete,
"time-limit=s" => \&obsolete,
"merge-user-menu" => \&obsolete,
- "desktop=s" => \&obsolete,
+ "desktop=s" => \$desktop,
"su_gui" => \$root_cmd,
"kde_config" => \$kde_cmd,
- "verbose" => \$verbose
+ "content=s" => \$menu_content,
+ "global" => \$global,
+ "verbose" => \$verbose
);
icon_init();
+if (defined $desktop) {
+ $xdg_menu_prefix = "$desktop-";
+}
+
# kde-config helps us find things.
# sometimes it has funny names.
# we can get by without it.
@@ -225,7 +262,25 @@
$DefaultAppDirs = get_app_dirs();
$DefaultDirectoryDirs = get_desktop_dirs();
-$root_menu = get_root_menu();
Would be nice to use File::BaseDir to be honest for all of this.
+# get menus
+unless ($global) {
+ # first check user home
+ foreach my $dir (split(/:/, $xdg_config_home)) {
+ my $user_file_list = get_file_list("$dir/menus", 0);
What's 0 here supposed to signify?
+ print "\n";
+ foreach my $prefix (sort (keys %xdg_menus)) {
+ warn "\tDEBUG: ${prefix}menu is \'$xdg_menus{$prefix}\'.";
Read up on the qq operator here.
+foreach my $prefix (sort (keys %xdg_menus)) {
You sort this a lot -- why? Can you not sort %xdg_menus once, or better yet
Tie it?
- if ($file !~ /^\// and defined $basedir)
- {
- $file = "$basedir/$file";
- }
-
unless (defined $basedir)
{
$basedir = $file;
$basedir =~ s/\/[^\/]*$//;
+ } else {
+ if ($file !~ /^\// and defined $basedir)
+ {
+ $file = "$basedir/$file";
+ } else {
+ $basedir = $file;
+ $basedir =~ s/\/[^\/]*$//;
Do we guarantee "-d $basedir" at all with all this mangling? See File::Copy
for instance.
+sub get_file_list
+{
+ my ($path, $subdirs) = @_;
+ $subdirs = defined($subdirs)?$subdirs:0;
But $subdirs is always defined here -- you're passing it in. Note that the
idiom you're referring to is:
$subdirs ||= 0;
+ my %files = ();
+ push (my @all_directories, $path);
Why?
+ foreach my $dir (@all_directories) {
+ opendir (my $IN, $dir) || return {};
+ my @all = readdir($IN);
+ closedir $IN;
+
+ foreach my $file (@all) {
+ next if ($file eq '..' || $file eq '.');
You can avoid this by not slurping readdir into an array.
my @all = grep { !/^\.+/ and -d } readdir(...);
And note that @all needs a better name.
+ if (-d "$dir/$file") {
+ if ($subdirs == 1) {
+ push (@all_directories, "$dir/$file");
+ $files{"$dir/$file"} = $file;
Nasty to use a key name like that.
-# Fixme, remove unsupported options.
+sub get_prefixes_and_menus {
+ my $file_list = shift;
+ my $verbosecheck = 0;
+ foreach my $file (sort (keys %{$file_list})) {
Why sort?
+ next if (-d $file);
Unlikely.
+ if ($file_list->{$file} =~ m/(.*)$menu_content.menu(.*)/) {
This is a little ugly, given the interolation each time. Substr() the
thing instead.
+ my $front_prefix = $1;
+ my $back_prefix = $2;
You should always check these, especially given the greedy patten match.
+sub create_fvwm2_menu_hash {
+ my $menu = shift;
+ my %new_menu = ();
+ my @reference_menu = split("\n", $menu);
+ # first create menu hashes
+ foreach my $line (@reference_menu) {
+ if ($line =~ m/^DestroyMenu \"(.*)\"/) {
+ $new_menu{$1} = ();
+ }
+ }
my %new_menu = map {
/^DestroyMenu \"(.*)\"/ and $1 => ''
} split("\n", $menu);
Assigning an empty list literal as you've done is meaningless in this
context.
+ # add menu entries
+ foreach my $key (keys %new_menu) {
+ my $start = 0;
+ foreach my $line (@reference_menu) {
+ if ($line =~ m/^DestroyMenu \"$key\"/) {
But you can eliminate this outerloop based on the subset of data in
%new_menu, and remove this check.
+sub merge_fvwm2_menus {
+ my $weight = 0;
+ my $reference_menu;
+ # check first if xdg_menu_prefix is set
+ if ($xdg_menu_prefix ne '') {
+ $reference_menu = $xdg_menu_prefix;
+ } else {
+ # check if applications.menu exist
+ if (exists $fvwm2_menus{''}) {
The only time you'd get this is due to autovivication, where you've failed
in your data validation. This check is not the right thing to do, and you
should evaluate where the undef/empty string condition is occuring.
+ %{$fvwm2_menus{'fvwm-'}} = %{$fvwm2_menus{$reference_menu}};
+ foreach my $compare_menu (keys %fvwm2_menus) {
+ next if ($compare_menu eq $reference_menu || $compare_menu eq 'fvwm-');
+ foreach my $compare_menu (keys %fvwm2_menus) {
+ next if ($compare_menu eq $reference_menu || $compare_menu eq
'fvwm-');
+ foreach my $refkey (keys %{$fvwm2_menus{$reference_menu}}) {
+ foreach my $compkey (keys %{$fvwm2_menus{$compare_menu}}) {
Ouch, this is O(logn2) which is going to be very expensive.
[...]
I don't mind the changes so far, Thomas, they're fine, but I am not happy
with the recursive nature of these loops, the global state we're still
banding about, and the lack of a coherent structure overall for all of the
data you're manipulating.
Can you describe the internal data overall, and now it's derived, and then
we can look at how best to process it. That way, we can reduce a lot of the
clutter, and define a more cleaner interface for all of this.
HTH,
-- Thomas Adam
