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 awesome-devel-unsubscr...@naquadah.org.