-----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?)
> +-- 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"?
> +-- 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...
> +-- @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.
> +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
Way less code duplication...
> +-- Edit a function's speeds.
Modify? Dunno, edit sounds weird (to me).
> +-- @param reg_time Regular speed for the function
> +-- @param slow_time Slow speed for the function
s/_time/_speed/? (just a proposal)
> +-- @param fn Function that you want to alter the speed of
Why isn't this optional her but everywhere else?
> +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
The code looks almost identical to timer.register().. :/
> +-- @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>
> +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?
> +-- 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
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-----
--
To unsubscribe, send mail to awesome-devel-unsubscr...@naquadah.org.
