Excerpts from Uli Schlachter's message of Thu Jul 30 10:21:21 -0400 2009:
> > +local defaults = {
> > + format = "$title - $album - $artist",
> > + length = 75,
> > + unknown = "(unknown)",
> > +}
> > +local settings = {}
> > +for key, value in pairs(defaults) do
> > + settings[key] = value
> > +end
>
> Uhm... why don't you just put the defaults in settings and let the user
> override
> them?
So that in case they fuck up a setting by making it nil or something, I
have a fallback in the defaults.
> > +local widget = widget({ type = "textbox",
> > + name = "mpd-playing",
> > + align = "left" })
>
> "align"? Wasn't that the funny thing which was teared, feathered, shot, buried
> and exploded by farhaven? (and then he started to *really* hurt it)
>
> "name"? If "align" is a zombie, "name" never was alive in the first place,
> right?
Yeah, I can probably safely drop these attributes. I haven't updated to
widget-layouts yet.
> > +-- Utility function to handle the text for MPD
> > +-- @param songinfo: a table with fields "artist", "album", "title" in text
> > +-- @return formatted (settings.format) string to display on the widget.
> > This
> > +-- respects settings.length and tries to make fields as close to the same
> > +-- lenghths as possible if shortening is required.
> ^^^^^^^^
> lengths
Thanks.
> > +local function format_metadata(songinfo)
> > + format = settings.format or defaults.format
> > +
> > + if (settings.length or defaults.length) <= 0 then
> > + return ""
> > + end
> > +
> > + used_keys = {}
> > + start = 1
> > + stop = 1
> > + while start do
> > + start, stop = string.find(format, "%$%w+", stop)
> > + key = string.match(format, "%$(%w+)", stop)
> > + if key then
> > + if songinfo[key] then
> > + used_keys[key] = songinfo[key]
> > + else
> > + used_keys[key] = settings.unknown or
> > + defaults.unknown
> > + end
> > + end
> > + end
>
> Ah, the joy of doing string-processing in Lua...
>
> I don't remember the details, but there was some lua function which let you do
>
> for match in <here's the part that I forgot> do
>
> I *think* it might have been :gmatch() (for match in
> format:gmatch("<dunno>")?)
I'll try to find this, would simplify this code a lot.
> > + retval = ""
> > + while true do
> > + retval = string.gsub(format, "%$(%w+)", used_keys)
> > + if #retval > (settings.length or defaults.length) then
> > + longest_key = nil
> > + longest_value = ""
> > + for key, value in pairs(used_keys) do
> > + if #value > #longest_value then
> > + longest_key = key
> > + longest_value = value
> > + end
> > + end
> > + if longest_key then
> > + -- shorten the longest by 1
> > + used_keys[longest_key] = string.sub(
> > used_keys[longest_key],
> > + 1,
> > + #longest_value - 1)
> > + else
> > + -- Seems like the format itself's too long
> > + err = "obvious.basic_mpd: impossible to
> > fit " ..
> > + "output into " .. (settings.length
> > or
> > + defaults.length) .. "
> > characters.\n" ..
> > + "Widget paused."
> > + naughty.notify({ text = err, timeout = 0 })
> > + lib.hooks.timer.stop(update)
> > + return ""
> > + end
> > + else
> > + -- All good!
> > + break
> > + end
> > + end
> > + return awful.util.escape(retval)
> > +end
>
> I haven't even tried to understand it, looks awful. :(
Nah, it's pretty simple, though it's algorithmically the most complex
code here. It checks to see whether the total string we're going to send
back is too long based on the length setting. If it is, we find the
longest component in it (remember, the mpd output has components like
$artist), and shorten it by one. Then we loop until it's the correct
length, or send up an error if that's impossible.
> > +-- Updates the widget's display
> > +function update()
> > + local status = lib.mpd.send("status")
> > + local now_playing, songstats
> > +
> > + if not status.state then
> > + now_playing = "Music Off"
> > + now_playing = lib.util.colour("yellow", now_playing)
>
> You misspelled "color" *duck*
You're mistaken. The original British + Canadian spelling is colour, and
the Americans have it subjectively wrong.
> > + elseif status.state == "stop" then
> > + now_playing = "Music Stopped"
> > + else
> > + songstats = lib.mpd.send("playlistid " .. status.songid)
>
> Heh, my "mpd lib" can do that as-well... The do-it-yourself lib.
Alright. Though if using BSD is okay, I don't see a huge amount of
inscentive to switch to your slighly less powerful lib at the moment.
Maybe one day.
> > + format = settings.format or defaults.format
> > + if type(format) == "string" then
> > + now_playing = format_metadata(songstats)
> > + elseif type(format) == "function" then
> > + now_playing = format(songstats)
> > + else
> > + naughty.notify({ text = "obvious.basic_mpd:
> > Invalid " ..
> > + "display format. Widget "
> > ..
> > + "paused." })
> > + lib.hooks.timer.stop(update)
> > + end
> > + end
> > +
> > + widget.text = now_playing
> > +end
> > +update()
> > +lib.hooks.timer.register(1, 30, update, "basic_mpd widget refresh rate")
> > +
> > +-- SETTINGS
> [snip, lots of functions here]
>
> Uhm, why? Just let the user modify settings directly. If he wants to shoot it
> into his foot, let him.
We set up a standardized interface to settings using the set_ functions.
It's just agreed upon. This way, widgets can handle settings internally
however they want and provide a consistent interface to the user across
all obvious code.
Also, it's not the case that just setting the setting is always enough
to apply it. In some of my code, I've had to run some update functions
as well that may be local and the user shouldn't be worrying about that.
If you really want to provide the settings directly with our system, you
can probably do some lua meta-magic to generate the functions
dynamically.
> > diff --git a/basic_mpd/readme b/basic_mpd/readme
> > new file mode 100644
> > index 0000000..20d2d29
> > --- /dev/null
> > +++ b/basic_mpd/readme
>
> skipped this, I'm better at critizing code than free text. (Ask farhaven, he
> gotta do sth too :P)
Yeah, that's fine.
> > diff --git a/lib/mpd/init.lua b/lib/mpd/init.lua
> > new file mode 100644
> > index 0000000..d1f0e35
> > --- /dev/null
> > +++ b/lib/mpd/init.lua
> > @@ -0,0 +1,160 @@
> > +-- Small interface to MusicPD
> > +-- based on a netcat version from Steve Jothen <sjothen at gmail dot com>
> > +-- (see http://github.com/otkrove/ion3-config/tree/master/mpd.lua)
> > +--
> > +-- Copyright (c) 2008-2009, Alexandre Perrin <kaw...@kaworu.ch>
> > +-- All rights reserved.
> > +--
> > +-- Redistribution and use in source and binary forms, with or without
> > +-- modification, are permitted provided that the following conditions
> > +-- are met:
> > +--
> > +-- 1. Redistributions of source code must retain the above copyright
> > +-- notice, this list of conditions and the following disclaimer.
> > +-- 2. Redistributions in binary form must reproduce the above copyright
> > +-- notice, this list of conditions and the following disclaimer in the
> > +-- documentation and/or other materials provided with the distribution.
> > +-- 4. Neither the name of the author nor the names of its contributors
> > +-- may be used to endorse or promote products derived from this software
> > +-- without specific prior written permission.
> > +--
> > +-- THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> > +-- ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > +-- IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> > PURPOSE
> > +-- ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
> > +-- FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > CONSEQUENTIAL
> > +-- DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> > +-- OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> > +-- HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> > STRICT
> > +-- LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY
> > WAY
> > +-- OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > +-- SUCH DAMAGE.
> [snip, haven't looked closely at the stuff after this]
>
> This screams "GPL INCOMPATIBLE". Ask jd if awesome modules have to be
> GPL-compatible, I'm not sure what the GPL says about scripting languages like
> lua.
I'm no lawyer, but I still kind of don't think so. Personally, I don't
care enough to investigate too deeply; no one's going to sue a small
open source project for having components that are open source licensed.
At least not yet.
Anyway, thanks for the review, and I'll do the fixing when I can. Cheers
:)
--
Andrei Thorp, Developer: Xandros Corp. (http://www.xandros.com)
--
To unsubscribe, send mail to awesome-devel-unsubscr...@naquadah.org.
