On Thu, Jun 25, 2009 at 03:26:48PM +0200, Uli Schlachter wrote:
> [...]
> > Subject: [PATCH 01/12] wibox: rebuild table at every draw
> > diff --git a/wibox.c b/wibox.c
> > index eb092b2..30c85b2 100644
> > --- a/wibox.c
> > +++ b/wibox.c
> > @@ -541,9 +528,12 @@ luaA_wibox_invalidate_byitem(lua_State *L, const void
> > *item)
> > wibox_t *wibox = *w;
> > if(luaA_wibox_hasitem(L, wibox, item))
>
> First..
>
> > {
> > - /* recompute widget node list */
> > - wibox_widgets_table_build(L, wibox);
> > - lua_pop(L, 1); /* remove widgets table */
> > + if(luaA_wibox_hasitem(L, wibox, item))
>
> ...and second call, one time should be enough
>
> > + {
> > + /* update wibox */
> > + wibox_need_update(wibox);
> > + lua_pop(L, 1); /* remove widgets table */
> > + }
> > }
> >
> > }
> [...]
Agreed, I think that slipped in in one of the many rebases I did while
writing the patches. I'll amend that.
> [...]
> This will kill the processes if e.g. the "x" field is a table instead of a
> number. luaA_getopt_number() should only ever be called from within
> lua_pcall()
> ("p" as in "protected" for this exact reason ;) )
>
> [...]
True, that's what patch #09 is for, I guess I'll rebase in a way so that
patch #09 comes before this one.
> [...]
> Let's break the deprecated widgets *duck*.
> [...]
Imho they should be patched as long as they are still in git master :)
> [...]
> WTF?
> [...]
> again: WTF? definitely not something which belongs into this patch.
> [...]
True, slipped by my attention, thanks for catching that :)
> [...]
> Oh, here it is (see above). I guess you could drop this line from the last
> patch
> then...
> [...]
Agreed, thy will be done :D
> [...]
> BTW: Why are you using '["layout"] =' ? Doesn't 'layout =' have the same
> effect
> and look better? Dunno, I guess this is a matter of taste... (Again: Feel free
> to ignore me)
> [...]
It's an old habit of mine :P No really, I used to use that style back
when I started writing the patches, but of course it adds clutter. I'll
remove that.
> > Subject: [PATCH 07/12] invaders: add support for widget layouts
> > diff --git a/lib/invaders.lua.in b/lib/invaders.lua.in
> > + gamedata.highscore.window:geometry(gamedata.highscore.table:extents())
> > + gamedata.highscore.window:geometry({ x = gamedata.field.x +
> > math.floor(gamedata.field.w / 2) - 100,
> > + y = gamedata.field.y +
> > math.floor(gamedata.field.h / 2) - 55 })
>
> Uhm, drop that first :geometry call?
That's not possible. The first call to :geometry sets the right width
and height for the wibox and the second call sets the X and Y
coordinates. If you can think of an easier way, please let me know :)
> [...]
> > Subject: [PATCH 09/12] luaA_getopt_number(): also return def if stack top
> > is neither number nor nil
> > diff --git a/luaa.h b/luaa.h
> > index ac99b0e..8c798ce 100644
> > --- a/luaa.h
> > +++ b/luaa.h
> > @@ -135,9 +135,10 @@ static inline lua_Number
> > luaA_getopt_number(lua_State *L, int idx, const char *name, lua_Number def)
> > {
> > lua_getfield(L, idx, name);
> > - lua_Number n = luaL_optnumber(L, -1, def);
> > + if (lua_isnil(L, -1) || lua_isnumber(L, -1))
> > + def = luaL_optnumber(L, -1, def);
> > lua_pop(L, 1);
> > - return n;
> > + return def;
> > }
>
> Uhm, I think this fixes the problem I had with patch 1 or 2 (the one which
> could
> cause a lua error outside of lua_pcall()). Ask jd twice if this is ok. ;)
>
See above
> > Subject: [PATCH 10/12] systray: don't crap up on odd-sized windows
> > diff --git a/widgets/systray.c b/widgets/systray.c
> > index 0504be8..bd53ef6 100644
> > --- a/widgets/systray.c
> > +++ b/widgets/systray.c
> > @@ -32,32 +32,27 @@
> > #define _NET_SYSTEM_TRAY_ORIENTATION_HORZ 0
> > #define _NET_SYSTEM_TRAY_ORIENTATION_VERT 1
> >
> > +typedef struct
> > +{
> > + /* systray height */
> > + int height;
> > +} systray_data_t;
> > +
>
> Uhm, do we really need a struct for this?
>
I wanted to stay consistent with the rest of the widgets, but as the
number of widgets in the C core is reduced, that might not be neccessary
anymore. JD: comments?
> > Subject: [PATCH 11/12] awful.util: add table.clone
>
> That's a nice, small patch, it's a candidate for merging independent of widget
> layouts. :)
>
If you really need it :P
> > Subject: [PATCH 12/12] widget.layout.*: correctly stack nested layouts
>
> A bugfix in the very first patch series? I'd propose to merge this stuff with
> the patches which added this code
>
Agreed, these patches are my current working branch, so expect some
splitting and reordering.
> I hope I didn't annoy you too much.
>
Nope, your feedback is very much appreciated, as well as everyone elses, thanks
a lot :)
--
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
pgpQfks1btaWG.pgp
Description: PGP signature
