Merge lp:~marco-gallotta/ibid/ascii-501638 into lp:~ibid-core/ibid/old-trunk-1.6

[marcog]

[lp:~marco-gallotta/ibid/ascii-501638]

810. By Marco Gallotta <marco@me> on 2009-12-30

Fix typo; change 'figlet list fonts

811. By Marco Gallotta <marco@me> on 2009-12-30

Fix caca rendering by writing directly to file rather than going through Image lib

[Michael Gorven (mgorven)]

I'd prefer the non-stdlib imports to be grouped separately. Also not mad about
the '/tmp/pyfiglet-0.4/fonts.zip' default, but as discussed on IRC the pyfiglet
package is kak and there's not really a good default.

[Stefano Rivera (stefanor)]

good imports ordering is:
stdlib

otherlibs

ibidlibs

code

Look at other plugins that call out to see how we handle binaries and paths. i.e. the stuff in setup that aborts loading if the binary isn't in $PATH.
We're not entirely happy with it, but it's the current standard.

> lib = "aa" if colour is None else "caca"

Conditional statements are only in Python >= 2.5, we support python >= 2.4.
Applies elsewhere too

> try ... except: event.addresponse(u'Cannot fetch %s' % url)

Is that totally necessary? We try to catch as few exceptions as possible and when we do catch them, return a suitable error. As soon as you catch an exception, the bot won't save a traceback so the devs / owners get no debugging information.

> event.addresponse(u'Sorry, don\'t understand lib %s' % lib)

Firstly, save a character by using double-quotes.
Second, do ", lib" rather than "% lib". Addresponse will support translation in the future. This applies to all your addresponses
Third, it complains about an internal variable (lib) rather than what the user sees ("colour")

> height = 30

That's enormous, maybe default to lower. I think it should have a configurable maximum height to limit spam.

> for line in screen.render().split('\n'):
> event.addresponse(unicode(line))

Use the new multiline API

> process = subprocess.Popen('%s -f irc %s %s %s' % (self.img2txt_bin, width, height, image),
> shell=True, stdout=subprocess.PIPE)

eek, that's begging for a shell injection. Rather use the arg-list-style popen, with shell=False if you can.

review: Needs Fixing

[lp:~marco-gallotta/ibid/ascii-501638]

812. By Marco Gallotta <marco@me> on 2009-12-30

use deaddressed for figlet to leave trailing punctuation

813. By Marco Gallotta <marco@me> on 2009-12-30

Update to handle api change for multiline responses

814. By Marco Gallotta <marco@me> on 2009-12-30

Handle unfound binary; remove conditional statements; remove some error messages; redo formatting of arg-based responses; reduce default height and make it configurable; remove potential shell injection

[marcog (marco-gallotta)]

Okay, I made changes based on all the comments below. Hope it's good.

> good imports ordering is:
> stdlib
>
> otherlibs
>
> ibidlibs
>
> code
>
> Look at other plugins that call out to see how we handle binaries and paths.
> i.e. the stuff in setup that aborts loading if the binary isn't in $PATH.
> We're not entirely happy with it, but it's the current standard.
>
> > lib = "aa" if colour is None else "caca"
>
> Conditional statements are only in Python >= 2.5, we support python >= 2.4.
> Applies elsewhere too
>
> > try ... except: event.addresponse(u'Cannot fetch %s' % url)
>
> Is that totally necessary? We try to catch as few exceptions as possible and
> when we do catch them, return a suitable error. As soon as you catch an
> exception, the bot won't save a traceback so the devs / owners get no
> debugging information.
>
> > event.addresponse(u'Sorry, don\'t understand lib %s' % lib)
>
> Firstly, save a character by using double-quotes.
> Second, do ", lib" rather than "% lib". Addresponse will support translation
> in the future. This applies to all your addresponses
> Third, it complains about an internal variable (lib) rather than what the user
> sees ("colour")
>
> > height = 30
>
> That's enormous, maybe default to lower. I think it should have a configurable
> maximum height to limit spam.
>
> > for line in screen.render().split('\n'):
> > event.addresponse(unicode(line))
>
> Use the new multiline API
>
> > process = subprocess.Popen('%s -f irc %s %s %s' % (self.img2txt_bin, width,
> height, image),
> > shell=True, stdout=subprocess.PIPE)
>
> eek, that's begging for a shell injection. Rather use the arg-list-style
> popen, with shell=False if you can.

