Excerpts from Uli Schlachter's message of Thu Jul 30 07:31:38 -0400 2009:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Andrei Thorp wrote:
> > I wanna get my patches reviewed dammit ;)
>
> Ok, here we go:
>
> > Subject: [PATCH 1/4] Lib: hooks lib w/ awful timer wrapper functions
> [...]
> > diff --git a/lib/hooks/init.lua b/lib/hooks/init.lua
> > new file mode 100644
> > index 0000000..ec94102
> > --- /dev/null
> > +++ b/lib/hooks/init.lua
> [...]
> > +-- Register a timer just as you would with awful.hooks.timer.register, but
> > +-- with one slight twist: you set your regular speed, (optionally) your
> > slow
> > +-- speed, and then your function.
> > +-- @param reg_time Regular speed for the widget
> > +-- @param slow_time Slow time for the widget
> > +-- @param fn The function that the timer should call
> > +-- @param descr The description of this function
> > +function timer.register(reg_time, slow_time, fn, descr)
> > + if not slow_time then slow_time = reg_time * 4 end
> > + registry[fn] = {
> > + regular=reg_time,
> > + slow=slow_time,
> > + speed="regular",
> > + running=true,
> > + description=descr or "Undescribed timer"
> > + }
> > + timer.set_speed(registry[fn].speed, fn)
> > +end
>
> How is this "(optionally) your slow speed" supposed to work?
> I would propose something like this:
>
> function timer.register(fn, desc, reg_time, slow_time)
>
> That way timer.register(bla, "bla", 42) results in "slow_time" being nil,
> waaay
> more optionally. (BTW, is slow_time being nil handled anywhere?)
Nah, I think I'll keep descr at the end to make that "more optional",
then edit the comment. I don't really want to change the ordering of the
arguments, because that ordering is used pretty consistently throughout
the program. If you want to specify slow_time as nil, you still can with
timer.register(5, nil, foo). I think that's good enough.
> > +-- Unregister the timer altogether
> > +-- Maybe you should just turn it to zero with timer.stop()?
> > +-- @param fn The function you want to unregister.
> > +function timer.unregister(fn)
> > + registry[fn] = nil
> > + awful.hooks.timer.unregister(fn)
> > +end
>
> Maybe you should not tell me what I want. :P
>
> How about this: "Please note that you can use timer.stop() and timer.start()
> to
> pause and unpause a timer"?
Not sure why you're even bothering reviewing this, but changed.
> > +-- Generic "let timer run at foo speed" function
> "Generic function to modify an existing timer's speed"
> "Switch timers between "slow" and "regular" mode"
> Something like this...
Sure, done.
> > +-- @param speed The speed at which you want the function(s) to run: one of
> > +-- "regular" or "slow"
> > +-- @param fn (Optional) Function that you want to set to foo speed. If you
> > +-- do not supply this argument, the system sets all timers to foo speed.
> That "foo" really sounds weird, sorry.
Okay.
> > +function timer.set_speed(speed, fn)
> > + if fn then
> > + registry[fn].speed = speed
> > + if not registry[fn].running then
> > + return
> > + end
> > + awful.hooks.timer.unregister(fn)
> > + if speed == "regular" then
> > + awful.hooks.timer.register(registry[fn].regular, fn)
> > + elseif speed == "slow" then
> > + awful.hooks.timer.register(registry[fn].slow, fn)
> > + end
> > + else
> > + for key, value in pairs(registry) do
> > + registry[key].speed = speed
> > + if registry[key].running then
> > + awful.hooks.timer.unregister(key)
> > + if speed == "regular" then
> > + awful.hooks.timer.register(registry[key].regular, key)
> > + elseif speed == "slow" then
> > + awful.hooks.timer.register(registry[key].slow, key)
> > + end
> > + end
> > + end
> > + end
> > +end
>
> How about this for the else branch:
> for key, value in pairs(registry) do
> timer.set_speed(speed, key)
> done
Clever, I like it.
> > +-- Edit a function's speeds.
> Modify? Dunno, edit sounds weird (to me).
Ignored.
> > +-- @param reg_time Regular speed for the function
> > +-- @param slow_time Slow speed for the function
> s/_time/_speed/? (just a proposal)
Ignored.
> > +-- @param fn Function that you want to alter the speed of
> Why isn't this optional her but everywhere else?
Because I don't want them to be able to set the speeds of all registered
timers at the same time. That doesn't make sense and could screw up a
lot of your running Obvious widgets. If you really wanted this, you
could get the list of all of them and iterate.
> > +function timer.set_speeds(reg_time, slow_time, fn)
> > + registry[fn] = {
> > + regular=reg_time,
> > + slow=slow_time,
> > + speed=registry[fn].speed,
> > + running=registry[fn].running,
> > + }
> > + timer.set_speed(registry[fn].speed, fn)
> > +end
Yeah, unfortunately similar. I don't really see too much I can do about
this though, they're sufficiently different and it doesn't seem obvious
to me how to reduce code and add a helper function.
> > +-- @param fn (Optional) Function that you want to know the data for. If not
> > +-- specified, this returns a table of all registered timers with their
> > data.
> > +-- @return A table with the regular speed, the slow speed, the currently
> > used
> > +-- speed, whether the timer is running or not, and the description.
> > +-- Note: This function returns a copy of the internal registry, so
> > assigning to
> > +-- it doesn't work.
> This comment needs some love. I really miss the general description at the
> beginning. Maybe it's just me, but I'd prefer something like
>
> This function returns the speeds of the timer
> @param The timer function you are interested in. Nil for all timers
> @return A table with one entry per timer (if multiple timers are returned,
> else directly the next thing (TODO: improve documenataion). Each item has the
> following table keys: <insert keys>
Description added. I like my more verbose variable descriptions though.
> > +function timer.get_speeds(fn)
> > + copy = {}
> > + if fn then
> > + for key, value in pairs(registry[fn]) do
> > + copy[key] = value
> > + end
> > + else
> > + for key, value in pairs(registry) do
> > + subcopy = {}
> > + for subkey, subvalue in pairs(registry[key]) do
> in pairs(value)
> > + subcopy[subkey] = subvalue
> > + end
> > + copy[key] = subcopy
> > + end
> > + end
> > + return copy
> > +end
> > +
> > +-- Pause timer(s)
> > +-- @param fn (Optional) Function to pause the timer for. If none is
> > specified,
> > +-- this pauses all registered timers.
> > +function timer.stop(fn)
> > + if fn then
> > + registry[fn].running = false
> > + awful.hooks.timer.unregister(fn)
> > + else
> > + for key, value in pairs(registry) do
> > + registry[key].running = false
> > + awful.hooks.timer.unregister(key)
> > + end
> > + end
> > +end
> > +
> > +-- Start timer(s)
> > +-- @param fn (Optional) Function to start the timer for. If none is
> > specified,
> > +-- this starts all registered timers.
> > +function timer.start(fn)
> > + if fn then
> > + registry[fn].running = true
> > + timer.set_speed(registry[fn].speed, fn)
> > + else
> > + for key, value in pairs(registry) do
> > + registry[key].running = true
> > + timer.set_speed(registry[key].speed, fn)
> > + end
> > + end
> > +end
> I don't like this idea myself, but what do you think about calling
> timer.start()
> for each entry in the else branch?
Nah...
> > +-- Checks whether the given function is registered
> > +-- @return boolean true/false of whether the function is registered
> > +function timer.has(fn)
> > + if registry[fn] then
> > + return true
> > + else
> > + return false
> > + end
> > +end
> > +
> > +-- vim:
> > filetype=lua:expandtab:shiftwidth=4:tabstop=4:softtabstop=4:encoding=utf-8:textwidth=80
> > diff --git a/lib/init.lua b/lib/init.lua
> > new file mode 100644
> > index 0000000..a290db1
> > --- /dev/null
> > +++ b/lib/init.lua
> > @@ -0,0 +1,9 @@
> > +------------------------------------------
> > +-- Author: Andrei "Garoth" Thorp --
> > +-- Copyright 2009 Andrei "Garoth" Thorp --
> > +------------------------------------------
> > +
> > +require("obvious.lib.hooks")
> > +
> > +module("obvious.lib")
> > +-- vim:
> > filetype=lua:expandtab:shiftwidth=4:tabstop=4:softtabstop=4:encoding=utf-8:textwidth=80
>
> Is this really necessary? :(
> When will the next lib item be added? If that's not too soon, we could skip
> this
> file for now... Dunno, I guess we can't
Yep. The mpd patch is already in here too.
> Uli
> who know has to get to work before his colleagues notice he hasn't been
> working
> - --
> "Do you know that books smell like nutmeg or some spice from a foreign land?"
> -- Faber in Fahrenheit 451
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iQEcBAEBCAAGBQJKcYSYAAoJECLkKOvLj8sGb+MH/3/s1RhiCrB9U5Za8zWg3QoW
> Zgqr8odjWKeQsJz2rSfFvYMVJmmX3OoiNCy2eMKGox0KOVsZaZc1YmklapP6j5If
> PJjp1Mr+Wx5/JI15xFLJpgbgnuzKSy9p9S0avYslVmYHUGwp4BQ7aDUP68bqxlJP
> mso4eynzzArPfBCQZ82pchmEaciYa6npv7KXI2F6Fw1uA+zmkC30K44wG6LXYGPP
> G9WMIoHHUlncOTKeNGERMk6p6P/IZZluVXqeUgLHAVxaDtbL5cTEkoE7XsCnfKT/
> eIoCa3Z+ne5UjbDvFPKX1eTjrikrJrWjqzZWtSafCUhDUrE4ib4xe5cVmmfNRio=
> =f58k
> -----END PGP SIGNATURE-----
--
Andrei Thorp, Developer: Xandros Corp. (http://www.xandros.com)
--
To unsubscribe, send mail to [email protected].
