On Tue, Jul 07, 2009 at 11:38:25AM +0200, Julien Danjou wrote: > [...] > > - y = area.y + (ctx->height - ext->height + 1) / 2 + margin->top; > > + y = area.y + margin->top; > > > > /* only honors alignment if enough space */ > > if(ext->width < area.width) > > I don't think this belongs to this patch. Would you mind splitting and > explaining it a little? > [...]
It does, because it removes the calculation of the actual text position
from C. Without that, widget_geometries isn't fully functional, as text
doesn't get drawn where the layout function decides it should.
> > [...]
> > +/** Retrieve a list of widget geometries using a Lua layout function.
> > + * \param wibox The wibox.
> > + * \return True is everything is ok, false otherwise.
> > + * \todo What do we do if there's no layout defined?
> > + */
>
> Comments does not indicate it push a table on the stack. Would you mind
> adding more comments?
> [...]
Added :)
> [...]
> > +bool
> > +widget_geometries(wibox_t *wibox)
> > +{
> > + /* get the layout field of the widget table */
> > + if (wibox->widgets_table != LUA_REFNIL)
> > + {
> > + lua_rawgeti(globalconf.L, LUA_REGISTRYINDEX, wibox->widgets_table);
> > + lua_getfield(globalconf.L, -1, "layout");
> > + }else
> > + lua_pushnil(globalconf.L);
>
> Put 'else' statement on its own line.
> Also pushing nil might be useless, just going to the else after might
> optimize a very very very little.
> [...]
Premature optimization is the root of all evil :)
I'll do that once everything else has been ironed out.
> > [...]
> > + /* if the layout field is a function */
> > + if(lua_isfunction(globalconf.L, -1))
> > + {
> > + /* Push 1st argument: wibox geometry */
> > + area_t geometry = wibox->sw.geometry;
> > + geometry.x = 0;
> > + geometry.y = 0;
> > + /* we need to exchange the width and height of the wibox window if
> > it
> > + * isn't at the top or bottom, so the layout function doesn't need
> > to
> > + * care about that */
>
> Wrong comment.
> [...]
Agreed, I changed that.
> > + if(wibox->sw.orientation != East)
> > + {
> > + int i = geometry.height;
> > + geometry.height = geometry.width;
> > + geometry.width = i;
> > + }
> > + geometry.height -= 2 * wibox->sw.border.width;
> > + geometry.width -= 2 * wibox->sw.border.width;
> > + luaA_pusharea(globalconf.L, geometry);
>
> I think this is bad. I'll comment on that later.
>
> > + /* Re-push 2nd argument: widget table */
> > + lua_pushvalue(globalconf.L, -3);
> > + /* Push 3rd argument: wibox screen */
> > + lua_pushnumber(globalconf.L,
> > screen_array_indexof(&globalconf.screens, wibox->screen));
> > + /* call the layout function with 3 arguments (wibox geometry,
> > widget
> > + * table, screen) and wait for one result */
> > + if(lua_pcall(globalconf.L, 3, 1, 0))
> > + {
> > + warn("error running layout function: %s",
> > + lua_tostring(globalconf.L, -1));
> > + lua_pop(globalconf.L, 2);
> > + return false;
> > + }
>
> Using luaA_dofunction() might be better.
> [...]
> > + lua_insert(globalconf.L, lua_gettop(globalconf.L) - 1);
> > + lua_pop(globalconf.L, 1);
>
> I think we said that's totally noop, no?
> [...]
It is not. lua_insert() needs an absolute stack position, which is why
the lua_gettop() is used there (-1 would be a relative position). These
two lines exchange the two top stack elements and then remove the new
top, effectively removing the second element from the top (which is the
layout function, it's not needed anymore and it would taint the stack if
it stayed on top of it).
> [...]
> > + warn("no layout function defined or layout function invalid,
> > falling back to simple default layout");
>
> No warn please.
> [...]
Agreed, I'll remove that.
> [...]
> > Subject: [PATCH 05/15] systray: don't crap up on odd-sized windows
>
> OK, but adding more explanation in the commit log would be nice, I did
> not get everything.
> [...]
I'll do. Basically, we get the size we want the systray to be drawn at
upon drawing the tray, thus removing the query to the xcb_get_geometry
function. As all windows embedded in the systray are squares, their size
is equal to their height, so we simply force the size to be wibox_height
* n x wibox_height, where n is the number of embedded windows, instead
of deriving it from the largest embedded window (which, for example in
the case of wicd, would be 200x200, way too much for one window on a
regular wibox).
> [...]
> > Subject: [PATCH 10/15] awful.widget: add layouts
> > +local function horizontal(direction, bounds, widgets, screen)
>
> That's here I comment about bounds.
>
> I don't like this part of the implementation. IMHO, the layout should
> have no idea what are the bounds it is drawn into.
> It should assume that x,y = 0,0 and then compute everthings based on
> that.
> Then, if actually x,y are, let's say, 100,100, that's the work of the
> callee to translate the returned geometries inside its own coordinates
> set.
> Am I clear?
> [...]
Totally. This would ease up the implementation quite a bit, I'll do it.
> [...]
> > + g = {
> > + ["width"] = 0,
> > + ["height"] = 0,
> > + }
>
> I hate your useless [""] style. :)
> [...]
An oversight, will be removed :)
> > + if v.resize and g.width > 0 then
> > + g.width = math.floor(g.height * g.ratio)
> > + end
>
> Brr, well, I don't force the removal of that v.resize but that's ugly.
> I'm sure you've put it because it breaks things if it's not here, but
> this should be removed ASAP, but probably later.
> [...]
It does indeed brake resizable imageboxes, and frankly, I have no idea
how we could replace that. I think the best idea would be to have things
settle down and then take a closer look at it.
> [...]
> > Subject: [PATCH 12/15] naughty: add support for widget layouts
>
> OK, maybe koniu would mind reviewing it?
> [...]
Would be very nice :)
> [...]
> > Subject: [PATCH 13/15] awful.menu: add support for widget layouts
>
> OK.
> (I hope someone will remove all that wiboxes and use really layout
> later. ;-)
> [...]
Will be done once I have all the other bugs ironed out :)
> [...]
This is just a feedback, to let you know I'm working on this stuff and
to clear up a few things. If anyone has further comments, please share
:)
--
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
pgpa6R4f43is1.pgp
Description: PGP signature