review: Needs Resubmitting

[Stefano Rivera (stefanor)]

> === modified file 'ibid.ini'

Somehow you modified ibid.ini. (probably when merging from trunk).

> (?:\s+(in colou?r))?

You only compare this string to none, so you can say r'(\s+in\s+colou?r)?'

> event.addresponse(u'File too large (limit is', self.max_size, 'KiB)')

No, not quite like that. You still do a substitution, but it does it for you:

event.addresponse(u'File too large (limit is %i KiB)', self.max_size)

> event.addresponse(response.replace('\r', ''), address=False, conflate=False)

I suspect that'll throw a unicode warning.

> fonts_zip = Option('fonts_zip', 'Zip file containing figlet fonts', '/tmp/pyfiglet-0.4/fonts.zip')

We can't merge with that default. I'd make the path relative to ibid.config.base (the botdir).
I.e. 'data/figlet-fonts.zip'

Overall, I'd still like some limits. Maximum width & height of all possible ascii art output.
If it sees that it's making something too big, it should just return something like
"I'm sorry, I can't draw ascii graffiti that big"

review: Needs Fixing

[Stefano Rivera (stefanor)]

> event.addresponse(response.replace('\r', ''), address=False, conflate=False)

On my machine it doesn't include CRs (\r). BTW the Debian/Ubuntu package is caca-utils you can mention that in the doc at the top

[Stefano Rivera (stefanor)]

figlet also throws a unicode warning

[lp:~marco-gallotta/ibid/ascii-501638]

815. By Marco Gallotta <marco@me> on 2009-12-31

Touch up minor issues (unicode responses, fonts_zip default)

816. By Marco Gallotta <marco@me> on 2009-12-31

Improved resizing that maintains aspect ratio and implements size limitations; Syntax for size changed from 10x20 to width 10 height 20 to allow only one dimension to be set

[marcog (marco-gallotta)]

All these issues should be resolved in the latest commits.

I did a bit of refactoring on the resizing:
* input syntax changes from [<width>x<height>] to [width <width>] [height <height>], so that just one dimension can be set and the other determined from aspect ratio.
* I've made the assumed font size (used to work out the dimensions in text form) configurable; I think the defaults are quite reasonable but feel free to suggest alternatives.
* lowered the default height and selected what I feel are sane defaults for max_{width,height}.
* if the aspect ratio would make the calculated dimension too large, it squashes the image rather than throwing an error.
* if the supplied dimensions are too large, throw an error.

> > === modified file 'ibid.ini'
>
> Somehow you modified ibid.ini. (probably when merging from trunk).
>
> > (?:\s+(in colou?r))?
>
> You only compare this string to none, so you can say r'(\s+in\s+colou?r)?'
>
> > event.addresponse(u'File too large (limit is', self.max_size, 'KiB)')
>
> No, not quite like that. You still do a substitution, but it does it for you:
>
> event.addresponse(u'File too large (limit is %i KiB)', self.max_size)
>
> > event.addresponse(response.replace('\r', ''), address=False, conflate=False)
>
> I suspect that'll throw a unicode warning.
>
> > fonts_zip = Option('fonts_zip', 'Zip file containing figlet fonts',
> '/tmp/pyfiglet-0.4/fonts.zip')
>
> We can't merge with that default. I'd make the path relative to
> ibid.config.base (the botdir).
> I.e. 'data/figlet-fonts.zip'
>
> Overall, I'd still like some limits. Maximum width & height of all possible
> ascii art output.
> If it sees that it's making something too big, it should just return something
> like
> "I'm sorry, I can't draw ascii graffiti that big"

review: Needs Resubmitting

[marcog (marco-gallotta)]

> > event.addresponse(response.replace('\r', ''), address=False, conflate=False)
>
> On my machine it doesn't include CRs (\r). BTW the Debian/Ubuntu package is
> caca-utils you can mention that in the doc at the top

