On 02/29/2012 10:54 PM, Uli Schlachter wrote:
/// all_categories code
Hm. And right now I wonder what about this is menu-specific. Should this really
live under menubar/? Other people will want to use this code for sure, too.
However, I don't have any good idea on where to else this should be put.
Me neither. I also thought about taking this closer to the user by I
don't know where. The other solution is to leave it be and mention about
it in documentation so the user can set the fields or manipulate with
menubar.menu_gen.all_categories in runtime.
/// esc_q function ///
This function doesn't make any sense to me. This does work correctly with ASCII,
might work OK-ish with most ISO-8859 encodings, but is certainly wrong with any
kind of unicode encoding. Since pango (and thus awesome's textbox) assume utf-8,
that's not really good.
Every single Japanese awesome user would hate you for this function. ;-)
Is this function really needed? Implementing it correctly for utf-8 should be
near to impossible and certainly nothing which should be put into a local
function.
Oh, this one is in the black sheep. I added because I had a bunch of
.desktop files lines in which ended with invisible symbols that broke
pango (I guess they were CR+LF linebreaks). I wrote this as a stub
because I didn't know what pango can handle and what it can't. Now I'll
rewrite it to something more appropriate.
/// all_icon_sizes ///
If they are private, please make them local. Also, where does this list of icon
sizes come from? Looks quite arbitrary.
I guess (since I'm not the original author) that they are spec-provided.
However it's me who shouldn't guess but STFU and check with the spec
instead:).
/// lookup_icon
I haven't read the spec, but would it make sense to make these paths
configurable? Also, menu_gen had a list of paths which looked quite identical.
Should the two modules use the same variable?
Not exactly. The variable in the menu_gen describes the folder where
.desktop files are located.
The path here is a spec-provided default directory for storing icons.
Like when you set the "Icon" value in your .desktop file to just
"firefox" (without any path) and the icon would be searched in this
directory.
+ -- or if it's equal to 'awesome'
+ if program.OnlyShowIn ~= nil and program.OnlyShowIn ~= "awesome" then
+ program.show = false
+ end
Due to me being confused, I googled this. The "NotShowIn" attribute isn't
implemented. :-)
Here's the page with OnlyShowIn/NotShowIn:
http://standards.freedesktop.org/menu-spec/latest/ar01s03.html . I added
this feature because of the user query so I'm sure there are .desktop
files that use this property.
Sorry for being so pedantic. If it bothers you, talk to jd. He is less
pedantic. :-)
No, that's actually very cool:). I don't get this kind of thoroughness
on code review at my job doing a project that operates pretty sensitive
personal data so your attitude shows the quality imposed on Awesome. It
is definitely admirable.
I will try to prepare second version of the patch on the weekend. Until
then thank you for your time and efforts!
Best regards,
Alexander
--
To unsubscribe, send mail to [email protected].
