Hi,
since the license issues are settled, let me complain about the rest of the
code. :-)
On 15.02.2012 23:22, Alexander Yakushev wrote:
> diff --git a/lib/menubar/menu_gen.lua.in b/lib/menubar/menu_gen.lua.in
> new file mode 100644
> index 0000000..620145f
> --- /dev/null
> +++ b/lib/menubar/menu_gen.lua.in
[...]
> +-- Menu generation module for menubar
> +module("menubar.menu_gen")
> +
> +-- Options section
> +all_menu_dirs = { '/usr/share/applications/' }
> +
> +all_categories = {
> + { app_type = "AudioVideo", name = "Multimedia",
> + icon_name = "applications-multimedia.png", use = true },
> + { app_type = "Development", name = "Development",
> + icon_name = "applications-development.png", use = true },
> + { app_type = "Education", name = "Education",
> + icon_name = "applications-science.png", use = false },
> + { app_type = "Game", name = "Games",
> + icon_name = "applications-games.png", use = true },
> + { app_type = "Graphics", name = "Graphics",
> + icon_name = "applications-graphics.png", use = true },
> + { app_type = "Office", name = "Office",
> + icon_name = "applications-office.png", use = true },
> + { app_type = "Network", name = "Internet",
> + icon_name = "applications-internet.png", use = true },
> + { app_type = "Settings", name = "Settings",
> + icon_name = "applications-utilities.png", use = false },
> + { app_type = "System", name = "System Tools",
> + icon_name = "applications-system.png", use = true },
> + { app_type = "Utility", name = "Accessories",
> + icon_name = "applications-accessories.png", use = true }
> +}
These two variables should get some documentation if users are allowed to mess
with them. E.g. I can guess that all_menu_dirs is where it looks for the
.desktop files (but only since I know that this uses .desktop files in the first
place), however I don't know if the dirs in here are search recursively.
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. If this
gets split up, it might deserve its own commit...
[...]
> +
> +-- Remove non-printable characters from the string and escape single quotes.
> +-- @param s The string to protect.
> +-- @return the protected string.
> +local function esc_q(s)
> + if s then
> + -- Remove all non-printable characters
> + return string.gsub(string.gsub(s, "'" ,"\\'"),
> + "(.)", function(c)
> + if string.byte(c, 1) > 31 then
> + return c
> + else
> + return ""
> + end
> + end)
> + else
> + return ""
> + end
> +end
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.
> +--- Generate an array of all visible menu entries.
> +-- @return all menu entries.
> +function generate()
> + local result = {}
> +
> + for i, dir in ipairs(all_menu_dirs) do
> + local entries = utils.parse_dir(dir) do
> + for i, program in ipairs(entries) do
> + -- check whether to include in the menu
> + if program.show and program.Name and program.cmdline then
> + local target_category = nil
> + if program.categories then
> + for _, category in ipairs(program.categories) do
> + local category_id, cat =
> + get_category_and_number_by_type(category)
> + if category_id and cat.use then
> + target_category = category_id
> + break
> + end
> + end
> + end
> + if target_category then
> + table.insert(result, { name = esc_q(program.Name) or
> "",
> + cmdline =
> esc_q(program.cmdline) or "",
> + icon =
> utils.lookup_icon(esc_q(program.icon_path)) or nil,
> + category = target_category })
> + end
> + end
> + end
> + end
> + end
> +
> + return result
> +end
Uhm, that last function might need some comments. Perhaps even introduce some
local variables to make that table.insert call more readable. I'm not really
sure what exactly each part does.
> diff --git a/lib/menubar/utils.lua.in b/lib/menubar/utils.lua.in
> new file mode 100644
> index 0000000..d8c4cea
> --- /dev/null
> +++ b/lib/menubar/utils.lua.in
> @@ -0,0 +1,154 @@
> +---------------------------------------------------------------------------
> +-- @author Antonio Terceiro
> +-- @copyright 2009 Antonio Terceiro
> +-- @release @AWESOME_VERSION@
> +---------------------------------------------------------------------------
At least 2009-2012. :-)
I wonder what copyright headers the rest of awesome uses...
Urgh, not that I looked I wish that I didn't.
> +-- Grab environment
> +local io = io
> +local table = table
> +local ipairs = ipairs
> +local string = string
> +local awful_util = require("awful.util")
I just want to mention that I haven't seen awful_util before. Other code uses
util or awful.util. Having said that, I don't care much and it can stay like
this.
> +-- Utility module for menubar
> +module("menubar.utils")
> +
> +-- Options section
> +terminal = 'xterm'
> +default_icon = nil
> +icon_theme = nil
Some docs, please.
> +-- Private section
> +all_icon_sizes = {
> + '128x128' ,
> + '96x96',
> + '72x72',
> + '64x64',
> + '48x48',
> + '36x36',
> + '32x32',
> + '24x24',
> + '22x22',
> + '16x16'
> +}
> +
> +icon_sizes = {}
If they are private, please make them local. Also, where does this list of icon
sizes come from? Looks quite arbitrary.
> +--- Lookup an icon in different folders of the filesystem.
> +-- @param icon_file Short or full name of the icon.
> +-- @return full name of the icon.
> +function lookup_icon(icon_file)
> + if not icon_file or icon_file == "" then
> + return default_icon
> + end
> + if icon_file:sub(1, 1) == '/' and (icon_file:find('.+%.png') or
> icon_file:find('.+%.xpm')) then
> + -- icons with absolute path and supported (AFAICT) formats
> + return icon_file
> + else
> + local icon_path = {}
> + local icon_theme_paths = {}
> + if icon_theme then
> + table.insert(icon_theme_paths, '/usr/share/icons/' .. icon_theme
> .. '/')
> + -- TODO also look in parent icon themes, as in freedesktop.org
> specification
:-)
> + end
> + table.insert(icon_theme_paths, '/usr/share/icons/hicolor/') --
> fallback theme cf spec
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?
> + local isizes = {}
> + for i, sz in ipairs(all_icon_sizes) do
> + table.insert(isizes, sz)
> + end
Uhm. Doesn't do anything? If you need to clone a table, use
awful.util.table.clone().
> + for i, icon_theme_directory in ipairs(icon_theme_paths) do
> + for j, size in ipairs(icon_file_sizes or isizes) do
This is the only reference to "icon_file_sizes". Is that intended?
> + table.insert(icon_path, icon_theme_directory .. size ..
> '/apps/')
> + table.insert(icon_path, icon_theme_directory .. size ..
> '/actions/')
> + table.insert(icon_path, icon_theme_directory .. size ..
> '/devices/')
> + table.insert(icon_path, icon_theme_directory .. size ..
> '/places/')
> + table.insert(icon_path, icon_theme_directory .. size ..
> '/categories/')
> + table.insert(icon_path, icon_theme_directory .. size ..
> '/status/')
I assume this list is from the spec, too? menu_gen has this list, too, but
configurable via all_categories. Could that list be used here? Should I STFU and
read the spec instead? ;-)
> + end
> + end
> + -- lowest priority fallbacks
> + table.insert(icon_path, '/usr/share/pixmaps/')
> + table.insert(icon_path, '/usr/share/icons/')
> +
> + for i, directory in ipairs(icon_path) do
> + if (icon_file:find('.+%.png') or icon_file:find('.+%.xpm')) and
> awful_util.file_readable(directory .. icon_file) then
> + return directory .. icon_file
> + elseif awful_util.file_readable(directory .. icon_file ..
> '.xpm') then
> + return directory .. icon_file .. '.xpm'
> + elseif awful_util.file_readable(directory .. icon_file ..
> '.png') then
> + return directory .. icon_file .. '.png'
> + end
> + end
Is this in the spec, too, or does it just work around broken files? Why does it
only allow png and xpm? What if I want a jpeg? ;-)
> + return default_icon
> + end
> +end
> +
> +--- Parse a .desktop file
> +-- @param file The .desktop file
> +-- @param requested_icon_sizes A list of icon sizes (optional). If this list
> is given, it will be used as a priority list for icon sizes when looking up
> for icons. If you want large icons, for example, you can put '128x128' as the
> first item in the list.
Long line is long. Please add a line break at column 80.
> +-- @return A table with file entries.
> +function parse(file, requested_icon_sizes)
I'm starting to believe my search function (ctrl+f in thunderbird) is broken,
but the second argument seems to be unused.
> + local program = { show = true, file = file }
> + for line in io.lines(file) do
> + for key, value in line:gmatch("(%w+)=(.+)") do
> + program[key] = value
> + end
> + end
> +
> + -- Don't show program if NoDisplay attribute is false
> + if program.NoDisplay and string.lower(program.NoDisplay) == "true" then
> + program.show = false
> + end
> +
> + -- Only show the program if there is not OnlyShowIn attribute
^^^
"no" instead of "not"
> + -- 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. :-)
Also, could this "awesome" string be made configurable? Just add it as a
variable at the top.
> + -- Look up for a icon.
> + if program.Icon then
> + program.icon_path = lookup_icon(program.Icon)
> + end
> +
> + -- Split categories into a table.
> + if program.Categories then
> + program.categories = {}
> + for category in program.Categories:gfind('[^;]+') do
I think this splits up the string at ";". Could a comment say so?
> + table.insert(program.categories, category)
> + end
> + end
> +
> + if program.Exec then
> + local cmdline = program.Exec:gsub('%%c', program.Name)
> + cmdline = cmdline:gsub('%%[fuFU]', '')
> + cmdline = cmdline:gsub('%%k', program.file)
> + if program.icon_path then
> + cmdline = cmdline:gsub('%%i', '--icon ' .. program.icon_path)
> + else
> + cmdline = cmdline:gsub('%%i', '')
> + end
> + if program.Terminal == "true" then
> + cmdline = terminal .. ' -e ' .. cmdline
> + end
> + program.cmdline = cmdline
> + end
Again: Magic. I think this replaces e.g. "%c" with the program's name, removes
some options which weren't implemented and does some icon-magic.
> +
> + return program
> +end
> +
> +--- Parse a directory with .desktop files
> +-- @param dir The directory.
> +-- @param icons_size, The icons sizes, optional.
> +-- @return A table with all .desktop entries.
> +function parse_dir(dir)
> + local programs = {}
> + local files = io.popen('find '.. dir ..' -maxdepth 1 -name
> "*.desktop"'):lines()
> + for file in files do
> + table.insert(programs, parse(file))
> + end
> + return programs
> +end
Sorry for being so pedantic. If it bothers you, talk to jd. He is less
pedantic. :-)
Uli
--
"In the beginning the Universe was created. This has made a lot of
people very angry and has been widely regarded as a bad move."
--
To unsubscribe, send mail to [email protected].
