-----Urspr√ľngliche Nachricht-----
Von: "Thomas Adam" <tho...@fvwm.org>
Gesendet: 20.10.2011 15:00:58
An: "Thomas Funk" <t.f...@web.de>
Betreff: Re: Reworked fvwm-menu-desktop

>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?
because it's follows the xdg spec.
Ok, $xdg_data_home not used. so can be deleted.
%xdg_menus is the hash to hold the founded menus:
%xdg_menus = { 'xdg_prefix' => 'file' }

> 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?
I use it for system wide menus. If not set (0) look first in $xdg_config_home
and if empty look in $xdg_config_dirs. Ok, perhaps bad coice of command name ...

>my $die_on_error = 0;
> my $verbose = 0;
>+my $desktop;
>
> my @language_keys;
>
> #my @accessed_files;
> my $TERM_CMD = "xterm -e";
>
>-

>Why?
because it's a command line variable? I thought I need it. It's like the same
as with $kde_cmd. Or not?

[...]
>
>+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.
Must RTFM first ...

>+# 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?
If $global = 0 (default) then the user home have to check first (xdg spec)
Is "if (!$global)" better?

>+ print "\n";
>+ foreach my $prefix (sort (keys %xdg_menus)) {
>+ warn "\tDEBUG: ${prefix}menu is \'$xdg_menus{$prefix}\'.";

>Read up on the qq operator here.
Ok, will do

>+foreach my $prefix (sort (keys %xdg_menus)) {

>You sort this a lot -- why?
I sort it because an empty key ('' = applications.menu) should start first.
But I see, in this case it is useless.
>Can you not sort %xdg_menus once, or better yet Tie it?
I red often that hash keys cannot sorted. They will be written
randomly into the hash structure. So, therefore I sort them ever i need 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.
The problem here was that not all included menus found.
This happens on my Debian system:
$HOME/.config/menus/gnome-applications.menu includes the gnome-applications.menu
which is located in /etc/xdg/menus. This menu includes the Debian menu which was
also located in /etc/xdg/menus/. The reading of this menu failed in the old
implementation because the script wants to read it under $HOME/.config/menus/

>+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;

Hmm .. ok. I confess that I've copied the function from my work on
fvwm-themes-config :S and this code is from a site about file handling.
I rewrite it for my needs and it fits here to get the files I needed.
Must rethink about the implementation (blush) ...

>+ 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.
I thought pattern matching is faster than working with substrings :S

>+ # 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.

Yes, I know the loops in sub merge_fvwm2_menus() are very heavily. But in 
principle
it works. And that was the main approach for me first.

The description will follow in the next mail. Thanks Thomas for the good tips 
and hints.

HTH,
Thomas


___________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192

Reply via email to