On Thu, Jul 09, 2009 at 10:53:52AM +0200, Uli Schlachter wrote: > [...] > Uhm... "I'll do" in the beginning of a commit message? Copy&paste is nice, but > don't forget grammar and context. :P > [...]
In my defence: It was late at night :)
> > [...]
> > + -- we are only interested in tables and widgets
> > + local keys = util.table.keys_filter(widgets, "table", "widget")
>
> Uhm... Why? What are you trying to get rid of? Since util.table.keys_filter()
> doesn't filter the table recursivly, other stuff can still be hidden in there.
> Oh and the for loop below checks via type() anyway and ignores all other
> stuff..
> [...]
Child tables will be filtered in their own layout functions. We
basically filter here because widget tables can contain other stuff, for
example boolean values (i.e. if you conditionally include a widget in
one wibox, but not in another) or functions (as was the case with the
titlebars). If we took those into account, the height calculation would
be messed up, as it needs the number of elements that will really be
placed onto the wibox.
> [...]
> Oh and: Does the C code catch loops in the table? If not I just found an
> endless
> loop below...
>
> > + for _, k in ipairs(keys) do
> > + local v = widgets[k]
> > + if type(v) == "table" then
> > + local layout = v["layout"] or function (...) return
> > horizontal(direction, ...) end
> > + local g = layout(bounds, v, screen)
> > + for _, v in ipairs(g) do
> > + v.x = v.x + x
> > + table.insert(geometries, v)
> > + end
> > + bounds = g.free
> > + x = x + g.free.x
> > + elseif type(v) == "widget" then
> > + local g
> > + if v.visible then
> > + g = v:extents(screen)
> > + else
> > + g = {
> > + width = 0,
> > + height = 0,
> > + }
> > + end
>
> Can't the C code filter invisible widgets or something like that...? (I guess
> not)
> Why can widgets be made invisible at all? :(
> [...]
Where exactly is that endless loop? I can't seem to find it. And
frankly said, if you use looping tables for your widgets, you shouldn't
be surprised :P
Making widgets invisible is quite nice e.g. for laptops. You'd need some
widgets, for example UMTS signal strength, only while the laptop is
mobile, so you can simply hide that widget while it is on AC instead of
doing tricks with two widget tables or something the like.
> [...]
> > +function flex(bounds, widgets, screen)
> [..]
> > + nelements = (nelements == 0) and 1 or nelements
>
> I read this twice and then thought:
> if nelements == 0 then nelements = 1 end
> [...]
It keeps consistence with the functions in e.g. awful.util. Those also
use
foo = foo or 1
instead of
if not foo then foo = 1 end
IMHO it's a matter of taste
> [...]
> > --- /dev/null
> > +++ b/lib/awful/widget/layout/vertical.lua.in
> > @@ -0,0 +1,108 @@
> [snip]
> > +local function vertical(direction, bounds, widgets, screen)
> [snip]
> > + -- we are only interested in tables and widgets
> > + local keys = util.table.keys_filter(widgets, "table", "widget")
> > + local nelements = #keys
> > + local height = math.floor(bounds.height / nelements)
>
> What if nelements is 0?
> [...]
Good catch, I'll fix that.
> [...]
> > Subject: [PATCH 5/9] awesomerc.lua: add support for widget layouts
> > Subject: [PATCH 6/9] naughty: add support for widget layouts
> > Subject: [PATCH 7/9] awful.menu: add support for widget layouts
> > Subject: [PATCH 8/9] invaders: add support for widget layouts
> > Subject: [PATCH 9/9] titlebar: add widget layout support
>
> More than half your patches fix things which you broke, well done! ;)
> [...]
Pssh :P
Thanks a lot for your time :)
--
GCS/IT/M d- s+:- a--- C++ UL+++ US UB++ P+++ L+++ E--- W+ N+ o--
K- w--- O M-- V PS+ PE- Y+ PGP+++ t+ 5 X+ R tv+ b++ DI+++ D+++ G+
e- h! r y+
Gregor Best
pgpaTPdFVzcZv.pgp
Description: PGP signature