It definitely includes them on my box. Either way, it's safer to strip them cause if they aren't there it just won't do anything. I've mentioned the debian package (thanks for that).

[lp:~marco-gallotta/ibid/ascii-501638]

817. By Marco Gallotta <marco@me> on 2009-12-31

Rather than squishing or returning an error, just shrink the size of the image and let the user know we drew it smaller

[marcog (marco-gallotta)]

Changed the handling of requests that are too large after discussion with Max on IRC. Instead of squishing the aspect ratio / returning an error, it now draws it as large as it can at the original aspect ratio while remaining within the max_{width,height} and tells the user we drew it smaller than asked for.

[Michael Gorven (mgorven)]

 review approve

review: Approve

[Stefano Rivera (stefanor)]

> max_width = IntOption('max_width', 'Maximum width for ascii output', 60)
> max_height = IntOption('max_width', 'Maximum width for ascii output', 15)

^ Some typo there

But otherwise I approve

review: Approve

[marcog (marco-gallotta)]

> > max_width = IntOption('max_width', 'Maximum width for ascii output', 60)
> > max_height = IntOption('max_width', 'Maximum width for ascii output', 15)
>
> ^ Some typo there

Well spotted :) fixed!

[lp:~marco-gallotta/ibid/ascii-501638]

818. By Marco Gallotta <marco@me> on 2009-12-31

Fix typo (s/width/height/)

[Jonathan Hitchcock (vhata)]

review: Approve

