Re: Reworked fvwm-menu-desktop

2011-11-01 Thread Thomas Funk
Hi Thomas,

-Ursprüngliche Nachricht-
Von: Thomas Adam tho...@fvwm.org
Gesendet: 31.10.2011 00:39:58
Hi,

[ I have no time at all to really give this the attention it deserves, so
I'm going to have to be brief and merely point you in the right directions.
I will though expect this to go through several iterations as a result. ]

It's more than I've looked forward to :-)

This is why your structure is incorrect -- and leads to inefficiencies like
this. What you actually have in the abstract case is a structure such as
this:

# This is just me thinking...
%menu = (
 'fvwm' = {
 'name' = mynicenewmenuname,
 'items' = [
 {
 'label' = 'label4',
 'action' = $some_action4,
 'icon' = undef,
 },
 {
 'label' = 'label3',
 'action' = $some_action3,
 'icon' = undef,
 }
 ],
 }
);

Note that you might have multiple item sections, perhaps where each section
has a separator line (+ Nop) defined in it. How this translates from
{gnome,kde}- prefix menus, I don't know. It's down to you to find out.

I constructed create_fvwm2_menu_hash() and merge_fvwm2_menus() because I don't
understand the structure Dan created in read_menu() ... but now it's more
important to analyze it deeper, than fix merge_fvwm2_menus() because using his
structure is the better way - it's in one context ...

Dan, if i need help to understand your work, would you help me? Would be great 
...

thanks,
Thomas

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



Re: Reworked fvwm-menu-desktop

2011-11-01 Thread Thomas Adam
On Tue, Nov 01, 2011 at 01:07:27PM +0100, Thomas Funk wrote:
 I constructed create_fvwm2_menu_hash() and merge_fvwm2_menus() because I don't
 understand the structure Dan created in read_menu() ... but now it's more
 important to analyze it deeper, than fix merge_fvwm2_menus() because using his
 structure is the better way - it's in one context ...
 
 Dan, if i need help to understand your work, would you help me? Would be 
 great ...

Dan didn't construct that structure, that's how it was originally based on
parsing the XML files for the XDG menu definitions, and it's always been the
structured formed from the XDG data which I've had an issue with, since it's
very inefficient.

And it's this, including my reply to you previously in this thread, which I
was hoping you'd be addressing.  Yes, the XDG spec talks about merging items
together, etc., and that's fine, but making that a direct facet of how the
data is structured and stored is not the bes thing to do, IMO.

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



Re: Reworked fvwm-menu-desktop

2011-11-01 Thread Thomas Funk
-Ursprüngliche Nachricht-
Von: Thomas Adam tho...@fvwm.org
Gesendet: 01.11.2011 13:12:56

Dan didn't construct that structure, that's how it was originally based on
parsing the XML files for the XDG menu definitions, and it's always been the
structured formed from the XDG data which I've had an issue with, since it's
very inefficient.

And it's this, including my reply to you previously in this thread, which I
was hoping you'd be addressing. Yes, the XDG spec talks about merging items
together, etc., and that's fine, but making that a direct facet of how the
data is structured and stored is not the bes thing to do, IMO.

Ok, I will try to find a better structure or way for the data ... but i cannot 
promise anything ...

Cheers,
Thomas

--
Life is like a box of chocolates - never know what you're gonna get.


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



Re: Reworked fvwm-menu-desktop

2011-10-30 Thread Thomas Funk
-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...
[...]

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.

Hi Thomas,
sorry for the delay, but I was seriously ill the last days. But now I've done 
the description of merge_fvwm2_menus.
Hope it's ok:


