On 04.03.2012 16:30, Alexander Yakushev wrote:
> Okay, the second try.
>
> I skimmed through Freedesktop's standards about .desktop files and icons
> themes/locations and added the respective notices in the utils.lua.
> About JPG icons: jpg format is not supported by the specification. Only
> png, xpf and svg; but svg is not supported by Awesome so this leaves us
> only with two of them.
>
> I left utils.lua where it was. If someone decides to write something
> that uses it then with the context of his work we could put make it more
> general. Now I can't think of a place to move it to.
Thanks.
Patch 1: Merged
Patch 2:
> +function run(args, textbox, exe_callback, completion_callback, history_path,
> history_max, done_callback, changed_callback, keypressed_callback)
Some day this should be turned into a table instead of lots and lots of
arguments. However, that day is not yet (= doesn't have to be in this patch).
Merged (after fixing up the now-superflous "return true"s).
Patch 3:
awesomerc.lua.in:
> + -- Menubar
> + awful.key({ modkey }, "p", function() menubar.show() end)
How do you go from "p" to "menubar"? I admit that some of the other keybindings
are unintuitive, too, and that all the good letters are already taken, so any
kind of reason is fine with me. :-)
lib/menubar/init.lua.in:
> +-- Specifies the size and position of the menubar.
> +geometry = { width = nil,
> + height = 20,
> + x = nil,
> + y = nil }
What do the "nil"s do? I'd suggest something like this for the doc:
-- Specifies the geometry of the menubar. This is a table with the keys
-- x, y, width and height. Missing values are replaced via the screen's
-- geometry. However, missing height is replaced by the font size.
> +common_args = { w = wibox.layout.fixed.horizontal(),
> + data = setmetatable({}, { __mode = 'kv' }) }
Shouldn't this be local, too?
[skip lots of stuff]
> +function show(scr)
[...]
> + instance.wibox.height = geometry.height or 20
Could you get rid of this hardcoded 20? awful.wibox uses the following for the
default wibox height (arg.font is allowed to be nil):
arg.width = arg.width or beautiful.get_font_height(arg.font) * 1.5
Oh and for the same code:
> + instance.wibox.height = geometry.height or 20
> + instance.wibox.width = geometry.width or scrgeom.width
> + instance.wibox:geometry({x = x, y = y})
Why do you set half the values via the one way and the other half via the other
half? Could you put all of them into the :geometry() call?
> +--- Set the current system icon theme.
> +-- @param theme_name The icon theme's name.
> +function set_icon_theme(theme_name)
> + utils.icon_theme = theme_name
> + menu_gen.lookup_category_icons()
> +end
Could you put this into the theme file? E.g. the default (what's the default
btw?) gets hardcoded, but if theme.icon_theme is set, the value from beautiful
takes precedence over this one.
> +setmetatable(_M, { __call = function(_, ...) return get(...) end })
> \ No newline at end of file
No newline? Pfft. :-)
Also, please add the "usual" vim modeline (just because everything else has it,
too):
-- vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80
menu_gen.lua.in:
> +-- Specify the mapping of .desktop Categories section to the
> +-- categories in the menubar. If Use flag is set to false then any of
> +-- the applications that fall only to this category will not be shown.
Uhm, if there is a flag "don't use this", why can't one instead just not have
the entry? What's the "Use"-flag for? (also, documentation says "Use", but code
says "use")
> +-- Remove non-printable characters from the end of the string.
> +-- @param s string to trim
> +local function trim(s)
> + if s then
> + -- Check CR/LF newlines
> + if string.byte(s, #s) == 13 then
> + return string.sub(s, 1, #s - 1)
> + end
> + return s
> + end
> +end
The documentation doesn't match the quellcode. Also:
local function trim(s)
if not s then return end
-- Check CR/LF newlines
if string.byte(s, #s) == 13 then
[...]
> \ No newline at end of file
Same comment as for the previous file.
utils.lua.in:
> +-- NOTE: This icons/desktop files module was written according to the
> +-- following freedesktop.org specifications:
> +-- Icons:
> http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-0.11.html
> +-- Desktop files:
> http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-1.0.html
Yay. I like that. It's always a pain to figure out where the spec is when I have
to touch naughty.
> +-- Terminal which applications that need terminal would open in.
> +terminal = 'xterm'
Hm, awesomerc.lua.in already has such an option. Can we somehow merge the two
options into one? Although I don't really have any good idea on where that
should be put...
Perhaps just adding a line to the default rc.lua which sets this option based on
what is in the config is enough?
> +-- List of supported icon formats. Ignore SVG because Awesome doesn't
> +-- support it.
Remind me to turn that into an open feature request once this gets committed.
SVGs should be doable. :-)
> +local icon_formats = { ".png", ".xpm" }
> +
> +-- Check whether the icon format is supported.
> +-- @param icon_file Filename of the icon.
> +-- @return true if format is supported, false otherwise.
> +local function is_format_supported(icon_file)
> + for _, f in ipairs(icon_formats) do
> + if icon_file:match('.+' .. f) then
> + return true
> + end
> + end
> + return false
> +end
The "." in ".png" gets interpreted as a regex wildcard, right? That seems
unintuitive. Could we perhaps change the match() call into (and then have just
"png" instead of ".png" in icon_formats):
icon_file:match('\.' .. f)
(The ".+" shouldn't be needed, IMHO)
Patch looks good, I were almost tempted to try this out. But evil lua 5.2 broke
awesome in gentoo and I have to read up on lua 5.2 now. :-)
Cheers,
Uli
--
- He made himself, me nothing, you nothing out of the dust
- Er machte sich mir nichts, dir nichts aus dem Staub
--
To unsubscribe, send mail to [email protected].