=== added file 'ibid/plugins/ascii.py'
--- ibid/plugins/ascii.py	1970-01-01 00:00:00 +0000
+++ ibid/plugins/ascii.py	2009-12-31 17:46:13 +0000
@@ -0,0 +1,161 @@
+from cStringIO import StringIO
+import Image
+from os import remove
+import os.path
+import subprocess
+from sys import stderr
+from tempfile import mkstemp
+from urllib2 import urlopen
+from zipfile import ZipFile
+
+from aalib import AsciiScreen
+from pyfiglet import Figlet
+
+from ibid.config import Option, IntOption
+from ibid.plugins import Processor, match
+from ibid.utils import file_in_path
+
+"""
+Dependencies:
+  * libaa (Ubuntu's libaa1-dev package)
+  * libcaca (Ubuntu's caca-utils package)
+  * Imlib2 (Ubuntu's libimlib2-dev package)
+  * pyfiglet (http://sourceforge.net/projects/pyfiglet/)
+"""
+
+help = { 'draw'  : u'Retrieve images from the web and render them in ascii-art.',
+         'figlet': u'Render text in ascii-art using figlet.' }
+class DrawImage(Processor):
+    u"""draw <url> [in colour] [width <width>] [height <height>]"""
+    feature = 'draw'
+
+    max_filesize = IntOption('max_filesize', 'Only request this many KiB', 200)
+    def_height = IntOption('def_height', 'Default height for libaa output', 10)
+    max_width = IntOption('max_width', 'Maximum width for ascii output', 60)
+    max_height = IntOption('max_height', 'Maximum height for ascii output', 15)
+    font_width = IntOption('font_width', 'Font width assumed for output', 6)
+    font_height = IntOption('font_height', 'Font height assumed for output', 10)
+    img2txt_bin = Option('img2txt_bin', 'libcaca img2txt binary to use', 'img2txt')
+
+    def setup(self):
+        if not file_in_path(self.img2txt_bin):
+            raise Exception('Cannot locate img2txt executable')
+
+    @match(r'^draw\s+(\S+\.\S+)(\s+in\s+colou?r)?(?:\s+w(?:idth)?\s+(\d+))?(?:\s+h(?:eight)\s+(\d+))?$')
+    def draw(self, event, url, colour, width, height):
+        f = urlopen(url)
+
+        filesize = int(f.info().getheaders('Content-Length')[0])
+        if filesize > self.max_filesize * 1024:
+            event.addresponse(u'File too large (limit is %i KiB)', self.max_filesize)
+            return
+
+        try:
+            ext = os.path.splitext(url)[1]
+            image = mkstemp(suffix=ext)[1]
+            file = open(image, 'w')
+            file.write(f.read())
+            file.close()
+
+            try:
+                img = Image.open(StringIO(open(image, 'r').read())).convert('L')
+            except:
+                event.addresponse(u'Cannot understand image format')
+                return
+            input_width, input_height = img.size[0], img.size[1]
+
+            set_size = width is not None or height is not None
+            if width is None and height is None:
+                height = self.def_height
+                width = height * input_width * self.font_height / input_height / self.font_width
+            elif width is None: # only height is set
+                height = int(height)
+                width = height * input_width * self.font_height / input_height / self.font_width
+            elif height is None: # only width is set
+                width = int(width)
+                height = width * input_height * self.font_width / input_width / self.font_height
+            else: # both width and height are set
+                width, height = int(width), int(height)
+
+            smaller = False
+            if width > self.max_width:
+                width = self.max_width
+                height = width * input_height * self.font_width / input_width / self.font_height
+                smaller = True
+            if height > self.max_height:
+                height = self.max_height
+                width = height * input_width * self.font_height / input_height / self.font_width
+                smaller = True
+
+            if colour is None:
+                self.draw_aa(event, image, width, height)
+            else:
+                self.draw_caca(event, image, width, height)
+
+            if set_size and smaller:
+                event.addresponse(u'Sorry, I drew that smaller than you asked for')
+        finally:
+            remove(image)
+
+    def draw_aa(self, event, image, width, height):
+        try:
+            image = Image.open(StringIO(open(image, 'r').read())).convert('L')
+        except:
+            event.addresponse(u'Cannot understand image format')
+            return
+        screen = AsciiScreen(width=width, height=height)
+        image = image.resize(screen.virtual_size)
+        screen.put_image((0, 0), image)
+        event.addresponse(unicode(screen.render()), address=False, conflate=False)
+
+    def draw_caca(self, event, image, width, height):
+        from sys import stderr
+        process = subprocess.Popen(
+            [self.img2txt_bin, '-f', 'irc', '-W', str(width), '-H', str(height), image],
+            shell=False, stdout=subprocess.PIPE)
+        response, error = process.communicate()
+        code = process.wait()
+        if code == 0:
+            event.addresponse(unicode(response.replace('\r', '')), address=False, conflate=False)
+        else:
+            event.addresponse(u'Sorry, cannot understand image format')
+
+class WriteFiglet(Processor):
+    u"""figlet <text> [in <font>]
+    list figlet fonts [from <index>]"""
+    feature = 'figlet'
+
+    fonts_zip = Option('fonts_zip', 'Zip file containing figlet fonts', 'data/figlet-fonts.zip')
+
+    def __init__(self, name):
+        Processor.__init__(self, name)
+        zip = ZipFile(self.fonts_zip)
+        self.fonts = sorted(map(lambda n: os.path.splitext(os.path.split(n)[1])[0], zip.namelist()))
+
+    @match(r'^list\s+figlet\s+fonts(?:\s+from\s+(\d+))?$')
+    def list_fonts(self, event, index):
+        if index is None:
+            index = 0
+        index = int(index)
+        if index >= len(self.fonts):
+            event.addresponse(u'I wish I had that many fonts installed')
+            return
+        event.addresponse(unicode(', '.join(self.fonts[int(index):])))
+
+    @match(r'^figlet\s+(.+?)(\s+in\s+(\S+))?$', 'deaddressed')
+    def write(self, event, text, font_phrase, font):
+        if font is not None and font not in self.fonts:
+            text = '%s%s' % (text, font_phrase)
+            font = None
+        if font is None:
+            font = 'slant'
+        self._write(event, text, font)
+
+    def _write(self, event, text, font):
+        figlet = Figlet(font=font, zipfile=self.fonts_zip)
+        rendered = figlet.renderText(text).split('\n')
+        while rendered and rendered[0].strip() == '':
+            del rendered[0]
+        while rendered and rendered[-1].strip() == '':
+            del rendered[-1]
+        event.addresponse(unicode('\n'.join(rendered)), address=False, conflate=False)