sub merge_fvwm2_menus {
    my $weight = 0;
    my $reference_menu;
    # check first if xdg_menu_prefix is set
    if (defined $xdg_menu_prefix) {
    $reference_menu = $xdg_menu_prefix;
    } else {
    # check if applications.menu exist
    # question: correct to disable autovivification?
    if (ref $fvwm2_menus{''} eq 'HASH') {
    $reference_menu = '';
    } else {
    # get the biggest menu as reference
    foreach my $prefix (keys %fvwm2_menus) {
    my $size = scalar keys %{$fvwm2_menus{$prefix}};
    if ($size  $weight) {
    $reference_menu = $prefix;
    $weight = $size;
    }
    }
    }
    }
    ## compare the other menus with reference and delete same entries
    # create an fvwm menu hash
    %{$fvwm2_menus{'fvwm-'}} = %{$fvwm2_menus{$reference_menu}};
    # get the first hash key ('gnome-', 'kde-', 'fvwm-')
    foreach my $compare_menu (keys %fvwm2_menus) {
    # don't check the reference or the fvwm menu
    next if ($compare_menu eq $reference_menu || $compare_menu eq 'fvwm-');
    # get the menu names from the reference menu (check the known menus for 
same entries)
    foreach my $refkey (keys %{$fvwm2_menus{$reference_menu}}) {
    # get the menu names from the menu should compared
    foreach my $compkey (keys %{$fvwm2_menus{$compare_menu}}) {
    # compare array A (@{$fvwm2_menus{$compare_menu}{$compkey}}) to 
Array B (@{$fvwm2_menus{$reference_menu}{$refkey}})
    # and removing B elements from A. Exception: DestroyMenu and 
AddToMenu entires
    my @rest = grep { my $x = $_; not grep { $x =~ /\Q$_/i and $x 
!~ /DestroyMenu|AddToMenu/} @{$fvwm2_menus{$reference_menu}{$refkey}} } 
@{$fvwm2_menus{$compare_menu}{$compkey}};
    # clear compared array
    undef @{ $fvwm2_menus{$compare_menu}{$compkey} };
    # push cleared array into compare array
    push @{ $fvwm2_menus{$compare_menu}{$compkey} }, @rest;
    # if only DestroyMenu or AddToMenu entries exist, delete this 
menu name hash
    if (scalar @{ $fvwm2_menus{$compare_menu}{$compkey} } == 2) {
    delete $fvwm2_menus{$compare_menu}{$compkey};
    }
    }
    }
    ## check if double entries in the compare menu exist and delete them
    # this happens on OpenSuSE 11.4 where in System AND Settings the same 
app entries exist
    foreach my $refkey (keys %{$fvwm2_menus{$compare_menu}}) {
    foreach my $compkey (keys %{$fvwm2_menus{$compare_menu}}) {
    next if ($compkey eq $refkey);
    my @rest = grep { my $x = $_; not grep { $x =~ /\Q$_/i and $x 
!~ /DestroyMenu|AddToMenu/} @{$fvwm2_menus{$compare_menu}{$refkey}} } 
@{$fvwm2_menus{$compare_menu}{$compkey}};
    undef @{ $fvwm2_menus{$compare_menu}{$compkey} };
    push @{ $fvwm2_menus{$compare_menu}{$compkey} }, @rest;
    }
    # if only DestroyMenu or AddToMenu entries exist in a reference 
menu, delete the menu name hash
    if (scalar @{ $fvwm2_menus{$compare_menu}{$refkey} } == 2) {
    delete $fvwm2_menus{$compare_menu}{$refkey};
    }
    }
    ## merge all '+' entries in existing ref menus
    foreach my $key (keys %{$fvwm2_menus{$compare_menu}}) {
    # don't check the fvwm root menu. This we will do later
    next if ($key eq 'FvwmMenu');
    # if menu name from compare menu exists in reference menu try 
merging
    # question: is defined correct to disable autovivification?
    if (defined $fvwm2_menus{$reference_menu}{$key}) {
    # get all + entries except with Popup|DestroyMenu|AddToMenu
    my @entries = (grep (/\+/, 
@{$fvwm2_menus{$compare_menu}{$key}}) and grep (!/Popup|DestroyMenu|AddToMenu/, 
@{$fvwm2_menus{$compare_menu}{$key}}));
    # if entries array has values, push lines into reference menu
    if (@entries) {
    push @{ $fvwm2_menus{$reference_menu}{$key} }, @entries;
    # get the rest like Popup|DestroyMenu|AddToMenu entries to 
create the
    # compare menu without the + entries moved to the reference
    my @rest = grep

Re: Reworked fvwm-menu-desktop

2011-10-30 Thread Thomas Adam
Hi,

[ I have no time at all to really give this the attention it deserves, so
I'm going to have to be brief and merely point you in the right directions.
I will though expect this to go through several iterations as a result. ]

On Sun, Oct 30, 2011 at 11:54:29PM +0100, Thomas Funk wrote:
 -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...
 [...]
 
 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.
 
 Hi Thomas,
 sorry for the delay, but I was seriously ill the last days. But now I've done 
 the description of merge_fvwm2_menus.

It's fine.  I wasn't interested in the descriptions actually, since I can
read your code fine.  I wanted the internal structure which you've now
given.

 sub merge_fvwm2_menus {
     my $weight = 0;
     my $reference_menu;
     # check first if xdg_menu_prefix is set
     if (defined $xdg_menu_prefix) {
     $reference_menu = $xdg_menu_prefix;
     } else {
     # check if applications.menu exist
     # question: correct to disable autovivification?
     if (ref $fvwm2_menus{''} eq 'HASH') {
     $reference_menu = '';


This is ultimately incorrect, but I think you now understand why.

     } else {
     # get the biggest menu as reference
     foreach my $prefix (keys %fvwm2_menus) {
     my $size = scalar keys %{$fvwm2_menus{$prefix}};


Can just say:

my $size = (keys %{...});

     ## compare the other menus with reference and delete same entries
     # create an fvwm menu hash
     %{$fvwm2_menus{'fvwm-'}} = %{$fvwm2_menus{$reference_menu}};
     # get the first hash key ('gnome-', 'kde-', 'fvwm-')
     foreach my $compare_menu (keys %fvwm2_menus) {
     # don't check the reference or the fvwm menu
     next if ($compare_menu eq $reference_menu || $compare_menu eq 
 'fvwm-');

     # get the menu names from the reference menu (check the known menus 
 for same entries)
     foreach my $refkey (keys %{$fvwm2_menus{$reference_menu}}) {
     # get the menu names from the menu should compared
     foreach my $compkey (keys %{$fvwm2_menus{$compare_menu}}) {
     # compare array A (@{$fvwm2_menus{$compare_menu}{$compkey}}) 
 to Array B (@{$fvwm2_menus{$reference_menu}{$refkey}})
     # and removing B elements from A. Exception: DestroyMenu and 
 AddToMenu entires
     my @rest = grep { my $x = $_; not grep { $x =~ /\Q$_/i and $x 
 !~ /DestroyMenu|AddToMenu/} @{$fvwm2_menus{$reference_menu}{$refkey}} } 
 @{$fvwm2_menus{$compare_menu}{$compkey}};

This is why your structure is incorrect -- and leads to inefficiencies like
this.  What you actually have in the abstract case is a structure such as
this:

# This is just me thinking...
%menu = (
'fvwm' = {
'name' = mynicenewmenuname,
'items' = [
{
'label' = 'label4',
'action' = $some_action4,
'icon' = undef,
},
{
'label' = 'label3',
'action' = $some_action3,
'icon' = undef,
}
],
}
);

Note that you might have multiple item sections, perhaps where each section
has a separator line (+ Nop) defined in it.  How this translates from
{gnome,kde}- prefix menus, I don't know.  It's down to you to find out.

Traversing this is slightly easier then, for you can now do things like:

my @menu_entries = map {
   + . $_-{'label'} . \tPopup  . $_-{'action'} . ,\n;
} @{ $menu{'fvwm'}{'items'} };

This will allow to use functions like join as well as anything else.  Note
the struture says nothing about the overall format for things like needing
{Add,Destroy}Menu lines.  These can be added at the point you generate the
menu, and do not need to be stored as part of the menu's data.  You need
only look at the rather hacked-up fvwm-convert-2.6 script to see examples of
this which I wrote.

The other benefit to the above is that traversing this also means you do not
end up grepping an array, but now can check a hash key which is O(1) and not
O(n*m) as in your current implementation.

Grepping/manipulating data which is already in the output format you're
after is almost always doomed to looping and ugly constructs because that
data is almost never in a parsable format to begin with.

HTH,

-- Thomas Adam

-- 
Deep in my heart I wish I was wrong.  But deep in my heart I know I am
not. -- Morrissey (Girl Least Likely To -- off of Viva Hate.)



Re: Reworked fvwm-menu-desktop

2011-10-20 Thread Thomas Funk
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.

--
Life is like a box of chocolates - never know what you're gonna get.

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


diff.txt
Description: Binary data


Re: Reworked fvwm-menu-desktop

2011-10-20 Thread Thomas Adam
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 

Re: Reworked fvwm-menu-desktop

2011-10-20 Thread Thomas Funk
-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

Re: Reworked fvwm-menu-desktop

2011-10-20 Thread Thomas Adam
On Thu, Oct 20, 2011 at 04:59:40PM +0200, Thomas Funk wrote:
 -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.

No, this was in reference to a blank line, which had no reason being there
(yes, I am picky.)

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

Same reason as above -- a blank line.  Why?

 I sort it because an empty key ('' = applications.menu) should start first.
 But I see, in this case it is useless.

A key of '' is a bug, not a feature.  :)

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

Yes, hash lookups are not ordered, but it wasn't that which I was asking, it
was why the need to sort at all, and why do it where it's needed?

 +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

No -- firing up the regexp engine is expensive on items where substr() is
more useful, no matter now tempting it is to do otherwise.  Especially for
interpolated content which is generally set once and then never changes.

-- Thomas Adam